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

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

[Jan Beulich]

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,
+            },
+        },
+        {
+            .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,
+            },
+        },
+        {
+            .name = "extd",
+            .p = {
+                /* Commonly available information only. */
+                .extd.max_leaf = 0x80000008,
+                .extd.maxphysaddr = 0x28,
+                .extd.maxlinaddr = 0x30,
+            },
+        },
+    };
+
+    printf("Testing CPUID maximum leaf shrinking:\n");
+
+    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+    {
+        const struct test *t = &tests[i];
+        struct cpuid_policy *p = memdup(&t->p);
+
+        p->basic.max_leaf = ARRAY_SIZE(p->basic.raw) - 1;
+        p->feat.max_subleaf = ARRAY_SIZE(p->feat.raw) - 1;
+        p->extd.max_leaf = 0x80000000 | (ARRAY_SIZE(p->extd.raw) - 1);
+
+        x86_cpuid_policy_shrink_max_leaves(p);
+
+        /* Check the the resulting max (sub)leaf values against expecations. */
+        if ( p->basic.max_leaf != t->p.basic.max_leaf )
+             fail("  Test %s basic fail - expected %#x, got %#x\n",
+                  t->name, t->p.basic.max_leaf, p->basic.max_leaf);
+
+        if ( p->extd.max_leaf != t->p.extd.max_leaf )
+             fail("  Test %s extd fail - expected %#x, got %#x\n",
+                  t->name, t->p.extd.max_leaf, p->extd.max_leaf);
+
+        if ( p->feat.max_subleaf != t->p.feat.max_subleaf )
+             fail("  Test %s feat fail - expected %#x, got %#x\n",
+                  t->name, t->p.feat.max_subleaf, p->feat.max_subleaf);
+
+        free(p);
+    }
+}
+
 static void test_is_compatible_success(void)
 {
     static struct test {
@@ -668,6 +768,7 @@ int main(int argc, char **argv)
     test_cpuid_serialise_success();
     test_cpuid_deserialise_failure();
     test_cpuid_out_of_range_clearing();
+    test_cpuid_maximum_leaf_shrinking();
 
     test_msr_serialise_success();
     test_msr_deserialise_failure();
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -341,6 +341,8 @@ static void __init calculate_host_policy
         p->extd.raw[0xa].d |= ((1u << SVM_FEATURE_VMCBCLEAN) |
                                (1u << SVM_FEATURE_TSCRATEMSR));
     }
+
+    x86_cpuid_policy_shrink_max_leaves(p);
 }
 
 static void __init guest_common_default_feature_adjustments(uint32_t *fs)
@@ -410,6 +412,8 @@ static void __init calculate_pv_max_poli
     recalculate_xstate(p);
 
     p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
+
+    x86_cpuid_policy_shrink_max_leaves(p);
 }
 
 static void __init calculate_pv_def_policy(void)
@@ -430,6 +434,8 @@ static void __init calculate_pv_def_poli
     sanitise_featureset(pv_featureset);
     cpuid_featureset_to_policy(pv_featureset, p);
     recalculate_xstate(p);
+
+    x86_cpuid_policy_shrink_max_leaves(p);
 }
 
 static void __init calculate_hvm_max_policy(void)
@@ -495,6 +501,8 @@ static void __init calculate_hvm_max_pol
     sanitise_featureset(hvm_featureset);
     cpuid_featureset_to_policy(hvm_featureset, p);
     recalculate_xstate(p);
+
+    x86_cpuid_policy_shrink_max_leaves(p);
 }
 
 static void __init calculate_hvm_def_policy(void)
@@ -519,6 +527,8 @@ static void __init calculate_hvm_def_pol
     sanitise_featureset(hvm_featureset);
     cpuid_featureset_to_policy(hvm_featureset, p);
     recalculate_xstate(p);
+
+    x86_cpuid_policy_shrink_max_leaves(p);
 }
 
 void __init init_guest_cpuid(void)
@@ -699,6 +709,8 @@ void recalculate_cpuid_policy(struct dom
 
     if ( !p->extd.page1gb )
         p->extd.raw[0x19] = EMPTY_LEAF;
+
+    x86_cpuid_policy_shrink_max_leaves(p);
 }
 
 int init_domain_cpuid_policy(struct domain *d)
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -124,7 +124,15 @@ void cpuid_viridian_leaves(const struct
     switch ( leaf )
     {
     case 0:
-        res->a = 0x40000006; /* Maximum leaf */
+        /* Maximum leaf */
+        cpuid_viridian_leaves(v, 0x40000006, 0, res);
+        if ( res->a | res->b | res->c | res->d )
+            res->a = 0x40000006;
+        else
+        {
+            cpuid_viridian_leaves(v, 0x40000005, 0, res);
+            res->a = 0x40000005 - !(res->a | res->b | res->c | res->d);
+        }
         memcpy(&res->b, "Micr", 4);
         memcpy(&res->c, "osof", 4);
         memcpy(&res->d, "t Hv", 4);
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -964,13 +964,15 @@ void cpuid_hypervisor_leaves(const struc
     uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
     uint32_t idx  = leaf - base;
     unsigned int limit = is_viridian_domain(d) ? p->hv2_limit : p->hv_limit;
+    unsigned int dflt = is_pv_domain(d) ? XEN_CPUID_MAX_PV_NUM_LEAVES
+                                        : XEN_CPUID_MAX_HVM_NUM_LEAVES;
 
     if ( limit == 0 )
         /* Default number of leaves */
-        limit = XEN_CPUID_MAX_NUM_LEAVES;
+        limit = dflt;
     else
         /* Clamp toolstack value between 2 and MAX_NUM_LEAVES. */
-        limit = min(max(limit, 2u), XEN_CPUID_MAX_NUM_LEAVES + 0u);
+        limit = min(max(limit, 2u), dflt);
 
     if ( idx > limit )
         return;
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -113,6 +113,10 @@
 /* Max. address width in bits taking memory hotplug into account. */
 #define XEN_CPUID_MACHINE_ADDRESS_WIDTH_MASK (0xffu << 0)
 
-#define XEN_CPUID_MAX_NUM_LEAVES 5
+#define XEN_CPUID_MAX_PV_NUM_LEAVES 5
+#define XEN_CPUID_MAX_HVM_NUM_LEAVES 4
+#define XEN_CPUID_MAX_NUM_LEAVES \
+    (XEN_CPUID_MAX_PV_NUM_LEAVES > XEN_CPUID_MAX_HVM_NUM_LEAVES ? \
+     XEN_CPUID_MAX_PV_NUM_LEAVES : XEN_CPUID_MAX_HVM_NUM_LEAVES)
 
 #endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -351,6 +351,13 @@ void x86_cpuid_policy_fill_native(struct
  */
 void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p);
 
+/**
+ * Shrink max leaf/subleaf values such that the last respective valid entry
+ * isn't all blank.  While permitted by the spec, such extraneous leaves may
+ * provide undue "hints" to guests.
+ */
+void x86_cpuid_policy_shrink_max_leaves(struct cpuid_policy *p);
+
 #ifdef __XEN__
 #include <public/arch-x86/xen.h>
 typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -236,6 +236,45 @@ void x86_cpuid_policy_clear_out_of_range
                 ARRAY_SIZE(p->extd.raw) - 1);
 }
 
+void x86_cpuid_policy_shrink_max_leaves(struct cpuid_policy *p)
+{
+    unsigned int i;
+
+    p->basic.raw[0x4] = p->cache.raw[0];
+
+    for ( i = p->feat.max_subleaf; i; --i )
+        if ( p->feat.raw[i].a | p->feat.raw[i].b |
+             p->feat.raw[i].c | p->feat.raw[i].d )
+            break;
+    p->feat.max_subleaf = i;
+    p->basic.raw[0x7] = p->feat.raw[i];
+
+    p->basic.raw[0xb] = p->topo.raw[0];
+
+    /*
+     * Due to the way xstate gets handled in the hypervisor (see
+     * recalculate_xstate()) there is (for now at least) no need to fiddle
+     * with the xstate subleaves (IOW we assume they're already in consistent
+     * shape, for coming from either hardware or recalculate_xstate()).
+     */
+    p->basic.raw[0xd] = p->xstate.raw[0];
+
+    for ( i = p->basic.max_leaf; i; --i )
+        if ( p->basic.raw[i].a | p->basic.raw[i].b |
+             p->basic.raw[i].c | p->basic.raw[i].d )
+            break;
+    p->basic.max_leaf = i;
+
+    for ( i = p->extd.max_leaf & 0xffff; i; --i )
+        if ( p->extd.raw[i].a | p->extd.raw[i].b |
+             p->extd.raw[i].c | p->extd.raw[i].d )
+            break;
+    if ( i | p->extd.raw[0].b | p->extd.raw[0].c | p->extd.raw[0].d )
+        p->extd.max_leaf = 0x80000000 | i;
+    else
+        p->extd.max_leaf = 0;
+}
+
 const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
 {
     static const uint32_t deep_features[] = INIT_DEEP_FEATURES;

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

[Roger Pau Monné]

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.

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.

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

[Jan Beulich]

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?

> 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

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

[Roger Pau Monné]

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.

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

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

[Jan Beulich]

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.

Jan

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

On 16.04.2021 15:16, 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>

First of all - I'm sorry Paul, I forgot to Cc you on the original
submission.

May I ask for an ack or otherwise for the Viridian part of this?
Please be sure, however, that you have seen the earlier discussion,
also on v2, as Roger is questioning whether the Viridian change
here wouldn't better be dropped.

Jan

> ---
> 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,
> +            },
> +        },
> +        {
> +            .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,
> +            },
> +        },
> +        {
> +            .name = "extd",
> +            .p = {
> +                /* Commonly available information only. */
> +                .extd.max_leaf = 0x80000008,
> +                .extd.maxphysaddr = 0x28,
> +                .extd.maxlinaddr = 0x30,
> +            },
> +        },
> +    };
> +
> +    printf("Testing CPUID maximum leaf shrinking:\n");
> +
> +    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> +    {
> +        const struct test *t = &tests[i];
> +        struct cpuid_policy *p = memdup(&t->p);
> +
> +        p->basic.max_leaf = ARRAY_SIZE(p->basic.raw) - 1;
> +        p->feat.max_subleaf = ARRAY_SIZE(p->feat.raw) - 1;
> +        p->extd.max_leaf = 0x80000000 | (ARRAY_SIZE(p->extd.raw) - 1);
> +
> +        x86_cpuid_policy_shrink_max_leaves(p);
> +
> +        /* Check the the resulting max (sub)leaf values against expecations. */
> +        if ( p->basic.max_leaf != t->p.basic.max_leaf )
> +             fail("  Test %s basic fail - expected %#x, got %#x\n",
> +                  t->name, t->p.basic.max_leaf, p->basic.max_leaf);
> +
> +        if ( p->extd.max_leaf != t->p.extd.max_leaf )
> +             fail("  Test %s extd fail - expected %#x, got %#x\n",
> +                  t->name, t->p.extd.max_leaf, p->extd.max_leaf);
> +
> +        if ( p->feat.max_subleaf != t->p.feat.max_subleaf )
> +             fail("  Test %s feat fail - expected %#x, got %#x\n",
> +                  t->name, t->p.feat.max_subleaf, p->feat.max_subleaf);
> +
> +        free(p);
> +    }
> +}
> +
>  static void test_is_compatible_success(void)
>  {
>      static struct test {
> @@ -668,6 +768,7 @@ int main(int argc, char **argv)
>      test_cpuid_serialise_success();
>      test_cpuid_deserialise_failure();
>      test_cpuid_out_of_range_clearing();
> +    test_cpuid_maximum_leaf_shrinking();
>  
>      test_msr_serialise_success();
>      test_msr_deserialise_failure();
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -341,6 +341,8 @@ static void __init calculate_host_policy
>          p->extd.raw[0xa].d |= ((1u << SVM_FEATURE_VMCBCLEAN) |
>                                 (1u << SVM_FEATURE_TSCRATEMSR));
>      }
> +
> +    x86_cpuid_policy_shrink_max_leaves(p);
>  }
>  
>  static void __init guest_common_default_feature_adjustments(uint32_t *fs)
> @@ -410,6 +412,8 @@ static void __init calculate_pv_max_poli
>      recalculate_xstate(p);
>  
>      p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
> +
> +    x86_cpuid_policy_shrink_max_leaves(p);
>  }
>  
>  static void __init calculate_pv_def_policy(void)
> @@ -430,6 +434,8 @@ static void __init calculate_pv_def_poli
>      sanitise_featureset(pv_featureset);
>      cpuid_featureset_to_policy(pv_featureset, p);
>      recalculate_xstate(p);
> +
> +    x86_cpuid_policy_shrink_max_leaves(p);
>  }
>  
>  static void __init calculate_hvm_max_policy(void)
> @@ -495,6 +501,8 @@ static void __init calculate_hvm_max_pol
>      sanitise_featureset(hvm_featureset);
>      cpuid_featureset_to_policy(hvm_featureset, p);
>      recalculate_xstate(p);
> +
> +    x86_cpuid_policy_shrink_max_leaves(p);
>  }
>  
>  static void __init calculate_hvm_def_policy(void)
> @@ -519,6 +527,8 @@ static void __init calculate_hvm_def_pol
>      sanitise_featureset(hvm_featureset);
>      cpuid_featureset_to_policy(hvm_featureset, p);
>      recalculate_xstate(p);
> +
> +    x86_cpuid_policy_shrink_max_leaves(p);
>  }
>  
>  void __init init_guest_cpuid(void)
> @@ -699,6 +709,8 @@ void recalculate_cpuid_policy(struct dom
>  
>      if ( !p->extd.page1gb )
>          p->extd.raw[0x19] = EMPTY_LEAF;
> +
> +    x86_cpuid_policy_shrink_max_leaves(p);
>  }
>  
>  int init_domain_cpuid_policy(struct domain *d)
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -124,7 +124,15 @@ void cpuid_viridian_leaves(const struct
>      switch ( leaf )
>      {
>      case 0:
> -        res->a = 0x40000006; /* Maximum leaf */
> +        /* Maximum leaf */
> +        cpuid_viridian_leaves(v, 0x40000006, 0, res);
> +        if ( res->a | res->b | res->c | res->d )
> +            res->a = 0x40000006;
> +        else
> +        {
> +            cpuid_viridian_leaves(v, 0x40000005, 0, res);
> +            res->a = 0x40000005 - !(res->a | res->b | res->c | res->d);
> +        }
>          memcpy(&res->b, "Micr", 4);
>          memcpy(&res->c, "osof", 4);
>          memcpy(&res->d, "t Hv", 4);
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -964,13 +964,15 @@ void cpuid_hypervisor_leaves(const struc
>      uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>      uint32_t idx  = leaf - base;
>      unsigned int limit = is_viridian_domain(d) ? p->hv2_limit : p->hv_limit;
> +    unsigned int dflt = is_pv_domain(d) ? XEN_CPUID_MAX_PV_NUM_LEAVES
> +                                        : XEN_CPUID_MAX_HVM_NUM_LEAVES;
>  
>      if ( limit == 0 )
>          /* Default number of leaves */
> -        limit = XEN_CPUID_MAX_NUM_LEAVES;
> +        limit = dflt;
>      else
>          /* Clamp toolstack value between 2 and MAX_NUM_LEAVES. */
> -        limit = min(max(limit, 2u), XEN_CPUID_MAX_NUM_LEAVES + 0u);
> +        limit = min(max(limit, 2u), dflt);
>  
>      if ( idx > limit )
>          return;
> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -113,6 +113,10 @@
>  /* Max. address width in bits taking memory hotplug into account. */
>  #define XEN_CPUID_MACHINE_ADDRESS_WIDTH_MASK (0xffu << 0)
>  
> -#define XEN_CPUID_MAX_NUM_LEAVES 5
> +#define XEN_CPUID_MAX_PV_NUM_LEAVES 5
> +#define XEN_CPUID_MAX_HVM_NUM_LEAVES 4
> +#define XEN_CPUID_MAX_NUM_LEAVES \
> +    (XEN_CPUID_MAX_PV_NUM_LEAVES > XEN_CPUID_MAX_HVM_NUM_LEAVES ? \
> +     XEN_CPUID_MAX_PV_NUM_LEAVES : XEN_CPUID_MAX_HVM_NUM_LEAVES)
>  
>  #endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */
> --- a/xen/include/xen/lib/x86/cpuid.h
> +++ b/xen/include/xen/lib/x86/cpuid.h
> @@ -351,6 +351,13 @@ void x86_cpuid_policy_fill_native(struct
>   */
>  void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p);
>  
> +/**
> + * Shrink max leaf/subleaf values such that the last respective valid entry
> + * isn't all blank.  While permitted by the spec, such extraneous leaves may
> + * provide undue "hints" to guests.
> + */
> +void x86_cpuid_policy_shrink_max_leaves(struct cpuid_policy *p);
> +
>  #ifdef __XEN__
>  #include <public/arch-x86/xen.h>
>  typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
> --- a/xen/lib/x86/cpuid.c
> +++ b/xen/lib/x86/cpuid.c
> @@ -236,6 +236,45 @@ void x86_cpuid_policy_clear_out_of_range
>                  ARRAY_SIZE(p->extd.raw) - 1);
>  }
>  
> +void x86_cpuid_policy_shrink_max_leaves(struct cpuid_policy *p)
> +{
> +    unsigned int i;
> +
> +    p->basic.raw[0x4] = p->cache.raw[0];
> +
> +    for ( i = p->feat.max_subleaf; i; --i )
> +        if ( p->feat.raw[i].a | p->feat.raw[i].b |
> +             p->feat.raw[i].c | p->feat.raw[i].d )
> +            break;
> +    p->feat.max_subleaf = i;
> +    p->basic.raw[0x7] = p->feat.raw[i];
> +
> +    p->basic.raw[0xb] = p->topo.raw[0];
> +
> +    /*
> +     * Due to the way xstate gets handled in the hypervisor (see
> +     * recalculate_xstate()) there is (for now at least) no need to fiddle
> +     * with the xstate subleaves (IOW we assume they're already in consistent
> +     * shape, for coming from either hardware or recalculate_xstate()).
> +     */
> +    p->basic.raw[0xd] = p->xstate.raw[0];
> +
> +    for ( i = p->basic.max_leaf; i; --i )
> +        if ( p->basic.raw[i].a | p->basic.raw[i].b |
> +             p->basic.raw[i].c | p->basic.raw[i].d )
> +            break;
> +    p->basic.max_leaf = i;
> +
> +    for ( i = p->extd.max_leaf & 0xffff; i; --i )
> +        if ( p->extd.raw[i].a | p->extd.raw[i].b |
> +             p->extd.raw[i].c | p->extd.raw[i].d )
> +            break;
> +    if ( i | p->extd.raw[0].b | p->extd.raw[0].c | p->extd.raw[0].d )
> +        p->extd.max_leaf = 0x80000000 | i;
> +    else
> +        p->extd.max_leaf = 0;
> +}
> +
>  const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
>  {
>      static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
> 

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

[Paul Durrant]

On 22/04/2021 12:38, Jan Beulich wrote:
> On 16.04.2021 15:16, 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>
> 
> First of all - I'm sorry Paul, I forgot to Cc you on the original
> submission.
> 

Ok. I did notice some discussion but wasn't really paying attention.

> May I ask for an ack or otherwise for the Viridian part of this?
> Please be sure, however, that you have seen the earlier discussion,
> also on v2, as Roger is questioning whether the Viridian change
> here wouldn't better be dropped.
> 

I confess that I'm not a fan of the recursive calls and I do agree with 
Roger that limiting the leaves simply because they have zero values is 
probably not the right thing to do and it could lead to issues with 
Windows. I think, to be on the safe side, it's best to leave the 
viridian code as-is.

   Paul

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

[Jan Beulich]

On 22.04.2021 14:34, Paul Durrant wrote:
> On 22/04/2021 12:38, Jan Beulich wrote:
>> On 16.04.2021 15:16, 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>
>>
>> First of all - I'm sorry Paul, I forgot to Cc you on the original
>> submission.
>>
> 
> Ok. I did notice some discussion but wasn't really paying attention.
> 
>> May I ask for an ack or otherwise for the Viridian part of this?
>> Please be sure, however, that you have seen the earlier discussion,
>> also on v2, as Roger is questioning whether the Viridian change
>> here wouldn't better be dropped.
>>
> 
> I confess that I'm not a fan of the recursive calls and I do agree with 
> Roger that limiting the leaves simply because they have zero values is 
> probably not the right thing to do and it could lead to issues with 
> Windows. I think, to be on the safe side, it's best to leave the 
> viridian code as-is.

Okay. In which case I have all needed acks, and the remaining part of
the change can go in.

Jan