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

[皮振伟 <pizhenwei@bytedance.com>]

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