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 (pizhen...@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 <pizhen...@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 / dgilb...@redhat.com / Manchester, UK > > > > > -- Eduardo
