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

[David Edmondson]

On Thursday, 2021-07-01 at 15:35:49 -05, 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...

The XSAVE changes are similar to
https://lore.kernel.org/r/20210520145647.3483809-1-david.edmondson@oracle.com,
in response to which Paolo suggested that I should have QEMU observe the
CPUID 0xd leaves rather than encoding knowledge about the state offsets
(at least, that's how I understood his comment).

I have patches that do this (which includes making X86XSaveArea a
TCG-only thing) that I plan to send out in the next couple of days. They
should make "amd-xsave" unnecessary.

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

dme.
-- 
I had my eyes closed in the dark.