diff mbox series

[v2] target/i386: Fix the CPUID leaf CPUID_Fn80000008

Message ID 20200417215345.64800.73351.stgit@localhost.localdomain (mailing list archive)
State New, archived
Headers show
Series [v2] target/i386: Fix the CPUID leaf CPUID_Fn80000008 | expand

Commit Message

Babu Moger April 17, 2020, 9:55 p.m. UTC
CPUID leaf CPUID_Fn80000008_ECX provides information about the
number of threads supported by the processor. It was found that
the field ApicIdSize(bits 15-12) was not set correctly.

ApicIdSize is defined as the number of bits required to represent
all the ApicId values within a package.

Valid Values: Value Description
3h-0h		Reserved.
4h		up to 16 threads.
5h		up to 32 threads.
6h		up to 64 threads.
7h		up to 128 threads.
Fh-8h		Reserved.

Fix the bit appropriately.

This came up during following thread.
https://lore.kernel.org/qemu-devel/158643709116.17430.15995069125716778943.malonedeb@wampee.canonical.com/#t

Refer the Processor Programming Reference (PPR) for AMD Family 17h
Model 01h, Revision B1 Processors. The documentation is available
from the bugzilla Link below.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537

Reported-by: Philipp Eppelt <1871842@bugs.launchpad.net>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v2: 
  Used env->pkg_offset for bits 15:12 which is already available.

 target/i386/cpu.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Philipp Eppelt April 21, 2020, 11:45 a.m. UTC | #1
Hi,

thanks for the patch, I tested it in my setup and I'm seeing numbers
that make sense.

However, I can create a setup which places the value 02h* into the
ApicIdSize field, which is reserved. However, I deem this a
configuration issue as well.

* -cpu EPYC-v2 -smp 4,cores=4 --> 0x8000_0008[ECX] = 0x2003

Cheers,
Philipp

On 4/17/20 11:55 PM, Babu Moger wrote:
> CPUID leaf CPUID_Fn80000008_ECX provides information about the
> number of threads supported by the processor. It was found that
> the field ApicIdSize(bits 15-12) was not set correctly.
> 
> ApicIdSize is defined as the number of bits required to represent
> all the ApicId values within a package.
> 
> Valid Values: Value Description
> 3h-0h		Reserved.
> 4h		up to 16 threads.
> 5h		up to 32 threads.
> 6h		up to 64 threads.
> 7h		up to 128 threads.
> Fh-8h		Reserved.
> 
> Fix the bit appropriately.
> 
> This came up during following thread.
> https://lore.kernel.org/qemu-devel/158643709116.17430.15995069125716778943.malonedeb@wampee.canonical.com/#t
> 
> Refer the Processor Programming Reference (PPR) for AMD Family 17h
> Model 01h, Revision B1 Processors. The documentation is available
> from the bugzilla Link below.
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> 
> Reported-by: Philipp Eppelt <1871842@bugs.launchpad.net>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v2: 
>   Used env->pkg_offset for bits 15:12 which is already available.
> 
>  target/i386/cpu.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 90ffc5f..5e5a605 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5830,11 +5830,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *eax = cpu->phys_bits;
>          }
>          *ebx = env->features[FEAT_8000_0008_EBX];
> -        *ecx = 0;
> -        *edx = 0;
>          if (cs->nr_cores * cs->nr_threads > 1) {
> -            *ecx |= (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
> +             * thread ID within a package". This is already stored at
> +             * CPUX86State::pkg_offset.
> +             * Bits 7:0 is "The number of threads in the package is NC+1"
> +             */
> +            *ecx = (env->pkg_offset << 12) |
> +                   ((cs->nr_cores * cs->nr_threads) - 1);
> +        } else {
> +            *ecx = 0;
>          }
> +        *edx = 0;
>          break;
>      case 0x8000000A:
>          if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
>
Paolo Bonzini May 21, 2020, 4:04 p.m. UTC | #2
On 17/04/20 23:55, Babu Moger wrote:
> CPUID leaf CPUID_Fn80000008_ECX provides information about the
> number of threads supported by the processor. It was found that
> the field ApicIdSize(bits 15-12) was not set correctly.
> 
> ApicIdSize is defined as the number of bits required to represent
> all the ApicId values within a package.
> 
> Valid Values: Value Description
> 3h-0h		Reserved.
> 4h		up to 16 threads.
> 5h		up to 32 threads.
> 6h		up to 64 threads.
> 7h		up to 128 threads.
> Fh-8h		Reserved.
> 
> Fix the bit appropriately.
> 
> This came up during following thread.
> https://lore.kernel.org/qemu-devel/158643709116.17430.15995069125716778943.malonedeb@wampee.canonical.com/#t
> 
> Refer the Processor Programming Reference (PPR) for AMD Family 17h
> Model 01h, Revision B1 Processors. The documentation is available
> from the bugzilla Link below.
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> 
> Reported-by: Philipp Eppelt <1871842@bugs.launchpad.net>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v2: 
>   Used env->pkg_offset for bits 15:12 which is already available.
> 
>  target/i386/cpu.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 90ffc5f..5e5a605 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5830,11 +5830,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *eax = cpu->phys_bits;
>          }
>          *ebx = env->features[FEAT_8000_0008_EBX];
> -        *ecx = 0;
> -        *edx = 0;
>          if (cs->nr_cores * cs->nr_threads > 1) {
> -            *ecx |= (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
> +             * thread ID within a package". This is already stored at
> +             * CPUX86State::pkg_offset.
> +             * Bits 7:0 is "The number of threads in the package is NC+1"
> +             */
> +            *ecx = (env->pkg_offset << 12) |
> +                   ((cs->nr_cores * cs->nr_threads) - 1);
> +        } else {
> +            *ecx = 0;
>          }
> +        *edx = 0;
>          break;
>      case 0x8000000A:
>          if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
> 

Queued, thanks.

Paolo
diff mbox series

Patch

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 90ffc5f..5e5a605 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5830,11 +5830,20 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *eax = cpu->phys_bits;
         }
         *ebx = env->features[FEAT_8000_0008_EBX];
-        *ecx = 0;
-        *edx = 0;
         if (cs->nr_cores * cs->nr_threads > 1) {
-            *ecx |= (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
+             * thread ID within a package". This is already stored at
+             * CPUX86State::pkg_offset.
+             * Bits 7:0 is "The number of threads in the package is NC+1"
+             */
+            *ecx = (env->pkg_offset << 12) |
+                   ((cs->nr_cores * cs->nr_threads) - 1);
+        } else {
+            *ecx = 0;
         }
+        *edx = 0;
         break;
     case 0x8000000A:
         if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {