Re: [Xen-devel] [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Tim Deegan <tim@xxxxxxx>]

At 15:43 +0000 on 19 Mar (1395240217), Julien Grall wrote:
> Introduce p2m_{save,restore}_state to save/restore p2m context.
> 
> The both functions will take care of:
>     - VTTBR: contains the pointer to the domain P2M
>     - Update HCR_RW if the domain is 64 bit
>     - SCTLR: contains bit to know if the MMU is enabled or not
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain.c     |   21 +++------------------
>  xen/arch/arm/p2m.c        |   28 ++++++++++++++++++++++++++++
>  xen/include/asm-arm/p2m.h |    4 ++++
>  3 files changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 46ee486..b125857 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -59,11 +59,12 @@ void idle_loop(void)
>  
>  static void ctxt_switch_from(struct vcpu *p)
>  {
> +    p2m_save_state(p);
> +
>      /* CP 15 */
>      p->arch.csselr = READ_SYSREG(CSSELR_EL1);
>  
>      /* Control Registers */
> -    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
>      p->arch.cpacr = READ_SYSREG(CPACR_EL1);
>  
>      p->arch.contextidr = READ_SYSREG(CONTEXTIDR_EL1);
> @@ -134,14 +135,7 @@ static void ctxt_switch_from(struct vcpu *p)
>  
>  static void ctxt_switch_to(struct vcpu *n)
>  {
> -    register_t hcr;
> -
> -    hcr = READ_SYSREG(HCR_EL2);
> -    WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
> -    isb();
> -
> -    p2m_load_VTTBR(n->domain);
> -    isb();
> +    p2m_restore_state(n);
>  
>      WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2);
>      WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
> @@ -189,7 +183,6 @@ static void ctxt_switch_to(struct vcpu *n)
>      isb();
>  
>      /* Control Registers */
> -    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
>      WRITE_SYSREG(n->arch.cpacr, CPACR_EL1);
>  
>      WRITE_SYSREG(n->arch.contextidr, CONTEXTIDR_EL1);
> @@ -214,14 +207,6 @@ static void ctxt_switch_to(struct vcpu *n)
>  
>      isb();
>  
> -    if ( is_32bit_domain(n->domain) )
> -        hcr &= ~HCR_RW;
> -    else
> -        hcr |= HCR_RW;
> -
> -    WRITE_SYSREG(hcr, HCR_EL2);
> -    isb();
> -
>      /* This is could trigger an hardware interrupt from the virtual
>       * timer. The interrupt needs to be injected into the guest. */
>      virt_timer_restore(n);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index b9d8ca6..979fe5b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -44,6 +44,34 @@ void p2m_load_VTTBR(struct domain *d)
>      isb(); /* Ensure update is visible */
>  }
>  
> +void p2m_save_state(struct vcpu *p)
> +{
> +    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
> +}
> +
> +void p2m_restore_state(struct vcpu *n)
> +{
> +    register_t hcr;
> +
> +    hcr = READ_SYSREG(HCR_EL2);
> +    WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
> +    isb();
> +
> +    p2m_load_VTTBR(n->domain);
> +    isb();
> +
> +    if ( is_32bit_domain(n->domain) )
> +        hcr &= ~HCR_RW;
> +    else
> +        hcr |= HCR_RW;
> +
> +    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
> +    isb();
> +
> +    WRITE_SYSREG(hcr, HCR_EL2);
> +    isb();
> +}

Are all of these isb()s necessary?  I guess this is only code motion,
so in any case, Acked-by: Tim Deegan <tim@xxxxxxx> (for the whole series)
but it seems like at least the one after the VTTBR write could go?

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel