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

[Julien Grall <julien.grall@linaro.org>]

Hi Tim,

On 03/20/2014 05:23 PM, Tim Deegan wrote:
> 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@linaro.org>
>> ---
>>  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@xen.org> (for the whole series)
> but it seems like at least the one after the VTTBR write could go?

Thanks for the review.

Yes, the isb() right after VTBBR can be removed.

Ian also pointed that unset HCR_VM bit is not useful. I will write an
incremental patch in the next to clean up the function.

Regards,

-- 
Julien Grall