Re: [PATCH] target/i386: Fix the CPUID leaf CPUID_Fn80000008

[Eduardo Habkost <ehabkost@redhat.com>]

Good catch, thanks for the patch.  Comments below:

On Fri, Apr 17, 2020 at 10:14:32AM -0500, 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>
> ---
>  target/i386/cpu.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 90ffc5f..68210f6 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5830,11 +5830,17 @@ 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;

I'm not sure we want a compatibility flag to keep ABI on older
machine types, here.  Strictly speaking, CPUID must never change
on older machine types, but sometimes trying hard to emulate bugs
of old QEMU versions is a pointless exercise.


> +            unsigned int max_apicids, bits_required;
> +
> +            max_apicids = (cs->nr_cores * cs->nr_threads) - 1;
> +            /* Find out the number of bits to represent all the apicids */
> +            bits_required = 32 - clz32(max_apicids);

This won't work if nr_cores > 1 and nr_threads is not a power of
2, will it?

For reference, the field is documented[1] as:

"The number of bits in the initial Core::X86::Apic::ApicId[ApicId]
value that indicate thread ID within a package"

This sounds like the value already stored at
CPUX86State::pkg_offset.


> +            *ecx = bits_required << 12 | max_apicids;

Bits 7:0 are documented as "The number of threads in the package
is NC+1", with no reference to APIC IDs at all.

Using ((nr_cores * nr_threads) - 1) for bits 7:0 sounds correct,
but the variable name seems misleading.


> +        } else {
> +            *ecx = 0;
>          }
> +        *edx = 0;
>          break;
>      case 0x8000000A:
>          if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
> 
> 

References:

[1] Processor Programming Reference (PPR) for
    AMD Family 17h Model 18h, Revision B1 Processors
    55570-B1 Rev 3.14 - Sep 26, 2019
    https://bugzilla.kernel.org/attachment.cgi?id=287395&action=edit


-- 
Eduardo