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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2021 11:16, Roger Pau Monné wrote:
> Adding Paul also for the Viridian part.
> 
> On Fri, Apr 16, 2021 at 03:16:41PM +0200, Jan Beulich wrote:
>> Zapping leaf data for out of range leaves is just one half of it: To
>> avoid guests (bogusly or worse) inferring information from mere leaf
>> presence, also shrink maximum indicators such that the respective
>> trailing entry is not all blank (unless of course it's the initial
>> subleaf of a leaf that's not the final one).
>>
>> This is also in preparation of bumping the maximum basic leaf we
>> support, to ensure guests not getting exposed related features won't
>> observe a change in behavior.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v3: Record the actual non-empty subleaf in p->basic.raw[0x7], rather
>>     than subleaf 0. Re-base over Viridian leaf 40000005 addition.
>> v2: New.
>>
>> --- 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)
>> +
>>  static unsigned int nr_failures;
>>  #define fail(fmt, ...)                          \
>>  ({                                              \
>> @@ -553,6 +556,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,
> 
> On a private conversation with Andrew he raised the issue that the
> shrinking might be overly simplistic. For example if the x2APIC
> feature bit in leaf 1 is set then the max leaf should be at least 0xb
> in order to be able to fetch the x2APIC ID, even if it's 0.

But in such a case the "type" field of leaf 0xb's first sub-leaf is
going to be non-zero, isn't it?

> I also wonder if we are shrinking the leaves too much, for example we
> should always report up to 0x40000000 (or 0x40000100) plus the Xen
> leaves, as we never hide those and it's also documented in the public
> headers?

Not sure I follow - I'm likely confused by you quoting 0x40000000
and 0x40000100 rather than 0x400000nn and 0x400001nn, as elsewhere
you suggested we may not want to clip sub-leaves there. Can you
clarify whether you really mean only the first sub-leaves (each)
here, and if so why you say "up to"? Furthermore for the Xen leaves
I don't think I do excessive clipping ...

Jan