Re: [Xen-devel] [PATCH 02/18] xen/arm: Restore HCR_EL2 register

[Wei Chen <Wei.Chen@xxxxxxx>]

On 2017/3/15 8:25, Stefano Stabellini wrote:
> On Mon, 13 Mar 2017, Wei Chen wrote:
>> Different domains may have different HCR_EL2 flags. For example, the
>> 64-bit domain needs HCR_RW flag but the 32-bit does not need it. So
>> we give each domain a default HCR_EL2 value and save it in the VCPU's
>> context.
>>
>> HCR_EL2 register has only one bit can be updated automatically without
>> explicit write (HCR_VSE). But we haven't used this bit currently, so
>> we can consider that the HCR_EL2 register will not be modified while
>> the guest is running. So save the HCR_EL2 while guest exiting to
>> hypervisor is not neccessary. We just have to restore this register for
>> each VCPU while leaving hypervisor.
>>
>> We prefer to restore HCR_EL2 in leave_hypervisor_tail rather than in
>> ctxt_switch_to. Because the leave_hypervisor_tail is the closest place
>> to the exception return. In this case, we don't need to warrant the
>> HCR_EL2 will not be changed between ctxt_switch_to and exception return.
>
> Please explain a bit better why it is good to restore HCR_EL2 in
> leave_hypervisor_tail. Why is it a good thing that is closer to the
> exception return? What can be the cause of a change in HCR_EL2?
>

Ok, I will try to improve it in next version. In current Xen code, I
can't see any code would change the HCR_EL2 between ctxt_switch_to and
return_from_trap. But my concern is that, in the future, if someone
want to insert some HCR_EL2 change code between them, we can't guarantee
he will restore correct HCR_EL2 value for current vcpu.

>
>
>> Even though we have restored HCR_EL2 in leave_hypervisor_tail, we still
>> have to keep the write to HCR_EL2 in p2m_restore_state. That because
>> p2m_restore_state could be used to switch between two p2m and possibly
>> to do address translation using hardware. For instance when building
>> the hardware domain, we are using the instruction to before copying
>> data. During the translation, some bits of base register (such as SCTLR
>> and HCR) could be cached in TLB and used for the translation.
>>
>> We had some issues in the past (see commit b3cbe129d "xen: arm: Ensure
>> HCR_EL2.RW is set correctly when building dom0"), so we should probably
>> keep the write to HCR_EL2 in p2m_restore_state.
>>
>> Signed-off-by: wei chen <Wei.Chen@xxxxxxx>
>> ---
>>  xen/arch/arm/domain.c        |  2 ++
>>  xen/arch/arm/p2m.c           | 15 +++++++++------
>>  xen/arch/arm/traps.c         |  1 +
>>  xen/include/asm-arm/domain.h |  3 +++
>>  4 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index bb327da..5d18bb0 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -513,6 +513,8 @@ int vcpu_initialise(struct vcpu *v)
>>
>>      v->arch.actlr = READ_SYSREG32(ACTLR_EL1);
>>
>> +    v->arch.hcr_el2 = get_default_hcr_flags();
>> +
>>      processor_vcpu_initialise(v);
>>
>>      if ( (rc = vcpu_vgic_init(v)) != 0 )
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 1fc6ca3..c49bfa6 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -128,26 +128,29 @@ void p2m_save_state(struct vcpu *p)
>>
>>  void p2m_restore_state(struct vcpu *n)
>>  {
>> -    register_t hcr;
>>      struct p2m_domain *p2m = &n->domain->arch.p2m;
>>
>>      if ( is_idle_vcpu(n) )
>>          return;
>>
>> -    hcr = READ_SYSREG(HCR_EL2);
>> -
>>      WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
>>      isb();
>>
>>      if ( is_32bit_domain(n->domain) )
>> -        hcr &= ~HCR_RW;
>> +        n->arch.hcr_el2 &= ~HCR_RW;
>>      else
>> -        hcr |= HCR_RW;
>> +        n->arch.hcr_el2 |= HCR_RW;
>
> It makes sense to move this if/else statement to one of the vcpu
> initialization functions, but I can see that you are going to do that in
> a later patch, so that's OK.
>

Thanks.

>
>>      WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
>>      isb();
>>
>> -    WRITE_SYSREG(hcr, HCR_EL2);
>> +    /*
>> +     * p2m_restore_state could be used to switch between two p2m and 
>> possibly
>> +     * to do address translation using hardware. And these operations may
>> +     * happen during the interval between enter/leave hypervior, so we 
>> should
>> +     * probably keep the write to HCR_EL2 here.
>> +     */
>
> Please rewrite this to:
>
>   p2m_restore_state could be used to switch between two p2m and possibly
>   to do address translation using hardware. These operations may
>   happen during the interval between enter/leave hypervior, so we need
>   to restore the right HCR_EL2 here.
>

Thanks, I will update the code comment in next version.

>
>> +    WRITE_SYSREG(n->arch.hcr_el2, HCR_EL2);
>>      isb();
>>  }
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index d343c66..9792d02 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2811,6 +2811,7 @@ asmlinkage void leave_hypervisor_tail(void)
>>          local_irq_disable();
>>          if (!softirq_pending(smp_processor_id())) {
>>              gic_inject();
>
> Please add another in-code comment:
>
>   Set HCR_EL2 in leave_hypervisor_tail, because it is the closest code
>   site to the exception return and this is important because....
>

Ok, I will add code comment in next version.

>
>> +            WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2);
>>              return;
>>          }
>>          local_irq_enable();
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 09fe502..7b1dacc 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -204,6 +204,9 @@ struct arch_vcpu
>>      register_t tpidr_el1;
>>      register_t tpidrro_el0;
>>
>> +    /* HYP configuration */
>> +    register_t hcr_el2;
>> +
>>      uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
>>  #ifdef CONFIG_ARM_32
>>      /*
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel
>>
>


-- 
Regards,
Wei Chen

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