Re: [PATCH] x86: cpu: fixup number of addressable IDs for processor cores in the physical package

[Zhao Liu <zhao1.liu@intel.com>]

Hi Igor,

On Mon, May 27, 2024 at 05:03:17PM +0200, Igor Mammedov wrote:
> Date: Mon, 27 May 2024 17:03:17 +0200
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [PATCH] x86: cpu: fixup number of addressable IDs for
>  processor cores in the physical package
> X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-redhat-linux-gnu)
> 
> On Mon, 27 May 2024 11:13:33 +0800
> Chuang Xu <xuchuangxclwt@bytedance.com> wrote:
> 
> > When QEMU is started with:
> > -cpu host,host-cache-info=on,l3-cache=off \
> > -smp 2,sockets=1,dies=1,cores=1,threads=2
> > Guest can't acquire maximum number of addressable IDs for processor cores in
> > the physical package from CPUID[04H].
> 
> please add commit message, what you are actually seeing
> and expected values as well.
> And if guest complains about it also include related dmesg output.
> 
> 
> > This bug was introduced in commit d7caf13b5fcf742e5680c1d3448ba070fc811644.
> > Fix it by changing the judgement condition to a >= 1.
> > 
> > Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> > Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
> > Signed-off-by: Yipeng Yin <yinyipeng@bytedance.com>
> > ---
> >  target/i386/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index cd16cb893d..0369c01153 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -6097,7 +6097,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >              if (*eax & 31) {
> >                  int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
> >                  int vcpus_per_socket = cs->nr_cores * cs->nr_threads;
> 
> in light of dies and recent modules shouldn't we also account for them here?

cs->nr_cores now account dies and modules (as its comment said "Number
of cores within this CPU package"). The code for this patch is a bit
outdated, although the issue is still here on the latest master.

> > -                if (cs->nr_cores > 1) {
> > +                if (cs->nr_cores >= 1) {
> >                      *eax &= ~0xFC000000;
> >                      *eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
> >                  }
> above and also following condition
> 
>                 if (host_vcpus_per_cache > vcpus_per_socket) {
>                     ...
>                     *eax |= (pow2ceil(vcpus_per_socket) - 1) << 14;
> 
> Makes me think, do we really have to have both conditionals,
> Why not just drop conditions and always encode both values
> to ones configured on '-smp' CLI?

The first condition "cores_per_pkg >= 1" can be removed in this patch
since cs->nr_cores won't be 0. :-)

About the 2nd condition "host_vcpus_per_cache > vcpus_per_socket", more
work is needed for cleanup...removal is also desirable, but not direct
removal, otherwise, Guest's L1/L2/L3 cache topology will default to
package level.

Currently with host_vcpus_per_cache <= threads_per_pkg, QEMU will
directly give Guest the Host's EAX[bits 25 -14], which is also
inaccurate, especially if there is a big difference between Guest and
Host CPU topology.

The correct way to do it is to parse the specific level of cache
topology on Host (core/module/die/package), and then encode Guest's
EAX[bits 25 -14] according to the specific items configured in -smp.

By cleaning up in this way, the second condition can be removed
naturally.

The host-cache-info cleanup was originally planned for me as well, I
think I can do this after Chuang's fix.

Thanks,
Zhao