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.
>> --- 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.
>> --- 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)
>
> I guess we don't have any form of max macro available here to use?
I'm not aware of one that could be used in pre-processor directives
as well.
>> --- a/xen/lib/x86/cpuid.c
>> +++ b/xen/lib/x86/cpuid.c
>> @@ -238,6 +238,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[0];
>
> Do you need to use p->feat.raw[i] to assure the basic 0x7 leaf is seen
> as populated?
>
> I think in theory raw[0] could be clear while raw[i] cannot as long as
> there's a populated leaf > 0 due to the check above.
Hmm, yes, this may not be very likely, but is definitely possible.
Jan
