All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] target/i386: Fix cpuid level for AMD
@ 2021-06-28 13:20 zhenwei pi
  2021-06-29 14:06 ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 16+ messages in thread
From: zhenwei pi @ 2021-06-28 13:20 UTC (permalink / raw)
  To: ehabkost, pbonzini, richard.henderson, philmd, armbru
  Cc: zhenwei pi, qemu-devel, like.xu

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



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-06-28 13:20 [PATCH] target/i386: Fix cpuid level for AMD zhenwei pi
@ 2021-06-29 14:06 ` Dr. David Alan Gilbert
  2021-06-29 21:29   ` Babu Moger
  2021-06-30 19:18   ` Michael Roth
  0 siblings, 2 replies; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2021-06-29 14:06 UTC (permalink / raw)
  To: zhenwei pi, babu.moger, wei.huang2
  Cc: ehabkost, like.xu, richard.henderson, qemu-devel, armbru,
	pbonzini, philmd

* 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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-06-29 14:06 ` Dr. David Alan Gilbert
@ 2021-06-29 21:29   ` Babu Moger
  2021-06-30 19:18   ` Michael Roth
  1 sibling, 0 replies; 16+ messages in thread
From: Babu Moger @ 2021-06-29 21:29 UTC (permalink / raw)
  To: Dr. David Alan Gilbert, zhenwei pi, wei.huang2
  Cc: ehabkost, pbonzini, richard.henderson, philmd, armbru,
	qemu-devel, like.xu



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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-06-29 14:06 ` Dr. David Alan Gilbert
  2021-06-29 21:29   ` Babu Moger
@ 2021-06-30 19:18   ` Michael Roth
  2021-07-01  8:43     ` Igor Mammedov
  2021-07-02 17:32     ` Eduardo Habkost
  1 sibling, 2 replies; 16+ messages in thread
From: Michael Roth @ 2021-06-30 19:18 UTC (permalink / raw)
  To: Dr. David Alan Gilbert, babu.moger, wei.huang2, zhenwei pi
  Cc: ehabkost, like.xu, richard.henderson, qemu-devel, armbru,
	pbonzini, philmd

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-06-30 19:18   ` Michael Roth
@ 2021-07-01  8:43     ` Igor Mammedov
  2021-07-01 20:35       ` Michael Roth
  2021-07-02 17:32     ` Eduardo Habkost
  1 sibling, 1 reply; 16+ messages in thread
From: Igor Mammedov @ 2021-07-01  8:43 UTC (permalink / raw)
  To: Michael Roth
  Cc: ehabkost, like.xu, armbru, wei.huang2, richard.henderson,
	Dr. David Alan Gilbert, zhenwei pi, qemu-devel, babu.moger,
	pbonzini, philmd

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-01  8:43     ` Igor Mammedov
@ 2021-07-01 20:35       ` Michael Roth
  2021-07-02  5:14         ` [External] " zhenwei pi
  2021-07-02  6:50         ` David Edmondson
  0 siblings, 2 replies; 16+ messages in thread
From: Michael Roth @ 2021-07-01 20:35 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: ehabkost, like.xu, armbru, wei.huang2, richard.henderson,
	Dr. David Alan Gilbert, zhenwei pi, qemu-devel, babu.moger,
	pbonzini, philmd

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [External] Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-01 20:35       ` Michael Roth
@ 2021-07-02  5:14         ` zhenwei pi
  2021-07-02 15:43           ` Michael Roth
  2021-07-02  6:50         ` David Edmondson
  1 sibling, 1 reply; 16+ messages in thread
From: zhenwei pi @ 2021-07-02  5:14 UTC (permalink / raw)
  To: Michael Roth, Igor Mammedov
  Cc: ehabkost, like.xu, qemu-devel, wei.huang2, richard.henderson,
	Dr. David Alan Gilbert, armbru, babu.moger, pbonzini, philmd

On 7/2/21 4:35 AM, 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...
> 
Hi, this patch wants to fix the issue:
AMD CPU (Rome/Milan) should get the cpuid level 0x10, not 0x1F in the 
guest. If QEMU reports a 0x1F to guest, guest(Linux) would use leaf 0x1F 
instead of leaf 0xB to get extended topology:

https://github.com/torvalds/linux/blob/master/arch/x86/kernel/cpu/topology.c#L49

static int detect_extended_topology_leaf(struct cpuinfo_x86 *c)
{
	if (c->cpuid_level >= 0x1f) {
		if (check_extended_topology_leaf(0x1f) == 0)
			return 0x1f;
	}

	if (c->cpuid_level >= 0xb) {
		if (check_extended_topology_leaf(0xb) == 0)
			return 0xb;
	}

	return -1;
}

Because of the wrong cpuid level, the guest gets unexpected topology 
from leaf 0x1F.

I tested https://github.com/mdroth/qemu/commits/new-cpu-types-upstream, 
and it seems that these patches could not fix this issue.

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

-- 
zhenwei pi


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-01 20:35       ` Michael Roth
  2021-07-02  5:14         ` [External] " zhenwei pi
@ 2021-07-02  6:50         ` David Edmondson
  2021-07-02 15:40           ` Michael Roth
  1 sibling, 1 reply; 16+ messages in thread
From: David Edmondson @ 2021-07-02  6:50 UTC (permalink / raw)
  To: Michael Roth, Igor Mammedov
  Cc: ehabkost, like.xu, qemu-devel, wei.huang2, richard.henderson,
	Dr. David Alan Gilbert, zhenwei pi, armbru, babu.moger, pbonzini,
	philmd

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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-02  6:50         ` David Edmondson
@ 2021-07-02 15:40           ` Michael Roth
  0 siblings, 0 replies; 16+ messages in thread
From: Michael Roth @ 2021-07-02 15:40 UTC (permalink / raw)
  To: David Edmondson
  Cc: Igor Mammedov, ehabkost, like.xu, qemu-devel, wei.huang2,
	richard.henderson, Dr. David Alan Gilbert, zhenwei pi, armbru,
	babu.moger, pbonzini, philmd

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [External] Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-02  5:14         ` [External] " zhenwei pi
@ 2021-07-02 15:43           ` Michael Roth
  2021-07-02 17:35             ` Eduardo Habkost
  0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2021-07-02 15:43 UTC (permalink / raw)
  To: zhenwei pi
  Cc: Igor Mammedov, ehabkost, like.xu, qemu-devel, wei.huang2,
	richard.henderson, Dr. David Alan Gilbert, armbru, babu.moger,
	pbonzini, philmd

On Fri, Jul 02, 2021 at 01:14:56PM +0800, zhenwei pi wrote:
> On 7/2/21 4:35 AM, 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...
> > 
> Hi, this patch wants to fix the issue:
> AMD CPU (Rome/Milan) should get the cpuid level 0x10, not 0x1F in the guest.
> If QEMU reports a 0x1F to guest, guest(Linux) would use leaf 0x1F instead of
> leaf 0xB to get extended topology:
> 
> https://github.com/torvalds/linux/blob/master/arch/x86/kernel/cpu/topology.c#L49
> 
> static int detect_extended_topology_leaf(struct cpuinfo_x86 *c)
> {
> 	if (c->cpuid_level >= 0x1f) {
> 		if (check_extended_topology_leaf(0x1f) == 0)
> 			return 0x1f;
> 	}
> 
> 	if (c->cpuid_level >= 0xb) {
> 		if (check_extended_topology_leaf(0xb) == 0)
> 			return 0xb;
> 	}
> 
> 	return -1;
> }
> 
> Because of the wrong cpuid level, the guest gets unexpected topology from
> leaf 0x1F.
> 
> I tested https://github.com/mdroth/qemu/commits/new-cpu-types-upstream, and
> it seems that these patches could not fix this issue.

Yes, I think your patch would still be needed. The question is whether it's
okay to change it for existing CPU types, e.g. EPYC-Milan, or only for new ones
when they set a certain flag/property, like the proposed "amd-cpuid-only" (which
the proposed EPYC-Milan-v2 would set).

> 
> > > 
> > > > 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
> > > > > 
> > > > 
> > > 
> > > 
> 
> -- 
> zhenwei pi
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-06-30 19:18   ` Michael Roth
  2021-07-01  8:43     ` Igor Mammedov
@ 2021-07-02 17:32     ` Eduardo Habkost
  1 sibling, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2021-07-02 17:32 UTC (permalink / raw)
  To: Michael Roth
  Cc: like.xu, armbru, wei.huang2, richard.henderson,
	Dr. David Alan Gilbert, zhenwei pi, qemu-devel, babu.moger,
	pbonzini, philmd

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [External] Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-02 15:43           ` Michael Roth
@ 2021-07-02 17:35             ` Eduardo Habkost
  2021-07-08  5:11               ` Michael Roth
  0 siblings, 1 reply; 16+ messages in thread
From: Eduardo Habkost @ 2021-07-02 17:35 UTC (permalink / raw)
  To: Michael Roth
  Cc: like.xu, armbru, wei.huang2, richard.henderson, qemu-devel,
	zhenwei pi, Dr. David Alan Gilbert, babu.moger, pbonzini,
	Igor Mammedov, philmd

On Fri, Jul 02, 2021 at 10:43:22AM -0500, Michael Roth wrote:
> On Fri, Jul 02, 2021 at 01:14:56PM +0800, zhenwei pi wrote:
> > On 7/2/21 4:35 AM, 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...
> > > 
> > Hi, this patch wants to fix the issue:
> > AMD CPU (Rome/Milan) should get the cpuid level 0x10, not 0x1F in the guest.
> > If QEMU reports a 0x1F to guest, guest(Linux) would use leaf 0x1F instead of
> > leaf 0xB to get extended topology:
> > 
> > https://github.com/torvalds/linux/blob/master/arch/x86/kernel/cpu/topology.c#L49
> > 
> > static int detect_extended_topology_leaf(struct cpuinfo_x86 *c)
> > {
> > 	if (c->cpuid_level >= 0x1f) {
> > 		if (check_extended_topology_leaf(0x1f) == 0)
> > 			return 0x1f;
> > 	}
> > 
> > 	if (c->cpuid_level >= 0xb) {
> > 		if (check_extended_topology_leaf(0xb) == 0)
> > 			return 0xb;
> > 	}
> > 
> > 	return -1;
> > }
> > 
> > Because of the wrong cpuid level, the guest gets unexpected topology from
> > leaf 0x1F.
> > 
> > I tested https://github.com/mdroth/qemu/commits/new-cpu-types-upstream, and
> > it seems that these patches could not fix this issue.
> 
> Yes, I think your patch would still be needed. The question is whether it's
> okay to change it for existing CPU types, e.g. EPYC-Milan, or only for new ones
> when they set a certain flag/property, like the proposed "amd-cpuid-only" (which
> the proposed EPYC-Milan-v2 would set).

I tried to answer this in a separate reply in this thread, but
answering here for visibility:

You can safely do it on existing CPU types, because the new
behavior doesn't introduce host software or hardware requirements
when enabled.  You just need to disable the new behavior in
MachineClass.compat_props for older machine types.

-- 
Eduardo



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [External] Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-02 17:35             ` Eduardo Habkost
@ 2021-07-08  5:11               ` Michael Roth
  2021-07-08 13:09                 ` 皮振伟
  0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2021-07-08  5:11 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: like.xu, armbru, wei.huang2, richard.henderson, qemu-devel,
	zhenwei pi, Dr. David Alan Gilbert, babu.moger, pbonzini,
	Igor Mammedov, philmd

Quoting Eduardo Habkost (2021-07-02 12:35:34)
> On Fri, Jul 02, 2021 at 10:43:22AM -0500, Michael Roth wrote:
> > On Fri, Jul 02, 2021 at 01:14:56PM +0800, zhenwei pi wrote:
> > > On 7/2/21 4:35 AM, 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...
> > > > 
> > > Hi, this patch wants to fix the issue:
> > > AMD CPU (Rome/Milan) should get the cpuid level 0x10, not 0x1F in the guest.
> > > If QEMU reports a 0x1F to guest, guest(Linux) would use leaf 0x1F instead of
> > > leaf 0xB to get extended topology:
> > > 
> > > https://github.com/torvalds/linux/blob/master/arch/x86/kernel/cpu/topology.c#L49
> > > 
> > > static int detect_extended_topology_leaf(struct cpuinfo_x86 *c)
> > > {
> > >     if (c->cpuid_level >= 0x1f) {
> > >             if (check_extended_topology_leaf(0x1f) == 0)
> > >                     return 0x1f;
> > >     }
> > > 
> > >     if (c->cpuid_level >= 0xb) {
> > >             if (check_extended_topology_leaf(0xb) == 0)
> > >                     return 0xb;
> > >     }
> > > 
> > >     return -1;
> > > }
> > > 
> > > Because of the wrong cpuid level, the guest gets unexpected topology from
> > > leaf 0x1F.
> > > 
> > > I tested https://github.com/mdroth/qemu/commits/new-cpu-types-upstream, and
> > > it seems that these patches could not fix this issue.
> > 
> > Yes, I think your patch would still be needed. The question is whether it's
> > okay to change it for existing CPU types, e.g. EPYC-Milan, or only for new ones
> > when they set a certain flag/property, like the proposed "amd-cpuid-only" (which
> > the proposed EPYC-Milan-v2 would set).
> 
> I tried to answer this in a separate reply in this thread, but
> answering here for visibility:
> 
> You can safely do it on existing CPU types, because the new
> behavior doesn't introduce host software or hardware requirements
> when enabled.  You just need to disable the new behavior in
> MachineClass.compat_props for older machine types.

Hi Eduardo,

Thanks for the suggestions. Since the CPUID changes no longer rely on
adding new CPU models, I've broken that out as a separate patch here
based on your input:

  https://lists.nongnu.org/archive/html/qemu-devel/2021-07/msg01679.html

Zhenwei, with the above patch I think you can change your patch to use:

  if (!cpu->vendor_cpuid_only || IS_INTEL_CPU(env))
    //add intel-specific range

Let me know if you want me to update and add to my series.

> 
> -- 
> Eduardo
> 
>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-08  5:11               ` Michael Roth
@ 2021-07-08 13:09                 ` 皮振伟
  0 siblings, 0 replies; 16+ messages in thread
From: 皮振伟 @ 2021-07-08 13:09 UTC (permalink / raw)
  To: Michael Roth
  Cc: Eduardo Habkost, like.xu, wei.huang2, richard.henderson,
	qemu-devel, armbru, babu.moger, Igor Mammedov, pbonzini, philmd,
	Dr. David Alan Gilbert

[-- Attachment #1: Type: text/plain, Size: 5346 bytes --]

On Thu, Jul 8, 2021, 13:12 <michael.roth@amd.com> wrote:
Quoting Eduardo Habkost (2021-07-02 12:35:34) > On Fri, Jul 02, 2021 at
10:43:22AM -0500, Michael Roth wrote: > > On Fri, Jul 02, 2021 at
01:14:56PM +0800, zhenwei pi wrote: > > > On 7/2/21 4:35 AM, 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... > > > >
> > > Hi, this patch wants to fix the issue: > > > AMD CPU (Rome/Milan)
should get the cpuid level 0x10, not 0x1F in the guest. > > > If QEMU
reports a 0x1F to guest, guest(Linux) would use leaf 0x1F instead of > > >
leaf 0xB to get extended topology: > > > > > >
https://github.com/torvalds/linux/blob/master/arch/x86/kernel/cpu/topology.c#L49
> > > > > > static int detect_extended_topology_leaf(struct cpuinfo_x86 *c)
> > > { > > > if (c->cpuid_level >= 0x1f) { > > > if
(check_extended_topology_leaf(0x1f) == 0) > > > return 0x1f; > > > } > > >
> > > if (c->cpuid_level >= 0xb) { > > > if
(check_extended_topology_leaf(0xb) == 0) > > > return 0xb; > > > } > > > >
> > return -1; > > > } > > > > > > Because of the wrong cpuid level, the
guest gets unexpected topology from > > > leaf 0x1F. > > > > > > I tested
https://github.com/mdroth/qemu/commits/new-cpu-types-upstream, and > > > it
seems that these patches could not fix this issue. > > > > Yes, I think
your patch would still be needed. The question is whether it's > > okay to
change it for existing CPU types, e.g. EPYC-Milan, or only for new ones > >
when they set a certain flag/property, like the proposed "amd-cpuid-only"
(which > > the proposed EPYC-Milan-v2 would set). > > I tried to answer
this in a separate reply in this thread, but > answering here for
visibility: > > You can safely do it on existing CPU types, because the new
> behavior doesn't introduce host software or hardware requirements > when
enabled. You just need to disable the new behavior in >
MachineClass.compat_props for older machine types. Hi Eduardo, Thanks for
the suggestions. Since the CPUID changes no longer rely on adding new CPU
models, I've broken that out as a separate patch here based on your input:
https://lists.nongnu.org/archive/html/qemu-devel/2021-07/msg01679.html
Zhenwei, with the above patch I think you can change your patch to use: if
(!cpu->vendor_cpuid_only || IS_INTEL_CPU(env)) //add intel-specific range
Let me know if you want me to update and add to my series.
Sure, thanks a lot. > > -- > Eduardo > >

[-- Attachment #2: Type: text/html, Size: 8779 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-08 17:06 ` [PATCH] target/i386: Fix cpuid level for AMD Michael Roth
@ 2021-07-08 21:05   ` Eduardo Habkost
  0 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2021-07-08 21:05 UTC (permalink / raw)
  To: Michael Roth
  Cc: Igor Mammedov, Richard Henderson, qemu-devel, zhenwei pi,
	Dr. David Alan Gilbert

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] target/i386: Fix cpuid level for AMD
  2021-07-08  0:36 [PATCH] target/i386: suppress CPUID leaves not defined by the CPU vendor Michael Roth
@ 2021-07-08 17:06 ` Michael Roth
  2021-07-08 21:05   ` Eduardo Habkost
  0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2021-07-08 17:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: zhenwei pi, Dr. David Alan Gilbert, Eduardo Habkost,
	Richard Henderson, Igor Mammedov

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



^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-07-08 21:07 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-28 13:20 [PATCH] target/i386: Fix cpuid level for AMD zhenwei pi
2021-06-29 14:06 ` Dr. David Alan Gilbert
2021-06-29 21:29   ` Babu Moger
2021-06-30 19:18   ` Michael Roth
2021-07-01  8:43     ` Igor Mammedov
2021-07-01 20:35       ` Michael Roth
2021-07-02  5:14         ` [External] " zhenwei pi
2021-07-02 15:43           ` Michael Roth
2021-07-02 17:35             ` Eduardo Habkost
2021-07-08  5:11               ` Michael Roth
2021-07-08 13:09                 ` 皮振伟
2021-07-02  6:50         ` David Edmondson
2021-07-02 15:40           ` Michael Roth
2021-07-02 17:32     ` Eduardo Habkost
2021-07-08  0:36 [PATCH] target/i386: suppress CPUID leaves not defined by the CPU vendor Michael Roth
2021-07-08 17:06 ` [PATCH] target/i386: Fix cpuid level for AMD Michael Roth
2021-07-08 21:05   ` Eduardo Habkost

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.