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

[Eduardo Habkost]

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