[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/i386: Fix cpuid level for AMD
From: |
Eduardo Habkost |
Subject: |
Re: [PATCH] target/i386: Fix cpuid level for AMD |
Date: |
Fri, 2 Jul 2021 13:32:23 -0400 |
On Wed, Jun 30, 2021 at 02:18:09PM -0500, Michael Roth 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?
If the change doesn't affect runnability of the CPU in a given
host (i.e. it doesn't introduce or remove host software or
hardware dependencies), it can be enabled for all CPU types in
newer machine types.
>
> 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
KVM has a hack that changes the CPUID vendor info depending on
the host (ignoring X86CPUDefinition.vendor completely). For that
reason, I would make the new behavior tied to the actual CPU
vendor seen by the guest, not to the CPU type. It will be a bit
more complicated, but less likely to cause problems when
management software tries to auto-detect the CPU model and
guesses a model from the wrong vendor.
We still need to keep compatibility somehow, though:
>
> 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).
I agree, especially if the planned CPUID changes are more
intrusive than just CPUID level adjustments.
I suggest adding a "vendor-cpuid-only" property, that would
hide CPUID leaves depending on the actual CPUID vendor seen by
the guest. Older machine types can set vendor-cpuid-only=off,
and newer machine-types would have vendor-cpuid-only=on by
default.
>
> 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
> >
> >
>
--
Eduardo
Re: [PATCH] target/i386: Fix cpuid level for AMD, David Edmondson, 2021/07/02
- Re: [PATCH] target/i386: Fix cpuid level for AMD,
Eduardo Habkost <=