Re: [Xen-devel] [PATCH] xen/x86: Record xsave features in c->x86_capabilities

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/09/15 15:00, Jan Beulich wrote:
> > > > On 21.09.15 at 15:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 21/09/15 14:04, Jan Beulich wrote:
> > > > > > On 17.09.15 at 13:40, <andrew.cooper3@xxxxxxxxxx> wrote:
> > > > Jan: I have opted for adding leaf 8 rather than reusing leaf 2, due to the
> > > > uncertainty with how this information is exposed in libxl.  This patch
> > > > introduces no change with how the information is represented in userspace.
> > > Mind explaining this "uncertainty"? I'd like to avoid extending the array
> > > for no real reason...
> > libxl exports "hw_caps" as uint32_t caps[8] in its API.
> >
> > I am uncertain what reusing word 2, or extending the length of the array
> > means WRT to the API/ABI guarantees of libxl.
> >
> > For hw_caps itself, the data is essentially useless as there is no
> > defined layout,  Furthermore, some of the leaves are
> > arbitrary/synthetic.  One option might be to just drop it from libxl
> > entirely, but this will need to be decided by the toolstack maintainers.
> Even more so a reason to re-use word 2.
This works for 0xd:1.eax, but the array has to be extended for 0x7:0.ebx 
and 0x7:0.ecx, both of which are also included in my levelling series.
Currently I have just left the libxl question alone.

> > > > @@ -325,20 +321,15 @@ void xstate_init(bool_t bsp)
> > > >    
> > > >        /* Check extended XSAVE features. */
> > > >        cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
> > > > -    if ( bsp )
> > > > -    {
> > > > -        cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
> > > > -        cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC);
> > > > -        /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */
> > > > -        /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */
> > > > -    }
> > > > -    else
> > > > -    {
> > > > -        BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
> > > > -        BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC));
> > > > -        /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1)); */
> > > > -        /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */
> > > > -    }
> > > > +
> > > > +    /* Mask out features not currently understood by Xen. */
> > > > +    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> > > > +            cpufeat_mask(X86_FEATURE_XSAVEC));
> > > > +
> > > > +    c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
> > > > +
> > > > +    if ( !bsp )
> > > > +        BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
> > > >    }
> > > The !bsp conditional seems pretty pointless. And with the revised
> > > model it looks like it could be relaxed (BUG only when bits the BSP
> > > has set aren't set on the AP).
> > I would be very wary about allowing a situation where certain amounts of
> > heterogeneity would be permitted.  Even moreso with the xsaves
> > extensions, any non-homogeneity in the system will result in data
> > corruption.
> >
> > I think it is better to keep this as strictly that the BSP must match
> > all APs.  As soon as we encounter a system where this is not the case,
> > far more areas will also need modifying.
> I guess you misunderstood - I didn't mean for both lines to be
> dropped; I meant the if() surrounding the BUG_ON() to go away.
I don't mind dropping the if(), but I was querying your suggestion in 
brackets.
~Andrew

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