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 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.

>> --- a/xen/arch/x86/hvm/viridian/viridian.c
>> +++ b/xen/arch/x86/hvm/viridian/viridian.c
>> @@ -121,7 +121,9 @@ void cpuid_viridian_leaves(const struct
>>      switch ( leaf )
>>      {
>>      case 0:
>> -        res->a = 0x40000006; /* Maximum leaf */
>> +        /* Maximum leaf */
>> +        cpuid_viridian_leaves(v, 0x40000006, 0, res);
>> +        res->a = res->a | res->b | res->c | res->d ? 0x40000006 : 
>> 0x40000004;
> 
> I think you would need to adjust this chunk to also take into account
> leaf 0x40000005 now.

Hmm, yes, looks like I failed to take note that I need to re-base
over that addition.

> I also wonder whether we should actually limit HyperV leaves. I think
> it's perfectly fine to report up to the maximum supported by Xen, even
> if it turns out none of the advertised feat are present, as in: Xen
> supports those leaves, but none of the features exposed are
> available.

Well, if the Viridian maintainers (I realize I failed to Cc Paul on the
original submission) think I should leave the Viridian leaves alone
(rather than handling consistently with other leaves), I can drop this
part of the change.

>> --- a/xen/include/public/arch-x86/cpuid.h
>> +++ b/xen/include/public/arch-x86/cpuid.h
>> @@ -113,6 +113,10 @@
>>  /* Max. address width in bits taking memory hotplug into account. */
>>  #define XEN_CPUID_MACHINE_ADDRESS_WIDTH_MASK (0xffu << 0)
>>  
>> -#define XEN_CPUID_MAX_NUM_LEAVES 5
>> +#define XEN_CPUID_MAX_PV_NUM_LEAVES 5
>> +#define XEN_CPUID_MAX_HVM_NUM_LEAVES 4
>> +#define XEN_CPUID_MAX_NUM_LEAVES \
>> +    (XEN_CPUID_MAX_PV_NUM_LEAVES > XEN_CPUID_MAX_HVM_NUM_LEAVES ? \
>> +     XEN_CPUID_MAX_PV_NUM_LEAVES : XEN_CPUID_MAX_HVM_NUM_LEAVES)
> 
> I guess we don't have any form of max macro available here to use?

I'm not aware of one that could be used in pre-processor directives
as well.

>> --- a/xen/lib/x86/cpuid.c
>> +++ b/xen/lib/x86/cpuid.c
>> @@ -238,6 +238,45 @@ void x86_cpuid_policy_clear_out_of_range
>>                  ARRAY_SIZE(p->extd.raw) - 1);
>>  }
>>  
>> +void x86_cpuid_policy_shrink_max_leaves(struct cpuid_policy *p)
>> +{
>> +    unsigned int i;
>> +
>> +    p->basic.raw[0x4] = p->cache.raw[0];
>> +
>> +    for ( i = p->feat.max_subleaf; i; --i )
>> +        if ( p->feat.raw[i].a | p->feat.raw[i].b |
>> +             p->feat.raw[i].c | p->feat.raw[i].d )
>> +            break;
>> +    p->feat.max_subleaf = i;
>> +    p->basic.raw[0x7] = p->feat.raw[0];
> 
> Do you need to use p->feat.raw[i] to assure the basic 0x7 leaf is seen
> as populated?
> 
> I think in theory raw[0] could be clear while raw[i] cannot as long as
> there's a populated leaf > 0 due to the check above.

Hmm, yes, this may not be very likely, but is definitely possible.

Jan