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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Apr 19, 2021 at 02:29:38PM +0200, Jan Beulich wrote:
> On 19.04.2021 14:09, Roger Pau Monné wrote:
> > 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@suse.com>
> >>>> ---
> >>>> 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.
> 
> My take is that, just like for the x2APIC case, such leaves won't be
> all blank if the qualifying bit is set. Or if one ends up being all
> blank, a bug likely sits elsewhere.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I'm not able to spot any more dependencies ATM, so worse case we
discover some of those along the way and add the missing logic if
required, in any case seems like a fine starting point.

Thanks, Roger.