Re: [PATCH v2 3/3] x86/vmx: fix vmentry failure with TSX bits in LBR

[Sergey Dyasli <sergey.dyasli@citrix.com>]

On Wed, 2017-02-22 at 03:26 -0700, Jan Beulich wrote:
> > > > On 17.02.17 at 16:42, <sergey.dyasli@citrix.com> wrote:
> > 
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2284,6 +2284,8 @@ static void pi_notification_interrupt(struct cpu_user_regs *regs)
> >      raise_softirq(VCPU_KICK_SOFTIRQ);
> >  }
> >  
> > +static void __init vmx_lbr_tsx_fixup_check(void);
> 
> vmx_ prefixes are pretty pointless for static functions in this
> particular source file (there's at least one more below).

I agree on that. Will remove in v3.

> 
> > @@ -2876,7 +2938,11 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
> >              for ( ; (rc == 0) && lbr->count; lbr++ )
> >                  for ( i = 0; (rc == 0) && (i < lbr->count); i++ )
> >                      if ( (rc = vmx_add_guest_msr(lbr->base + i)) == 0 )
> > +                    {
> >                          vmx_disable_intercept_for_msr(v, lbr->base + i, MSR_TYPE_R | MSR_TYPE_W);
> > +                        if ( vmx_lbr_tsx_fixup_needed )
> > +                            v->arch.hvm_vmx.lbr_tsx_fixup_enabled = true;
> 
> Is there anything wrong with
> 
>     v->arch.hvm_vmx.lbr_tsx_fixup_enabled = vmx_lbr_tsx_fixup_needed;
> 
> ?

Only 80 characters limit prevented me from doing it that way.

> 
> > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> > @@ -147,6 +147,7 @@ struct arch_vmx_struct {
> >      uint8_t              vmx_realmode;
> >      /* Are we emulating rather than VMENTERing? */
> >      uint8_t              vmx_emulate;
> > +    bool                 lbr_tsx_fixup_enabled;
> >      /* Bitmask of segments that we can't safely use in virtual 8086 mode */
> >      uint16_t             vm86_segment_mask;
> >      /* Shadow CS, SS, DS, ES, FS, GS, TR while in virtual 8086 mode */
> 
> Please put blank lines around your addition.

Will do in v3.

> 
> > --- a/xen/include/asm-x86/msr-index.h
> > +++ b/xen/include/asm-x86/msr-index.h
> > @@ -55,6 +55,8 @@
> >  #define MSR_IA32_PEBS_ENABLE		0x000003f1
> >  #define MSR_IA32_DS_AREA		0x00000600
> >  #define MSR_IA32_PERF_CAPABILITIES	0x00000345
> > +/* Lower 6 bits define the format of the address in the LBR stack */
> > +#define LBR_FORMAT_MSR_IA32_PERF_CAP	0x3f
> 
> Commonly the MSR name precedes the field specific name component.

I was trying to follow the naming style in that file but I might've
gotten it wrong. The new define can be renamed to a more appropriate
name, I'm open to suggestions.

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel