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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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?

Thanks, Roger.