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

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

On Mon, Apr 19, 2021 at 01:46:02PM +0200, Jan Beulich wrote:
> 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?

Right, as type 0 is invalid according to Intel SDM, so you will never
be able to shrink below 0xb while having x2APIC set.

I still wonder however if there's any other such dependency, where
shrinking the max cpuid leaf could force us to drop features exposed
in inferior leaves.

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

No, sorry, I was confused. What you do is fine, I would even (as said
in the previous patch) just report the max leaf unconditionally even
if empty, as we are not leaking any hardware state in this case.

Thanks, Roger.