Re: [PATCH v6] i386/cpu: fixup number of addressable IDs for logical processors in the physical package

[Daniel P . Berrangé]

On Tue, Dec 03, 2024 at 03:33:41PM +0800, Zhao Liu wrote:
> > However, back to the patch, I think we cannot change it as this patch
> > directly. Instead, we need a compat_props for the changed behavior, because
> > this isn't a bug fix and it introduces guest-visible differences.
> 
> This is a fix, not a new feature, so compat_props is not needed.

Note from QEMU's POV, whether or not a change requires use of compat_props
is NOT determined by whether it is a bugfix or feature.

The decision is driven by whether a running guest OS will continue to
function correctly when it is live migrated between 2 QEMUs, before/after
the commit.

If the commit breaks a running guest, then toggling usage of the changed
code based on compat_props is required. Sometimes we can get away without
this for bug fixes, other bug fixes not so lucky.

My gut feeling is in this case we're probably safe-ish without compat_props,
as topology is the kind of info that's read once at OS startup and then
cached until reboot. So changing the logical processor per package
behind a running guest (probably) won't cause trouble.

One of the i386 maintainers should sanity check though, as this code isn't
my normal area of expertize

> 
> > For ancient Intel CPUs, EBX[23:16] did represent the number of Logical
> > processor per package. I believe this should be the reason why QEMU
> > implemented it as is:
> > 
> >   - on SDM version 013, EBX[23:16]: Number of logical processors per
> > physical processor; two for the Pentium 4 processor supporting
> > Hyper-Threading Technology.
> > 
> >   - on SDM version 015, it changed to: Number of initial APIC IDs reserved
> > for this physical package. Normally, this is the number of logical
> > processors per physical package.
> > 
> >   - on SDM version 016, it changed to: Maximum number of logical processors
> > in this physical package.
> > 
> >   - finally, starting from SDM version 026, it changed to what reads now:
> > Maximum number of addressable IDs for logical processors in this physical
> > package.
> 
> And this is an architecturally defined CPUID, so SDM ensures backward
> compatibility.
> 
> Regards,
> Zhao
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|