From: Michael Roth <michael.roth@amd.com>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: like.xu@linux.intel.com, armbru@redhat.com, wei.huang2@amd.com,
richard.henderson@linaro.org, qemu-devel@nongnu.org,
zhenwei pi <pizhenwei@bytedance.com>,
Dr. David Alan Gilbert <dgilbert@redhat.com>,
babu.moger@amd.com, pbonzini@redhat.com,
Igor Mammedov <imammedo@redhat.com>,
philmd@redhat.com
Subject: Re: [External] Re: [PATCH] target/i386: Fix cpuid level for AMD
Date: Thu, 08 Jul 2021 00:11:56 -0500 [thread overview]
Message-ID: <162572111635.19908.4278294895739120232@amd.com> (raw)
In-Reply-To: <20210702173534.qy7yd4uievvhwave@habkost.net>
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
>
>
next prev parent reply other threads:[~2021-07-08 5:13 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 ` [External] " zhenwei pi
2021-07-02 15:43 ` Michael Roth
2021-07-02 17:35 ` Eduardo Habkost
2021-07-08 5:11 ` Michael Roth [this message]
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=162572111635.19908.4278294895739120232@amd.com \
--to=michael.roth@amd.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=pbonzini@redhat.com \
--cc=philmd@redhat.com \
--cc=pizhenwei@bytedance.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).