Re: [Xen-devel] [PATCH 03/27] x86/cpuid: Introduce struct cpuid_policy

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.17 at 16:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/01/17 14:22, Jan Beulich wrote:
>>>>> On 04.01.17 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> struct cpuid_policy will eventually be a complete replacement for the 
>>> cpuids[]
>>> array, with a fixed layout and named fields to allow O(1) access to specific
>>> information.
>>>
>>> For now, the CPUID content is capped at the 0xd and 0x8000001c leaves, which
>>> matches the maximum policy that the toolstack will generate for a domain.
>> Especially (but not only) leaf 0x17 and extended leaf 0x8000001e
>> make me wonder whether this is a good starting point.
> 
> The starting point matches what the toolstack currently does.
> 
> I'd prefer to logically separate this series (reworking how the
> hypervisor deals with CPUID data), from altering the default policy
> given to guests, but I do agree that we should move in the direction you
> suggest.

Okay.

>>> @@ -67,6 +80,55 @@ static void __init sanitise_featureset(uint32_t *fs)
>>>                            (fs[FEATURESET_e1d] & 
>>> ~CPUID_COMMON_1D_FEATURES));
>>>  }
>>>  
>>> +static void __init calculate_raw_policy(void)
>>> +{
>>> +    struct cpuid_policy *p = &raw_policy;
>>> +    unsigned int i;
>>> +
>>> +    cpuid_leaf(0, &p->basic.raw[0]);
>>> +    for ( i = 1; i < min(ARRAY_SIZE(p->basic.raw),
>>> +                         p->basic.max_leaf + 1ul); ++i )
>>> +    {
>>> +        /* Collected later. */
>>> +        if ( i == 0x7 || i == 0xd )
>>> +            continue;
>>> +
>>> +        cpuid_leaf(i, &p->basic.raw[i]);
>> Leaves 2, 4, 0xb, and 0xf are, iirc, a multiple invocation ones too.
>> There should at least be a comment here clarifying why they don't
>> need treatment similar to 7 and 0xd.
> 
> Leaf 2 is magic.  It doesn't take a subleaf parameter, but may return
> different information on repeated invocation.  I am half tempted to
> require this to be a static leaf, which appears to be the case on most
> hardware I have available to me.

Then we should have at least a warning logged somewhere if
multiple invocations would be needed, in the hopes that people
encountering it would tell us.

> The handling of leaf 4 is all per-cpu rather than per-domain, which is
> why it isn't expressed in this structure.  That is going to require the
> per-cpu topology work to do sensibly.  (There is a lot more CPUID work
> than is just presented in this series, but it was frankly getting
> unwieldy large.)

Well, okay, understood.

> 0xf isn't currently exposed (due to max_leaf being 0xd),

True.

>>> @@ -65,6 +66,78 @@ extern struct cpuidmasks cpuidmask_defaults;
>>>  /* Whether or not cpuid faulting is available for the current domain. */
>>>  DECLARE_PER_CPU(bool, cpuid_faulting_enabled);
>>>  
>>> +#define CPUID_GUEST_NR_BASIC      (0xdu + 1)
>>> +#define CPUID_GUEST_NR_FEAT       (0u + 1)
>>> +#define CPUID_GUEST_NR_XSTATE     (62u + 1)
>>> +#define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1)
>>> +#define CPUID_GUEST_NR_EXTD_AMD   (0x1cu + 1)
>>> +#define CPUID_GUEST_NR_EXTD       MAX(CPUID_GUEST_NR_EXTD_INTEL, \
>>> +                                      CPUID_GUEST_NR_EXTD_AMD)
>>> +
>>> +struct cpuid_policy
>>> +{
>>> +    /*
>>> +     * WARNING: During the CPUID transition period, not all information 
>>> here
>>> +     * is accurate.  The following items are accurate, and can be relied 
>>> upon.
>>> +     *
>>> +     * Global *_policy objects:
>>> +     *
>>> +     * - Host accurate:
>>> +     *   - max_{,sub}leaf
>>> +     *   - {xcr0,xss}_{high,low}
>>> +     *
>>> +     * - Guest appropriate:
>>> +     *   - Nothing
>> I don't understand the meaning of the "accurate" above and
>> the "appropriate" here.
> 
> This might make more sense in the context of patches 7 and 22, where we
> end up with a mix of host values, unsanitised and sanitised guest
> values.  This comment describes which values fall into which category,
> and is updated across the series.

Well - my main issue here is the use of two _different_ words,
whereas later for per-domain stuff you use the same word twice.

>>> +     *
>>> +     * Everything else should be considered inaccurate, and not 
>>> necesserily 
> 0.
>>> +     */
>>> +
>>> +    /* Basic leaves: 0x000000xx */
>>> +    union {
>>> +        struct cpuid_leaf raw[CPUID_GUEST_NR_BASIC];
>>> +        struct {
>>> +            /* Leaf 0x0 - Max and vendor. */
>>> +            struct {
>>> +                uint32_t max_leaf, :32, :32, :32;
>> Also is there really value in nesting unnamed structures like this?
> 
> It makes tools like `pahole` looking at struct cpuid_policy far clearer
> to read.  But like above, it aids clarity, particularly when adding in
> the higher numbered fields in later patches.

I've seen some of those later patches meanwhile, but I don't think
the double struct-s help (other than cluttering these already not
easy to look at declarations). The comments which are there
should be enough to separate groups.

Jan

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