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

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

On Thu, Apr 15, 2021 at 12:37:20PM +0200, Jan Beulich wrote:
> 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.

Oh, I've worded that sentence wrongly I think. What I meant to say is
that for the purposes of the test here you could just fill the fields
with ~0 instead of using things like XSTATE_FP_SSE?

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

As I understand this is change is partially motivated to avoid leaking
the hardware max number of leaves when not required. With Viridian
it's all software based, so we are not leaking any hardware details
AFAICT, and hence I would be fine with just using a fixed value.

Thanks, Roger.