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

[Xiaoyao Li]

On 6/4/2024 5:43 PM, Zhao Liu wrote:
> Hi Chuang,
>
> On Mon, Jun 03, 2024 at 04:36:41PM +0800, Chuang Xu wrote:
> > Date: Mon,  3 Jun 2024 16:36:41 +0800
> > From: Chuang Xu <xuchuangxclwt@bytedance.com>
> > Subject: [PATCH v2] i386/cpu: fixup number of addressable IDs for processor
> >   cores in the physical package
> > X-Mailer: git-send-email 2.24.3 (Apple Git-128)
> >
> > 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].
> >
> > When testing Intel TDX, guest attempts to acquire extended topology from 
> > CPUID[0bH],
> > but because the TDX module doesn't provide the emulation of CPUID[0bH],
> > guest will instead acquire extended topology from CPUID[04H]. However,
> > due to QEMU's inaccurate emulation of CPUID[04H], one of the vcpus in 2c TDX
> > guest would be offline.
>
> I guess this case is based on downstream's TDX patches... Since TDX
> hasn't landed in QEMU yet, it's a bit ahead of the curve to elaborate on
> TDX-specific case.
>
> Because normal VM will also face the such cache topology error, I think
> it could be stated a bit more generically like:

yes. it's not TDX specific though it's found by TDX case.

I think We can reproduce it by limiting the "min-level" to less than 0xb 
and disable "full-cpuid-auto-level". With it, CPUID leaves 0xb are not 
exposed to guest and guest will use leaves 0x4 to enumerate the CPU 
topology.

> When creating a CPU topology of 1 core per package, host-cache-info only
> uses the Host's addressable core IDs field (CPUID.04H.EAX[bits 31-26]),
> resulting in a conflict (on the multicore Host) between the Guest core
> topology information in this field and the Guest's actual cores number.
>
> Fix it by removing the unnecessary condition to cover 1 core per package
> case. This is safe because cores_per_pkg will not be 0 and will be at
> least 1.
>
> > Fix it by removing the unnecessary condition.
> >
> > Fixes: d7caf13b5fcf742e5680c1d3448ba070fc811644 ("x86: cpu: fixup number of 
> > addressable IDs for logical processors sharing cache")

yeah. This is the exact commit that introduced the issue. Because it moved

        *eax &= ~0xFC000000;

into the condition of

        cs->nr_cores > 1

> 12 characters (d7caf13b5fcf) is enough. No blank line. ;-)
>
> > Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
> > Signed-off-by: Yipeng Yin <yinyipeng@bytedance.com>
> > Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> > ---
> >   target/i386/cpu.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index bc2dceb647..b68f7460db 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -6426,10 +6426,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> > uint32_t count,
> >               if (*eax & 31) {
> >                   int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
> >   
> > -                if (cores_per_pkg > 1) {
> > -                    *eax &= ~0xFC000000;
> > -                    *eax |= max_core_ids_in_package(&topo_info) << 26;
> > -                }
> > +                *eax &= ~0xFC000000;
> > +                *eax |= max_core_ids_in_package(&topo_info) << 26;
> >                   if (host_vcpus_per_cache > threads_per_pkg) {
> >                       *eax &= ~0x3FFC000;
> >   
> > --
> > 2.20.1