Re: [PATCH v2 12/17] x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2021 14:30, Roger Pau Monné wrote:
> On Thu, Apr 15, 2021 at 12:37:20PM +0200, Jan Beulich wrote:
>> On 15.04.2021 11:52, Roger Pau Monné wrote:
>>> On Mon, Nov 23, 2020 at 03:33:03PM +0100, Jan Beulich wrote:
>>>> --- a/tools/tests/cpu-policy/test-cpu-policy.c
>>>> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
>>>> @@ -8,10 +8,13 @@
>>>>  #include <err.h>
>>>>  
>>>>  #include <xen-tools/libs.h>
>>>> +#include <xen/asm/x86-defns.h>
>>>>  #include <xen/asm/x86-vendors.h>
>>>>  #include <xen/lib/x86/cpu-policy.h>
>>>>  #include <xen/domctl.h>
>>>>  
>>>> +#define XSTATE_FP_SSE  (X86_XCR0_FP | X86_XCR0_SSE)
>>>
>>> This gets used only once...
>>>
>>>> +
>>>>  static unsigned int nr_failures;
>>>>  #define fail(fmt, ...)                          \
>>>>  ({                                              \
>>>> @@ -564,6 +567,103 @@ static void test_cpuid_out_of_range_clea
>>>>      }
>>>>  }
>>>>  
>>>> +static void test_cpuid_maximum_leaf_shrinking(void)
>>>> +{
>>>> +    static const struct test {
>>>> +        const char *name;
>>>> +        struct cpuid_policy p;
>>>> +    } tests[] = {
>>>> +        {
>>>> +            .name = "basic",
>>>> +            .p = {
>>>> +                /* Very basic information only. */
>>>> +                .basic.max_leaf = 1,
>>>> +                .basic.raw_fms = 0xc2,
>>>> +            },
>>>> +        },
>>>> +        {
>>>> +            .name = "cache",
>>>> +            .p = {
>>>> +                /* Cache subleaves present. */
>>>> +                .basic.max_leaf = 4,
>>>> +                .cache.subleaf[0].type = 1,
>>>> +            },
>>>> +        },
>>>> +        {
>>>> +            .name = "feat#0",
>>>> +            .p = {
>>>> +                /* Subleaf 0 only with some valid bit. */
>>>> +                .basic.max_leaf = 7,
>>>> +                .feat.max_subleaf = 0,
>>>> +                .feat.fsgsbase = 1,
>>>> +            },
>>>> +        },
>>>> +        {
>>>> +            .name = "feat#1",
>>>> +            .p = {
>>>> +                /* Subleaf 1 only with some valid bit. */
>>>> +                .basic.max_leaf = 7,
>>>> +                .feat.max_subleaf = 1,
>>>> +                .feat.avx_vnni = 1,
>>>> +            },
>>>> +        },
>>>> +        {
>>>> +            .name = "topo",
>>>> +            .p = {
>>>> +                /* Topology subleaves present. */
>>>> +                .basic.max_leaf = 0xb,
>>>> +                .topo.subleaf[0].type = 1,
>>>> +            },
>>>> +        },
>>>> +        {
>>>> +            .name = "xstate",
>>>> +            .p = {
>>>> +                /* First subleaf always valid (and then non-zero). */
>>>> +                .basic.max_leaf = 0xd,
>>>> +                .xstate.xcr0_low = XSTATE_FP_SSE,
>>>
>>> ...here.
>>
>> For now, yes. I'm introducing the constant because I think it wants
>> using in other places too, to avoid using literal numbers. See e.g.
>>
>>                 .xstate.xcr0_low = 7,
>>
>> in test_cpuid_serialise_success().
>>
>>> And then I also wonder whether this requires having any
>>> specific values rather than just using ~0 or any random non-0 value.
>>
>> I'm afraid I don't understand: There's no ~0 here and no random
>> non-zero value - all other structure elements are left default-
>> initialized.
> 
> Oh, I've worded that sentence wrongly I think. What I meant to say is
> that for the purposes of the test here you could just fill the fields
> with ~0 instead of using things like XSTATE_FP_SSE?

The test would perhaps be fine, at least right now. But ~0 is not
really a legitimate value, especially if - for consistency - also
putting it in .xcr0_high. I wanted to have a well-defined, always
valid value here, avoiding the risk of needing to change the value
again later on.

Jan