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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.
For my cpuid levelling series, there is a new set exported which has an 
ABI guarantee of which words map to which cpuid feature leaves, and a 
strict rule of no synthetic values.
> > @@ -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.
~Andrew

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