diff mbox series

x86: cpu: fixup number of addressable IDs for processor cores in the physical package

Message ID 20240527031333.85932-1-xuchuangxclwt@bytedance.com (mailing list archive)
State New, archived
Headers show
Series x86: cpu: fixup number of addressable IDs for processor cores in the physical package | expand

Commit Message

Chuang Xu May 27, 2024, 3:13 a.m. UTC 
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].

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(-)

Comments

Igor Mammedov May 27, 2024, 3:03 p.m. UTC | #1 
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?


> -                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?
Zhao Liu May 27, 2024, 4:56 p.m. UTC | #2 
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
Zhao Liu May 28, 2024, 2:31 a.m. UTC | #3 
Hi Chuang,

On Mon, May 27, 2024 at 11:13:33AM +0800, Chuang Xu wrote:
> Date: Mon, 27 May 2024 11:13:33 +0800
> From: Chuang Xu <xuchuangxclwt@bytedance.com>
> Subject: [PATCH] x86: cpu: fixup number of addressable IDs for processor
>  cores in the physical package

According to the usual practice of QEMU commits, people tend to use
"i386/cpu" as the subject prefix, which indicates the code path.

> 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 \

Just a discussion, "l3-cache=off" doesn't work in host cache pssthu
case, do you have a specific need that you don't want to see l3 cache?

> -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].
> 
> This bug was introduced in commit d7caf13b5fcf742e5680c1d3448ba070fc811644.
> Fix it by changing the judgement condition to a >= 1.

Pls add a "Fixes" tag like:

Fixes: d7caf13b5fcf ("x86: cpu: fixup number of addressable IDs for logical processors sharing cache")

Since this is a historical issue that deserves to be ported to the
stable branch, you can cc stable list by:

Cc: qemu-stable@nongnu.org

> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>

As the patch sender, it's better to put your signature on the last line.
;-)

> 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;
> -                if (cs->nr_cores > 1) {
> +                if (cs->nr_cores >= 1) {

Like Igor suggested, this condition could be removed since cs->nr_cores can't
be 0.

>                      *eax &= ~0xFC000000;
>                      *eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
>                  }

...the code is outdated, pls rebase on the latest master branch.

Regards,
Zhao
Chuang Xu May 28, 2024, 3:34 a.m. UTC | #4 
Hi Zhao,

On 2024/5/28 上午10:31, Zhao Liu wrote:
> Hi Chuang,
>
> On Mon, May 27, 2024 at 11:13:33AM +0800, Chuang Xu wrote:
>> Date: Mon, 27 May 2024 11:13:33 +0800
>> From: Chuang Xu <xuchuangxclwt@bytedance.com>
>> Subject: [PATCH] x86: cpu: fixup number of addressable IDs for processor
>>   cores in the physical package
> According to the usual practice of QEMU commits, people tend to use
> "i386/cpu" as the subject prefix, which indicates the code path.
>
>> 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 \
> Just a discussion, "l3-cache=off" doesn't work in host cache pssthu
> case, do you have a specific need that you don't want to see l3 cache?
>
No specific need, just generated by libvirt.
>> -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].
>>
>> This bug was introduced in commit d7caf13b5fcf742e5680c1d3448ba070fc811644.
>> Fix it by changing the judgement condition to a >= 1.
> Pls add a "Fixes" tag like:
>
> Fixes: d7caf13b5fcf ("x86: cpu: fixup number of addressable IDs for logical processors sharing cache")
>
> Since this is a historical issue that deserves to be ported to the
> stable branch, you can cc stable list by:
>
> Cc: qemu-stable@nongnu.org
>
>> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> As the patch sender, it's better to put your signature on the last line.
> ;-)
>
>> 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;
>> -                if (cs->nr_cores > 1) {
>> +                if (cs->nr_cores >= 1) {
> Like Igor suggested, this condition could be removed since cs->nr_cores can't
> be 0.
>
>>                       *eax &= ~0xFC000000;
>>                       *eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
>>                   }
> ...the code is outdated, pls rebase on the latest master branch.
My fault, sorry for forgetting to pull the latest code..
> Regards,
> Zhao
>
Thanks for all your suggestions!

Chuang
diff mbox series

Patch

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;
-                if (cs->nr_cores > 1) {
+                if (cs->nr_cores >= 1) {
                     *eax &= ~0xFC000000;
                     *eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
                 }