[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/i386: Fix reporting of CPU dies when nr_cores=nr_thre
From: |
Wen, Qian |
Subject: |
Re: [PATCH] target/i386: Fix reporting of CPU dies when nr_cores=nr_threads=1 |
Date: |
Wed, 16 Aug 2023 16:37:51 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.14.0 |
On 7/24/2023 2:59 AM, 小太 wrote:
> When QEMU is started with `-smp D,sockets=1,dies=D,cores=1,threads=1` (that
> is, 1 socket with D dies but each die contains just a single thread), both
> Linux and Windows guests incorrectly interprets the system as having D
> sockets with 1 die each
>
> Ultimately this is caused by various CPUID leaves not being die-aware in
> their "threads per socket" calculations, so this patch fixes that
>
> These changes are referenced to the AMD PPR for Family 19h Model 01h (Milan)
> and Family 17h Model 01h (Naples) manuals:
> - CPUID_Fn00000001_EBX[23:16]: Number of threads in the processor
> (Core::X86::Cpuid::SizeId[NC] + 1)
> - CPUID_Fn0000000B_EBX_x01[15:0]: Number of logical cores in processor
> socket (not present until Rome)
> - CPUID_Fn80000001_ECX[1]: Multi core product
> (Core::X86::Cpuid::SizeId[NC] != 0)
> - CPUID_Fn80000008_ECX[7:0]: The number of threads in the package - 1
> (Core::X86::Cpuid::SizeId[NC])
>
> Note there are two remaining occurences that I didn't touch:
> - CPUID_Fn8000001E_ECX[10:8]: Always 0 (1 node per processor) for Milan.
> But for Naples, it can also be 2 or 4 nodes
> where each node is defined as one or two
> CCXes (CCD?). But Milan also has multiple
> CCXes, so clearly the definition of a node is
> different from model to model, so I've left
> it untouched. (QEMU seems to use the Naples
> definition)
> - MSR_CORE_THREAD_COUNT: This MSR doesn't exist on Milan or Naples
>
> Signed-off-by: 小太 <nospam@kota.moe>
> ---
> target/i386/cpu.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 97ad229d8b..6ff23fa590 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6049,8 +6049,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> *ecx |= CPUID_EXT_OSXSAVE;
> }
> *edx = env->features[FEAT_1_EDX];
> - if (cs->nr_cores * cs->nr_threads > 1) {
> - *ebx |= (cs->nr_cores * cs->nr_threads) << 16;
> + if (env->nr_dies * cs->nr_cores * cs->nr_threads > 1) {
> + *ebx |= (env->nr_dies * cs->nr_cores * cs->nr_threads) << 16;
The nr_cores in CPUState means "Number of cores within this CPU package", so it
doesn't need
"nr_dies" here. Zhao Liu is fixing the calculation of nr_cores [1].
[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg07187.html
Thanks,
Qian
> *edx |= CPUID_HT;
> }
> if (!cpu->enable_pmu) {
> @@ -6230,7 +6230,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> break;
> case 1:
> *eax = apicid_pkg_offset(&topo_info);
> - *ebx = cs->nr_cores * cs->nr_threads;
> + *ebx = env->nr_dies * cs->nr_cores * cs->nr_threads;
> *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
> break;
> default:
> @@ -6496,7 +6496,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> * discards multiple thread information if it is set.
> * So don't set it here for Intel to make Linux guests happy.
> */
> - if (cs->nr_cores * cs->nr_threads > 1) {
> + if (env->nr_dies * cs->nr_cores * cs->nr_threads > 1) {
> if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
> env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
> env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
> @@ -6562,7 +6562,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> *eax |= (cpu_x86_virtual_addr_width(env) << 8);
> }
> *ebx = env->features[FEAT_8000_0008_EBX];
> - if (cs->nr_cores * cs->nr_threads > 1) {
> + if (env->nr_dies * cs->nr_cores * cs->nr_threads > 1) {
> /*
> * Bits 15:12 is "The number of bits in the initial
> * Core::X86::Apic::ApicId[ApicId] value that indicate
> @@ -6570,7 +6570,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> * Bits 7:0 is "The number of threads in the package is NC+1"
> */
> *ecx = (apicid_pkg_offset(&topo_info) << 12) |
> - ((cs->nr_cores * cs->nr_threads) - 1);
> + ((env->nr_dies * cs->nr_cores * cs->nr_threads) - 1);
> } else {
> *ecx = 0;
> }