target/i386/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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(-)
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;
}
--
2.20.1Hi 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
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
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?
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
© 2016 - 2024 Red Hat, Inc.