[PATCH] target/i386: suppress CPUID leaves not defined by the CPU vendor

[PATCH] target/i386: suppress CPUID leaves not defined by the CPU vendor

[Michael Roth]

Currently all built-in CPUs report cache information via CPUID leaves 2
and 4, but these have never been defined for AMD. In the case of
SEV-SNP this can cause issues with CPUID enforcement. Address this by
allowing CPU types to suppress these via a new "x-vendor-cpuid-only"
CPU property, which is true by default, but switched off for older
machine types to maintain compatibility.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: zhenwei pi <pizhenwei@bytedance.com>
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 hw/i386/pc.c      | 1 +
 target/i386/cpu.c | 6 ++++++
 target/i386/cpu.h | 3 +++
 3 files changed, 10 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8e1220db72..aa79c5e0e6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -98,6 +98,7 @@ GlobalProperty pc_compat_6_0[] = {
     { "qemu64" "-" TYPE_X86_CPU, "family", "6" },
     { "qemu64" "-" TYPE_X86_CPU, "model", "6" },
     { "qemu64" "-" TYPE_X86_CPU, "stepping", "3" },
+    { TYPE_X86_CPU, "x-vendor-cpuid-only", "off" },
 };
 const size_t pc_compat_6_0_len = G_N_ELEMENTS(pc_compat_6_0);
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5f595a0d7e..0b6c921e5a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5155,6 +5155,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         if (cpu->cache_info_passthrough) {
             host_cpuid(index, 0, eax, ebx, ecx, edx);
             break;
+        } else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
+            *eax = *ebx = *ecx = *edx = 0;
+            break;
         }
         *eax = 1; /* Number of CPUID[EAX=2] calls required */
         *ebx = 0;
@@ -5176,6 +5179,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             if ((*eax & 31) && cs->nr_cores > 1) {
                 *eax |= (cs->nr_cores - 1) << 26;
             }
+        } else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
+            *eax = *ebx = *ecx = *edx = 0;
         } else {
             *eax = 0;
             switch (count) {
@@ -6647,6 +6652,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
     DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
     DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
+    DEFINE_PROP_BOOL("x-vendor-cpuid-only", X86CPU, vendor_cpuid_only, true),
     DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
     DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
     DEFINE_PROP_BOOL("kvm-no-smi-migration", X86CPU, kvm_no_smi_migration,
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 0f7ddbfeae..c4152634e7 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1746,6 +1746,9 @@ struct X86CPU {
     /* Enable auto level-increase for all CPUID leaves */
     bool full_cpuid_auto_level;
 
+    /* Only advertise CPUID leaves defined by the vendor */
+    bool vendor_cpuid_only;
+
     /* Enable auto level-increase for Intel Processor Trace leave */
     bool intel_pt_auto_level;
 
-- 
2.25.1

[PATCH] target/i386: Fix cpuid level for AMD

[Michael Roth]

From: zhenwei pi <pizhenwei@bytedance.com>

A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
should not be changed to 0x1f in multi-dies case.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: zhenwei pi <pizhenwei@bytedance.com>
Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support for multi-dies PCMachine)
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
* to maintain compatibility with older machine types, only implement
  this change when the CPU's "x-vendor-cpuid-only" property is false
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 target/i386/cpu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0b6c921e5a..289dd2b1d8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5950,8 +5950,15 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
             }
         }
 
-        /* CPU topology with multi-dies support requires CPUID[0x1F] */
-        if (env->nr_dies > 1) {
+        /*
+         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
+         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
+         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU, unless
+         * cpu->vendor_cpuid_only has been unset for compatibility with older
+         * machine types.
+         */
+        if ((env->nr_dies > 1) &&
+            (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) {
             x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
         }
 
-- 
2.25.1

Re: [PATCH] target/i386: Fix cpuid level for AMD

[Eduardo Habkost]

On Thu, Jul 08, 2021 at 12:06:41PM -0500, Michael Roth wrote:
> From: zhenwei pi <pizhenwei@bytedance.com>
> 
> A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
> should not be changed to 0x1f in multi-dies case.
> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: zhenwei pi <pizhenwei@bytedance.com>
> Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support for multi-dies PCMachine)
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> * to maintain compatibility with older machine types, only implement
>   this change when the CPU's "x-vendor-cpuid-only" property is false
> Signed-off-by: Michael Roth <michael.roth@amd.com>

Queued, thanks!

-- 
Eduardo

Re: [PATCH] target/i386: suppress CPUID leaves not defined by the CPU vendor

[Eduardo Habkost]

On Wed, Jul 07, 2021 at 07:36:23PM -0500, Michael Roth wrote:
> Currently all built-in CPUs report cache information via CPUID leaves 2
> and 4, but these have never been defined for AMD. In the case of
> SEV-SNP this can cause issues with CPUID enforcement. Address this by
> allowing CPU types to suppress these via a new "x-vendor-cpuid-only"
> CPU property, which is true by default, but switched off for older
> machine types to maintain compatibility.
> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: zhenwei pi <pizhenwei@bytedance.com>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>

Queued, thanks!

-- 
Eduardo

[PATCH] target/i386: Fix cpuid level for AMD

[zhenwei pi]

A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
should not be changed to 0x1f in multi-dies case.

Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
for multi-dies PCMachine)
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 target/i386/cpu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a9fe1662d3..3934c559e4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
             }
         }
 
-        /* CPU topology with multi-dies support requires CPUID[0x1F] */
-        if (env->nr_dies > 1) {
+        /*
+         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
+         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
+         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.
+         */
+        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
             x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
         }
 
-- 
2.25.1

Re: [PATCH] target/i386: Fix cpuid level for AMD

[Dr. David Alan Gilbert]

* zhenwei pi (pizhenwei@bytedance.com) wrote:
> A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
> should not be changed to 0x1f in multi-dies case.
> 
> Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
> for multi-dies PCMachine)
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

(Copying in Babu)

Hmm I think you're right.  I've cc'd in Babu and Wei.

Eduardo: What do we need to do about compatibility, do we need to wire
this to machine type or CPU version?

Dave

> ---
>  target/i386/cpu.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a9fe1662d3..3934c559e4 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>              }
>          }
>  
> -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
> -        if (env->nr_dies > 1) {
> +        /*
> +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
> +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
> +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.
> +         */
> +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
>              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
>          }
>  
> -- 
> 2.25.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] target/i386: Fix cpuid level for AMD

[Babu Moger]

On 6/29/21 9:06 AM, Dr. David Alan Gilbert wrote:
> * zhenwei pi (pizhenwei@bytedance.com) wrote:
>> A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
>> should not be changed to 0x1f in multi-dies case.
>>
>> Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
>> for multi-dies PCMachine)
>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> 
> (Copying in Babu)
> 
> Hmm I think you're right.  I've cc'd in Babu and Wei.
> 
> Eduardo: What do we need to do about compatibility, do we need to wire
> this to machine type or CPU version?
> 
> Dave
> 
>> ---
>>  target/i386/cpu.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index a9fe1662d3..3934c559e4 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>>              }
>>          }
>>  
>> -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
>> -        if (env->nr_dies > 1) {
>> +        /*
>> +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
>> +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
>> +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.

The patch appears correct to me. AMD should use leaf 0xB to detect
extended topology. What is the problem here? Or is it just correcting the
cpuid based on the SPECS?

AMD uses nr_dies to simulate some topology. I dont know if it could become
a problem after this patch.


>> +         */
>> +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
>>              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
>>          }
>>  
>> -- 
>> 2.25.1
>>
>>

Re: [PATCH] target/i386: Fix cpuid level for AMD

[Michael Roth]

Quoting Dr. David Alan Gilbert (2021-06-29 09:06:02)
> * zhenwei pi (pizhenwei@bytedance.com) wrote:
> > A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
> > should not be changed to 0x1f in multi-dies case.
> > 
> > Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
> > for multi-dies PCMachine)
> > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> 
> (Copying in Babu)
> 
> Hmm I think you're right.  I've cc'd in Babu and Wei.
> 
> Eduardo: What do we need to do about compatibility, do we need to wire
> this to machine type or CPU version?

FWIW, there are some other CPUID entries like leaves 2 and 4 that are
also Intel-specific. With SEV-SNP CPUID enforcement, advertising them to
guests will result in failures when host SNP firmware checks the
hypervisor-provided CPUID values against the host-supported ones.

To address this we've been planning to add an 'amd-cpuid-only' property
to suppress them:

  https://github.com/mdroth/qemu/commit/28d0553fe748d30a8af09e5e58a7da3eff03e21b

My thinking is this property should be off by default, and only defined
either via explicit command-line option, or via new CPU types. We're also
planning to add new CPU versions for EPYC* CPU types that set this
'amd-cpuid-only' property by default:

  https://github.com/mdroth/qemu/commits/new-cpu-types-upstream

So in general I think maybe this change should be similarly controlled by
this proposed 'amd-cpuid-only' property. Maybe for this particular case it's
okay to do it unconditionally, but it sounds bad to switch up the valid CPUID
range after a guest has already booted (which might happen with old->new
migration for instance), since it might continue treating values in the range
as valid afterward (but again, not sure that's the case here or not).

There's some other changes with the new CPU types that we're still
considering/testing internally, but should be able to post them in some form
next week.

-Mike

> 
> Dave
> 
> > ---
> >  target/i386/cpu.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index a9fe1662d3..3934c559e4 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> >              }
> >          }
> >  
> > -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
> > -        if (env->nr_dies > 1) {
> > +        /*
> > +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
> > +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
> > +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.
> > +         */
> > +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
> >              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
> >          }
> >  
> > -- 
> > 2.25.1
> > 
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
>

Re: [PATCH] target/i386: Fix cpuid level for AMD

[Igor Mammedov]

On Wed, 30 Jun 2021 14:18:09 -0500
Michael Roth <michael.roth@amd.com> wrote:

> Quoting Dr. David Alan Gilbert (2021-06-29 09:06:02)
> > * zhenwei pi (pizhenwei@bytedance.com) wrote:  
> > > A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
> > > should not be changed to 0x1f in multi-dies case.
> > > 
> > > Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
> > > for multi-dies PCMachine)
> > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>  
> > 
> > (Copying in Babu)
> > 
> > Hmm I think you're right.  I've cc'd in Babu and Wei.
> > 
> > Eduardo: What do we need to do about compatibility, do we need to wire
> > this to machine type or CPU version?  
> 
> FWIW, there are some other CPUID entries like leaves 2 and 4 that are
> also Intel-specific. With SEV-SNP CPUID enforcement, advertising them to
> guests will result in failures when host SNP firmware checks the
> hypervisor-provided CPUID values against the host-supported ones.
> 
> To address this we've been planning to add an 'amd-cpuid-only' property
> to suppress them:
> 
>   https://github.com/mdroth/qemu/commit/28d0553fe748d30a8af09e5e58a7da3eff03e21b
> 
> My thinking is this property should be off by default, and only defined
> either via explicit command-line option, or via new CPU types. We're also
> planning to add new CPU versions for EPYC* CPU types that set this
> 'amd-cpuid-only' property by default:
> 
>   https://github.com/mdroth/qemu/commits/new-cpu-types-upstream
It look like having new cpu versions is enough to change behavior,
maybe keep 'amd-cpuid-only' as internal field and not expose it to users
as a property.

> So in general I think maybe this change should be similarly controlled by
> this proposed 'amd-cpuid-only' property. Maybe for this particular case it's
> okay to do it unconditionally, but it sounds bad to switch up the valid CPUID
> range after a guest has already booted (which might happen with old->new
> migration for instance), since it might continue treating values in the range
> as valid afterward (but again, not sure that's the case here or not).
> 
> There's some other changes with the new CPU types that we're still
> considering/testing internally, but should be able to post them in some form
> next week.
> 
> -Mike
> 
> > 
> > Dave
> >   
> > > ---
> > >  target/i386/cpu.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index a9fe1662d3..3934c559e4 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> > >              }
> > >          }
> > >  
> > > -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
> > > -        if (env->nr_dies > 1) {
> > > +        /*
> > > +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
> > > +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
> > > +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.
> > > +         */
> > > +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
> > >              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
> > >          }
> > >  
> > > -- 
> > > 2.25.1
> > > 
> > >   
> > -- 
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> >  
> 

Re: [PATCH] target/i386: Fix cpuid level for AMD

[Michael Roth]

Quoting Igor Mammedov (2021-07-01 03:43:13)
> On Wed, 30 Jun 2021 14:18:09 -0500
> Michael Roth <michael.roth@amd.com> wrote:
> 
> > Quoting Dr. David Alan Gilbert (2021-06-29 09:06:02)
> > > * zhenwei pi (pizhenwei@bytedance.com) wrote:  
> > > > A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
> > > > should not be changed to 0x1f in multi-dies case.
> > > > 
> > > > Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
> > > > for multi-dies PCMachine)
> > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>  
> > > 
> > > (Copying in Babu)
> > > 
> > > Hmm I think you're right.  I've cc'd in Babu and Wei.
> > > 
> > > Eduardo: What do we need to do about compatibility, do we need to wire
> > > this to machine type or CPU version?  
> > 
> > FWIW, there are some other CPUID entries like leaves 2 and 4 that are
> > also Intel-specific. With SEV-SNP CPUID enforcement, advertising them to
> > guests will result in failures when host SNP firmware checks the
> > hypervisor-provided CPUID values against the host-supported ones.
> > 
> > To address this we've been planning to add an 'amd-cpuid-only' property
> > to suppress them:
> > 
> >   https://github.com/mdroth/qemu/commit/28d0553fe748d30a8af09e5e58a7da3eff03e21b
> > 
> > My thinking is this property should be off by default, and only defined
> > either via explicit command-line option, or via new CPU types. We're also
> > planning to add new CPU versions for EPYC* CPU types that set this
> > 'amd-cpuid-only' property by default:
> > 
> >   https://github.com/mdroth/qemu/commits/new-cpu-types-upstream
> It look like having new cpu versions is enough to change behavior,
> maybe keep 'amd-cpuid-only' as internal field and not expose it to users
> as a property.

Hmm, I defined it as a property mainly to make use of
X86CPUVersionDefinition.props to create new versions of the CPU types
with those properties set.

There's a patch there that adds X86CPUVersionDefinition.cache_info so
that new cache definitions can be provided for new CPU versions. So
would you suggest a similar approach here, e.g. adding an
X86CPUVersionDefinition.amd_cpuid_only field that could be used directly
rather than going through X86CPUVersionDefinition.props?

There's also another new "amd-xsave" prop in that series that does something
similar to "amd-cpuid-only", so a little worried about tacking to much extra
into X86CPUVersionDefinition. But maybe that one could just be rolled into
"amd-cpuid-only" since it is basically fixing up xsave-related cpuid
entries for AMD...

> 
> > So in general I think maybe this change should be similarly controlled by
> > this proposed 'amd-cpuid-only' property. Maybe for this particular case it's
> > okay to do it unconditionally, but it sounds bad to switch up the valid CPUID
> > range after a guest has already booted (which might happen with old->new
> > migration for instance), since it might continue treating values in the range
> > as valid afterward (but again, not sure that's the case here or not).
> > 
> > There's some other changes with the new CPU types that we're still
> > considering/testing internally, but should be able to post them in some form
> > next week.
> > 
> > -Mike
> > 
> > > 
> > > Dave
> > >   
> > > > ---
> > > >  target/i386/cpu.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index a9fe1662d3..3934c559e4 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> > > >              }
> > > >          }
> > > >  
> > > > -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
> > > > -        if (env->nr_dies > 1) {
> > > > +        /*
> > > > +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
> > > > +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
> > > > +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.
> > > > +         */
> > > > +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
> > > >              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
> > > >          }
> > > >  
> > > > -- 
> > > > 2.25.1
> > > > 
> > > >   
> > > -- 
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > 
> > >  
> > 
> 
>

Re: [PATCH] target/i386: Fix cpuid level for AMD

[David Edmondson]

On Thursday, 2021-07-01 at 15:35:49 -05, Michael Roth wrote:

> Quoting Igor Mammedov (2021-07-01 03:43:13)
>> On Wed, 30 Jun 2021 14:18:09 -0500
>> Michael Roth <michael.roth@amd.com> wrote:
>> 
>> > Quoting Dr. David Alan Gilbert (2021-06-29 09:06:02)
>> > > * zhenwei pi (pizhenwei@bytedance.com) wrote:  
>> > > > A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
>> > > > should not be changed to 0x1f in multi-dies case.
>> > > > 
>> > > > Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
>> > > > for multi-dies PCMachine)
>> > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>  
>> > > 
>> > > (Copying in Babu)
>> > > 
>> > > Hmm I think you're right.  I've cc'd in Babu and Wei.
>> > > 
>> > > Eduardo: What do we need to do about compatibility, do we need to wire
>> > > this to machine type or CPU version?  
>> > 
>> > FWIW, there are some other CPUID entries like leaves 2 and 4 that are
>> > also Intel-specific. With SEV-SNP CPUID enforcement, advertising them to
>> > guests will result in failures when host SNP firmware checks the
>> > hypervisor-provided CPUID values against the host-supported ones.
>> > 
>> > To address this we've been planning to add an 'amd-cpuid-only' property
>> > to suppress them:
>> > 
>> >   https://github.com/mdroth/qemu/commit/28d0553fe748d30a8af09e5e58a7da3eff03e21b
>> > 
>> > My thinking is this property should be off by default, and only defined
>> > either via explicit command-line option, or via new CPU types. We're also
>> > planning to add new CPU versions for EPYC* CPU types that set this
>> > 'amd-cpuid-only' property by default:
>> > 
>> >   https://github.com/mdroth/qemu/commits/new-cpu-types-upstream
>> It look like having new cpu versions is enough to change behavior,
>> maybe keep 'amd-cpuid-only' as internal field and not expose it to users
>> as a property.
>
> Hmm, I defined it as a property mainly to make use of
> X86CPUVersionDefinition.props to create new versions of the CPU types
> with those properties set.
>
> There's a patch there that adds X86CPUVersionDefinition.cache_info so
> that new cache definitions can be provided for new CPU versions. So
> would you suggest a similar approach here, e.g. adding an
> X86CPUVersionDefinition.amd_cpuid_only field that could be used directly
> rather than going through X86CPUVersionDefinition.props?
>
> There's also another new "amd-xsave" prop in that series that does something
> similar to "amd-cpuid-only", so a little worried about tacking to much extra
> into X86CPUVersionDefinition. But maybe that one could just be rolled into
> "amd-cpuid-only" since it is basically fixing up xsave-related cpuid
> entries for AMD...

The XSAVE changes are similar to
https://lore.kernel.org/r/20210520145647.3483809-1-david.edmondson@oracle.com,
in response to which Paolo suggested that I should have QEMU observe the
CPUID 0xd leaves rather than encoding knowledge about the state offsets
(at least, that's how I understood his comment).

I have patches that do this (which includes making X86XSaveArea a
TCG-only thing) that I plan to send out in the next couple of days. They
should make "amd-xsave" unnecessary.

>> 
>> > So in general I think maybe this change should be similarly controlled by
>> > this proposed 'amd-cpuid-only' property. Maybe for this particular case it's
>> > okay to do it unconditionally, but it sounds bad to switch up the valid CPUID
>> > range after a guest has already booted (which might happen with old->new
>> > migration for instance), since it might continue treating values in the range
>> > as valid afterward (but again, not sure that's the case here or not).
>> > 
>> > There's some other changes with the new CPU types that we're still
>> > considering/testing internally, but should be able to post them in some form
>> > next week.
>> > 
>> > -Mike
>> > 
>> > > 
>> > > Dave
>> > >   
>> > > > ---
>> > > >  target/i386/cpu.c | 8 ++++++--
>> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
>> > > > 
>> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> > > > index a9fe1662d3..3934c559e4 100644
>> > > > --- a/target/i386/cpu.c
>> > > > +++ b/target/i386/cpu.c
>> > > > @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>> > > >              }
>> > > >          }
>> > > >  
>> > > > -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
>> > > > -        if (env->nr_dies > 1) {
>> > > > +        /*
>> > > > +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
>> > > > +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
>> > > > +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.
>> > > > +         */
>> > > > +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
>> > > >              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
>> > > >          }
>> > > >  
>> > > > -- 
>> > > > 2.25.1
>> > > > 
>> > > >   
>> > > -- 
>> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>> > > 
>> > >  
>> > 
>> 
>>

dme.
-- 
I had my eyes closed in the dark.

Re: [PATCH] target/i386: Fix cpuid level for AMD

[Michael Roth]

On Fri, Jul 02, 2021 at 07:50:03AM +0100, David Edmondson wrote:
> On Thursday, 2021-07-01 at 15:35:49 -05, Michael Roth wrote:
> 
> > Quoting Igor Mammedov (2021-07-01 03:43:13)
> >> On Wed, 30 Jun 2021 14:18:09 -0500
> >> Michael Roth <michael.roth@amd.com> wrote:
> >> 
> >> > Quoting Dr. David Alan Gilbert (2021-06-29 09:06:02)
> >> > > * zhenwei pi (pizhenwei@bytedance.com) wrote:  
> >> > > > A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
> >> > > > should not be changed to 0x1f in multi-dies case.
> >> > > > 
> >> > > > Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
> >> > > > for multi-dies PCMachine)
> >> > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>  
> >> > > 
> >> > > (Copying in Babu)
> >> > > 
> >> > > Hmm I think you're right.  I've cc'd in Babu and Wei.
> >> > > 
> >> > > Eduardo: What do we need to do about compatibility, do we need to wire
> >> > > this to machine type or CPU version?  
> >> > 
> >> > FWIW, there are some other CPUID entries like leaves 2 and 4 that are
> >> > also Intel-specific. With SEV-SNP CPUID enforcement, advertising them to
> >> > guests will result in failures when host SNP firmware checks the
> >> > hypervisor-provided CPUID values against the host-supported ones.
> >> > 
> >> > To address this we've been planning to add an 'amd-cpuid-only' property
> >> > to suppress them:
> >> > 
> >> >   https://github.com/mdroth/qemu/commit/28d0553fe748d30a8af09e5e58a7da3eff03e21b
> >> > 
> >> > My thinking is this property should be off by default, and only defined
> >> > either via explicit command-line option, or via new CPU types. We're also
> >> > planning to add new CPU versions for EPYC* CPU types that set this
> >> > 'amd-cpuid-only' property by default:
> >> > 
> >> >   https://github.com/mdroth/qemu/commits/new-cpu-types-upstream
> >> It look like having new cpu versions is enough to change behavior,
> >> maybe keep 'amd-cpuid-only' as internal field and not expose it to users
> >> as a property.
> >
> > Hmm, I defined it as a property mainly to make use of
> > X86CPUVersionDefinition.props to create new versions of the CPU types
> > with those properties set.
> >
> > There's a patch there that adds X86CPUVersionDefinition.cache_info so
> > that new cache definitions can be provided for new CPU versions. So
> > would you suggest a similar approach here, e.g. adding an
> > X86CPUVersionDefinition.amd_cpuid_only field that could be used directly
> > rather than going through X86CPUVersionDefinition.props?
> >
> > There's also another new "amd-xsave" prop in that series that does something
> > similar to "amd-cpuid-only", so a little worried about tacking to much extra
> > into X86CPUVersionDefinition. But maybe that one could just be rolled into
> > "amd-cpuid-only" since it is basically fixing up xsave-related cpuid
> > entries for AMD...
> 
> The XSAVE changes are similar to
> https://lore.kernel.org/r/20210520145647.3483809-1-david.edmondson@oracle.com,
> in response to which Paolo suggested that I should have QEMU observe the
> CPUID 0xd leaves rather than encoding knowledge about the state offsets
> (at least, that's how I understood his comment).
> 
> I have patches that do this (which includes making X86XSaveArea a
> TCG-only thing) that I plan to send out in the next couple of days. They
> should make "amd-xsave" unnecessary.

Ok, that does sounds like the better approach. Thanks for the heads up.

> 
> >> 
> >> > So in general I think maybe this change should be similarly controlled by
> >> > this proposed 'amd-cpuid-only' property. Maybe for this particular case it's
> >> > okay to do it unconditionally, but it sounds bad to switch up the valid CPUID
> >> > range after a guest has already booted (which might happen with old->new
> >> > migration for instance), since it might continue treating values in the range
> >> > as valid afterward (but again, not sure that's the case here or not).
> >> > 
> >> > There's some other changes with the new CPU types that we're still
> >> > considering/testing internally, but should be able to post them in some form
> >> > next week.
> >> > 
> >> > -Mike
> >> > 
> >> > > 
> >> > > Dave
> >> > >   
> >> > > > ---
> >> > > >  target/i386/cpu.c | 8 ++++++--
> >> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> >> > > > 
> >> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> > > > index a9fe1662d3..3934c559e4 100644
> >> > > > --- a/target/i386/cpu.c
> >> > > > +++ b/target/i386/cpu.c
> >> > > > @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> >> > > >              }
> >> > > >          }
> >> > > >  
> >> > > > -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
> >> > > > -        if (env->nr_dies > 1) {
> >> > > > +        /*
> >> > > > +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
> >> > > > +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
> >> > > > +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.
> >> > > > +         */
> >> > > > +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
> >> > > >              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
> >> > > >          }
> >> > > >  
> >> > > > -- 
> >> > > > 2.25.1
> >> > > > 
> >> > > >   
> >> > > -- 
> >> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >> > > 
> >> > >  
> >> > 
> >> 
> >>
> 
> dme.
> -- 
> I had my eyes closed in the dark.
> 

Re: [PATCH] target/i386: Fix cpuid level for AMD

[Eduardo Habkost]

On Wed, Jun 30, 2021 at 02:18:09PM -0500, Michael Roth wrote:
> Quoting Dr. David Alan Gilbert (2021-06-29 09:06:02)
> > * zhenwei pi (pizhenwei@bytedance.com) wrote:
> > > A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
> > > should not be changed to 0x1f in multi-dies case.
> > > 
> > > Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
> > > for multi-dies PCMachine)
> > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > 
> > (Copying in Babu)
> > 
> > Hmm I think you're right.  I've cc'd in Babu and Wei.
> > 
> > Eduardo: What do we need to do about compatibility, do we need to wire
> > this to machine type or CPU version?

If the change doesn't affect runnability of the CPU in a given
host (i.e. it doesn't introduce or remove host software or
hardware dependencies), it can be enabled for all CPU types in
newer machine types.

> 
> FWIW, there are some other CPUID entries like leaves 2 and 4 that are
> also Intel-specific. With SEV-SNP CPUID enforcement, advertising them to
> guests will result in failures when host SNP firmware checks the
> hypervisor-provided CPUID values against the host-supported ones.
> 
> To address this we've been planning to add an 'amd-cpuid-only' property
> to suppress them:
> 
>   https://github.com/mdroth/qemu/commit/28d0553fe748d30a8af09e5e58a7da3eff03e21b
> 
> My thinking is this property should be off by default, and only defined
> either via explicit command-line option, or via new CPU types. We're also
> planning to add new CPU versions for EPYC* CPU types that set this
> 'amd-cpuid-only' property by default:
> 
>   https://github.com/mdroth/qemu/commits/new-cpu-types-upstream

KVM has a hack that changes the CPUID vendor info depending on
the host (ignoring X86CPUDefinition.vendor completely).  For that
reason, I would make the new behavior tied to the actual CPU
vendor seen by the guest, not to the CPU type.  It will be a bit
more complicated, but less likely to cause problems when
management software tries to auto-detect the CPU model and
guesses a model from the wrong vendor.

We still need to keep compatibility somehow, though:

> 
> So in general I think maybe this change should be similarly controlled by
> this proposed 'amd-cpuid-only' property. Maybe for this particular case it's
> okay to do it unconditionally, but it sounds bad to switch up the valid CPUID
> range after a guest has already booted (which might happen with old->new
> migration for instance), since it might continue treating values in the range
> as valid afterward (but again, not sure that's the case here or not).

I agree, especially if the planned CPUID changes are more
intrusive than just CPUID level adjustments.

I suggest adding a "vendor-cpuid-only" property, that would
hide CPUID leaves depending on the actual CPUID vendor seen by
the guest.  Older machine types can set vendor-cpuid-only=off,
and newer machine-types would have vendor-cpuid-only=on by
default.


> 
> There's some other changes with the new CPU types that we're still
> considering/testing internally, but should be able to post them in some form
> next week.
> 
> -Mike
> 
> > 
> > Dave
> > 
> > > ---
> > >  target/i386/cpu.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index a9fe1662d3..3934c559e4 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> > >              }
> > >          }
> > >  
> > > -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
> > > -        if (env->nr_dies > 1) {
> > > +        /*
> > > +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
> > > +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should detect
> > > +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.
> > > +         */
> > > +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
> > >              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
> > >          }
> > >  
> > > -- 
> > > 2.25.1
> > > 
> > > 
> > -- 
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> >
> 

-- 
Eduardo