From: zhenwei pi <pizhenwei@bytedance.com>
To: Michael Roth <michael.roth@amd.com>, Igor Mammedov <imammedo@redhat.com>
Cc: ehabkost@redhat.com, like.xu@linux.intel.com,
qemu-devel@nongnu.org, wei.huang2@amd.com,
richard.henderson@linaro.org,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
armbru@redhat.com, babu.moger@amd.com, pbonzini@redhat.com,
philmd@redhat.com
Subject: Re: [External] Re: [PATCH] target/i386: Fix cpuid level for AMD
Date: Fri, 2 Jul 2021 13:14:56 +0800 [thread overview]
Message-ID: <2952f218-f391-36d5-6331-006d9312cc66@bytedance.com> (raw)
In-Reply-To: <162517174973.564224.1039189315728194554@amd.com>
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
next prev parent reply other threads:[~2021-07-02 5:16 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` zhenwei pi [this message]
2021-07-02 15:43 ` [External] " 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2952f218-f391-36d5-6331-006d9312cc66@bytedance.com \
--to=pizhenwei@bytedance.com \
--cc=armbru@redhat.com \
--cc=babu.moger@amd.com \
--cc=dgilbert@redhat.com \
--cc=ehabkost@redhat.com \
--cc=imammedo@redhat.com \
--cc=like.xu@linux.intel.com \
--cc=michael.roth@amd.com \
--cc=pbonzini@redhat.com \
--cc=philmd@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=wei.huang2@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).