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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?



> 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.


>      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.


> +    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....


> +            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
> 

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