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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 17 Mar 2017, Julien Grall wrote:
> Hi Wei,
> 
> On 03/17/2017 06:51 AM, Wei Chen wrote:
> > Hi Stefano,
> > 
> > On 2017/3/17 7:17, Stefano Stabellini wrote:
> > > 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.
> > > 
> > 
> > Ok, I think switch to macro is easier then static inline. I will switch
> > it to a macro in next version.
> 
> Nack from my side. We are getting ride of macro in Xen in favor of static
> inline in the whole. I see no point here to do the macro, except saving 4
> lines....
> 
> I don't want to see us in this game of trying to get the fewer number of lines
> just to claim we are small. Clarity and typesafety first.
> 
> In this case, a macro is not clarity nor safe.

/me shakes my head

As everybody knows, I am no fan of macros, but they have a place. In
fact, we have a few of them already in the asm-arm headers. But arguing
about this is a waste of my keystrokes.

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