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 <imamm...@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 <xuchuangxc...@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 <xuchuangxc...@bytedance.com>
> > Signed-off-by: Guixiong Wei <weiguixi...@bytedance.com>
> > Signed-off-by: Yipeng Yin <yinyip...@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
