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

[Michael Roth]

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