Re: [Xen-devel] [PATCH 2/2] x86/vmx: conditionally disable LBR support due to TSX format quirk

[Matt Wilson <msw@xxxxxxxx>]

On Wed, Aug 10, 2016 at 06:34:10AM -0600, Jan Beulich wrote:
> >>> On 10.08.16 at 08:59, <msw@xxxxxxxx> wrote:
> > Systems that support LBR formats that include TSX information but do
> > not support TSX require special handling when saving and restoring MSR
> > values. For example, see the Linux kernel quirks[1, 2] in the MSR
> > context switching code. As a wrmsr with certain values under these
> > conditions causes a #GP, VM entry will fail due to MSR loading (see
> > last bullet of SDM 26.4 "Loading MSRS").
> 
> I had recently noticed this as an area needing work too, so I'm glad
> you (hopefully) eliminate this item from my todo list. However, I'm
> sad to see that you really just disable LBR use in the problem case.
> I'd prefer to take such a change only as an immediate workaround,
> with the understanding that a proper one will be made available not
> too much later.

I agree that this is a short term workaround. I spent a while trying
to fix up the values but I wasn't successful.

> Jun, Kevin - workarounds for hardware quirks like this one are really
> what I would think should come out of Intel, and preferably not
> after the first problem report on Xen, but soon after the quirk has
> got identified in general (which according to the Linux patches must
> have been at least 2 months ago).

+1

> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2576,8 +2576,22 @@ static const struct lbr_info 
> > *last_branch_msr_get(void)
> >          /* Haswell */
> >          case 60: case 63: case 69: case 70:
> >          /* Broadwell */
> > -        case 61: case 71: case 79: case 86:
> > +        case 61: case 71: case 79: case 86: {
> > +            u64 caps;
> > +            bool_t tsx_support = boot_cpu_has(X86_FEATURE_HLE) ||
> > +                                 boot_cpu_has(X86_FEATURE_RTM);
> > +
> > +            rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps);
> 
> This is guarded by a X86_FEATURE_PDCM check in Linux - why
> would we not need the same here?

You're right, it should be. It also seems to be missing from
core2_vpmu_init().

> Also I think this RDMSR should be performed once at boot, not every
> time we come here.

I thought you might say that. It didn't seem obviously right to put
this in boot_cpu_data -- is that what you suggest?

> > +            /*
> > +             * Unimplemented fixups are required if the processor
> > +             * supports an LBR format that includes TSX information,
> > +             * but not TSX. Disable LBR save/load on such platforms.
> > +             */
> > +            if ( !tsx_support && (caps & 4) )
> 
> As I understand it the low 6 bits of the MSR contain an enumeration
> like value, so just checking bit 2 can't be right (and would wrongly
> trigger on already defined format types 5 and 6 - even if those were
> known to be impossible on the models currently covered, this would
> be a latent trap for someone to fall into when adding further model
> values).

Fair point.

--msw

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