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

[Michael Roth]

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

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