Re: [Xen-devel] [PATCH 03/18] xen/arm: Avoid setting/clearing HCR_RW at every context switch

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 16 Mar 2017, Julien Grall wrote:
> On 03/16/2017 10:40 PM, Stefano Stabellini wrote:
> > On Wed, 15 Mar 2017, Wei Chen wrote:
> > > Hi Stefano,
> > > 
> > > On 2017/3/15 8:25, Stefano Stabellini wrote:
> > > > On Mon, 13 Mar 2017, Wei Chen wrote:
> > > > > The HCR_EL2 flags for 64-bit and 32-bit domains are different. But
> > > > > when we initialized the HCR_EL2 for vcpu0 of Dom0 and all vcpus of
> > > > > DomU in vcpu_initialise, we didn't know the domain's address size
> > > > > information. We had to use compatible flags to initialize HCR_EL2,
> > > > > and set HCR_RW for 64-bit domain or clear HCR_RW for 32-bit domain
> > > > > at every context switch.
> > > > > 
> > > > > But, after we added the HCR_EL2 to vcpu's context, this behaviour
> > > > > seems a little fussy. We can update the HCR_RW bit in vcpu's context
> > > > > as soon as we get the domain's address size to avoid setting/clearing
> > > > > HCR_RW at every context switch.
> > > > > 
> > > > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > > > ---
> > > > >  xen/arch/arm/arm64/domctl.c  | 6 ++++++
> > > > >  xen/arch/arm/domain.c        | 5 +++++
> > > > >  xen/arch/arm/domain_build.c  | 7 +++++++
> > > > >  xen/arch/arm/p2m.c           | 5 -----
> > > > >  xen/include/asm-arm/domain.h | 1 +
> > > > >  5 files changed, 19 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> > > > > index 44e1e7b..ab8781f 100644
> > > > > --- a/xen/arch/arm/arm64/domctl.c
> > > > > +++ b/xen/arch/arm/arm64/domctl.c
> > > > > @@ -14,6 +14,8 @@
> > > > > 
> > > > >  static long switch_mode(struct domain *d, enum domain_type type)
> > > > >  {
> > > > > +    struct vcpu *v;
> > > > > +
> > > > >      if ( d == NULL )
> > > > >          return -EINVAL;
> > > > >      if ( d->tot_pages != 0 )
> > > > > @@ -23,6 +25,10 @@ static long switch_mode(struct domain *d, enum
> > > > > domain_type type)
> > > > > 
> > > > >      d->arch.type = type;
> > > > > 
> > > > > +    if ( is_64bit_domain(d) )
> > > > > +        for_each_vcpu(d, v)
> > > > > +            vcpu_switch_to_aarch64_mode(v);
> > > > > +
> > > > >      return 0;
> > > > >  }
> > > > > 
> > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > > > index 5d18bb0..69c2854 100644
> > > > > --- a/xen/arch/arm/domain.c
> > > > > +++ b/xen/arch/arm/domain.c
> > > > > @@ -537,6 +537,11 @@ void vcpu_destroy(struct vcpu *v)
> > > > >      free_xenheap_pages(v->arch.stack, STACK_ORDER);
> > > > >  }
> > > > > 
> > > > > +void vcpu_switch_to_aarch64_mode(struct vcpu *v)
> > > > > +{
> > > > > +    v->arch.hcr_el2 |= HCR_RW;
> > > > > +}
> > > > 
> > > > if possible, I would write it as a static inline function
> > > > 
> > > 
> > > I had tried to write it as a static inline function in asm/domain.h
> > > But while the first source file (arm64/asm-offsets.c) include
> > > xen/sched.h wanted to compile this inline function it could not find
> > > 'struct vcpu'. Because the xen/sched.h included the asm/domain.h
> > > but defined the vcpu type later. Even though we had included the
> > > xen/sched.h in asm/domain.h already.
> > 
> > It might be too complex to disentangle. In this case, there isn't much
> > type safety to be gained by using a static inline over a macro, so it
> > would be OK to use a macro for this.
> 
> It is not like vCPU will be switch to AArch64 mode often? Only once at vCPU
> creation time.
> 
> So what do we gain to switch to static inline or even macro?

To turn 5 lines of code into 1. Obviously it's no big deal.

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