Re: [Xen-devel] [PATCH 2/6] x86: save GUEST_BNDCFGS on context switch...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Tian, Kevin [mailto:kevin.tian@xxxxxxxxx]
> Sent: 28 January 2019 02:34
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Roger Pau
> Monne <roger.pau@xxxxxxxxxx>; Nakajima, Jun <jun.nakajima@xxxxxxxxx>
> Subject: RE: [PATCH 2/6] x86: save GUEST_BNDCFGS on context switch...
> 
> > From: Paul Durrant [mailto:paul.durrant@xxxxxxxxxx]
> > Sent: Monday, January 7, 2019 8:03 PM
> >
> > ...to avoid the need for a VMCS reload when the value of
> > MSR_IA32_BNDCFGS is
> > read by the tool-stack.
> 
> the frequency of context switch is much higher than the
> one of reading by tool-stack (at least in general case), then
> is it right thing to add a vmread for every context switch
> to save a VMCS reload in tool stack path?

AIUI the cost of the VMCS reload is sufficiently high (and the extra vmread 
sufficiently cheap) to make this desirable. 

Andrew, do you have any comment?

  Paul

> 
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> > Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/hvm.c     | 18 +++++++++++++++---
> >  xen/arch/x86/hvm/vmx/vmx.c |  3 +++
> >  xen/include/asm-x86/msr.h  |  5 +++++
> >  3 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 5fd5478b7d..b86aed7c24 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -311,6 +311,7 @@ int hvm_set_guest_pat(struct vcpu *v, u64
> > guest_pat)
> >  bool hvm_set_guest_bndcfgs(struct vcpu *v, uint64_t val)
> >  {
> >      const struct cpuid_policy *cp = v->domain->arch.cpuid;
> > +    struct vcpu_msrs *msrs = v->arch.msrs;
> >
> >      if ( !cp->feat.mpx )
> >          return false;
> > @@ -347,7 +348,8 @@ bool hvm_set_guest_bndcfgs(struct vcpu *v,
> > uint64_t val)
> >              /* nothing, best effort only */;
> >      }
> >
> > -    hvm_funcs.set_guest_bndcfgs(v, val);
> > +    msrs->bndcfgs.raw = val;
> > +    hvm_funcs.set_guest_bndcfgs(v, msrs->bndcfgs.raw);
> >
> >      return true;
> >  }
> > @@ -355,12 +357,22 @@ bool hvm_set_guest_bndcfgs(struct vcpu *v,
> > uint64_t val)
> >  bool hvm_get_guest_bndcfgs(struct vcpu *v, uint64_t *val)
> >  {
> >      const struct cpuid_policy *cp = v->domain->arch.cpuid;
> > +    struct vcpu_msrs *msrs = v->arch.msrs;
> >
> >      if ( !cp->feat.mpx )
> >          return false;
> >
> > -    ASSERT(hvm_funcs.get_guest_bndcfgs);
> > -    *val = hvm_funcs.get_guest_bndcfgs(v);
> > +    /*
> > +     * The value only need be read in current context as a context
> > +     * switch will save the value into msrs->bndcfgs.
> > +     */
> > +    if ( v == current )
> > +    {
> > +        ASSERT(hvm_funcs.get_guest_bndcfgs);
> > +        msrs->bndcfgs.raw = hvm_funcs.get_guest_bndcfgs(v);
> > +    }
> > +
> > +    *val = msrs->bndcfgs.raw;
> >
> >      return true;
> >  }
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index 4bfabe8d0e..7dba92da45 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -497,6 +497,9 @@ static void vmx_restore_host_msrs(void)
> >
> >  static void vmx_save_guest_msrs(struct vcpu *v)
> >  {
> > +    if ( cpu_has_mpx && cpu_has_vmx_mpx )
> > +        __vmread(GUEST_BNDCFGS, &v->arch.msrs->bndcfgs.raw);
> > +
> >      /*
> >       * We cannot cache SHADOW_GS_BASE while the VCPU runs, as it can
> >       * be updated at any time via SWAPGS, which we cannot trap.
> > diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
> > index ad8688a61f..c69ce56963 100644
> > --- a/xen/include/asm-x86/msr.h
> > +++ b/xen/include/asm-x86/msr.h
> > @@ -305,6 +305,11 @@ struct vcpu_msrs
> >       * values here may be stale in current context.
> >       */
> >      uint32_t dr_mask[4];
> > +
> > +    /* 0x00000d90 - MSR_IA32_BNDCFGS */
> > +    struct {
> > +        uint64_t raw;
> > +    } bndcfgs;
> >  };
> >
> >  void init_guest_msr_policy(void);
> > --
> > 2.20.1

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel