Re: [PATCH v5 2/9] hw/core: Make CPU topology enumeration arch-agnostic

[EwanHai <ewandevelop@gmail.com>]

Apologies for the noise, please ignore my previous reply, the corporate mailer
collapsed all the newlines. Resending in plain text below.

Zhao Liu wrote on 11/1/24 4:33 PM:
> Cache topology needs to be defined based on CPU topology levels. Thus,
> define CPU topology enumeration in qapi/machine.json to make it generic
> for all architectures.
> 
> To match the general topology naming style, rename CPU_TOPO_LEVEL_* to
> CPU_TOPOLOGY_LEVEL_*, and rename SMT and package levels to thread and
> socket.
> 
> Also, enumerate additional topology levels for non-i386 arches, and add
> a CPU_TOPOLOGY_LEVEL_DEFAULT to help future smp-cache object to work
> with compatibility requirement of arch-specific cache topology models.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Acked-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

...


> @@ -373,17 +373,17 @@ static void encode_topo_cpuid1f(CPUX86State *env, uint32_t count,
>      unsigned long level, base_level, next_level;
>      uint32_t num_threads_next_level, offset_next_level;
>  
> -    assert(count <= CPU_TOPO_LEVEL_PACKAGE);
> +    assert(count <= CPU_TOPOLOGY_LEVEL_SOCKET);
>  

Hi Zhao,

While booting Linux under TCG QEMU (-kernel + rootfs) with a CPU model that
enables cpuid_0x1f (e.g. Shijidadao-v2), I hit this assert: when the guest
queries CPUID.1F with a topology level greater than CPU_TOPOLOGY_LEVEL_SOCKET,
QEMU aborts. Under KVM the path isn't reached (CPUID is served from
KVM_SET_CPUID2), so this is TCG-only, which is probably why it hadn't been
caught earlier.

I'm not sure the assert is necessary: the code that follows already handles
the "input level out of range" case correctly. find_next_bit() returns SOCKET
once the bitmap is exhausted, the loop breaks with level = INVALID, and the
encoding path below produces the SDM-defined invalid-domain response (EAX=0,
EBX=0, ECX = count | INVALID_type, EDX = apic_id).

Per Intel SDM, CPUID Leaf 1FH does not constrain the input ECX value, so guest
software is free to query any sub-leaf index. Did the assert intentionally
protect an invariant I'm missing, or is it a leftover defensive check?

For reference, the KVM kernel side (arch/x86/kvm/cpuid.c kvm_cpuid()) has
explicitly handled out-of-range 0x1F subleaves this way since 2019:
    } else {
        *eax = *ebx = *ecx = *edx = 0;
        /*
         * When leaf 0BH or 1FH is defined, CL is pass-through
         * and EDX is always the x2APIC ID, even for undefined
         * subleaves. ...
         */
        if (function == 0xb || function == 0x1f) {
            entry = kvm_find_cpuid_entry_index(vcpu, function, 1);
            if (entry) {
                *ecx = index & 0xff;
                *edx = entry->edx;
            }
        }
    }
introduced in commit 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves
0BH and 1FH"). So KVM-accelerated guests already see the SDM-compliant response;
only TCG aborts.

Thanks,
Ewan


>      /*
>       * Find the No.(count + 1) topology level in avail_cpu_topo bitmap.
> -     * The search starts from bit 0 (CPU_TOPO_LEVEL_SMT).
> +     * The search starts from bit 0 (CPU_TOPOLOGY_LEVEL_THREAD).
>       */
> -    level = CPU_TOPO_LEVEL_SMT;
> +    level = CPU_TOPOLOGY_LEVEL_THREAD;
>      base_level = level;
>      for (int i = 0; i <= count; i++) {
>          level = find_next_bit(env->avail_cpu_topo,
> -                              CPU_TOPO_LEVEL_PACKAGE,
> +                              CPU_TOPOLOGY_LEVEL_SOCKET,
>                                base_level);
>  
>          /*
> @@ -391,20 +391,20 @@ static void encode_topo_cpuid1f(CPUX86State *env, uint32_t count,
>           * and it just encodes the invalid level (all fields are 0)
>           * into the last subleaf of 0x1f.
>           */
> -        if (level == CPU_TOPO_LEVEL_PACKAGE) {
> -            level = CPU_TOPO_LEVEL_INVALID;
> +        if (level == CPU_TOPOLOGY_LEVEL_SOCKET) {
> +            level = CPU_TOPOLOGY_LEVEL_INVALID;
>              break;
>          }
>          /* Search the next level. */
>          base_level = level + 1;
>      }
>  
> -    if (level == CPU_TOPO_LEVEL_INVALID) {
> +    if (level == CPU_TOPOLOGY_LEVEL_INVALID) {
>          num_threads_next_level = 0;
>          offset_next_level = 0;
>      } else {
>          next_level = find_next_bit(env->avail_cpu_topo,
> -                                   CPU_TOPO_LEVEL_PACKAGE,
> +                                   CPU_TOPOLOGY_LEVEL_SOCKET,
>                                     level + 1);
>          num_threads_next_level = num_threads_by_topo_level(topo_info,
>                                                             next_level);
...