Re: [PATCH v5 00/16] APIC ID fixes for AMD EPYC CPU model

[Igor Mammedov]

On Tue, 03 Mar 2020 13:56:51 -0600
Babu Moger <address@hidden> wrote:

> This series fixes APIC ID encoding problem reported on AMD EPYC cpu models.
> https://bugzilla.redhat.com/show_bug.cgi?id=1728166
> 
> Currently, the APIC ID is decoded based on the sequence
> sockets->dies->cores->threads. This works for most standard AMD and other
> vendors' configurations, but this decoding sequence does not follow that of
> AMD's APIC ID enumeration strictly. In some cases this can cause CPU topology
> inconsistency.  When booting a guest VM, the kernel tries to validate the
> topology, and finds it inconsistent with the enumeration of EPYC cpu models.
> 
> To fix the problem we need to build the topology as per 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
> 
> Here is the text from the PPR.
> Operating systems are expected to use Core::X86::Cpuid::SizeId[ApicIdSize], 
> the
> number of least significant bits in the Initial APIC ID that indicate core ID
> within a processor, in constructing per-core CPUID masks.
> Core::X86::Cpuid::SizeId[ApicIdSize] determines the maximum number of cores
> (MNC) that the processor could theoretically support, not the actual number of
> cores that are actually implemented or enabled on the processor, as indicated
> by Core::X86::Cpuid::SizeId[NC].
> Each Core::X86::Apic::ApicId[ApicId] register is preset as follows:
> • ApicId[6] = Socket ID.
> • ApicId[5:4] = Node ID.
> • ApicId[3] = Logical CCX L3 complex ID
> • ApicId[2:0]= (SMT) ? {LogicalCoreID[1:0],ThreadId} : 
> {1'b0,LogicalCoreID[1:0]}
> 
> v5:
>  Generated the patches on top of git://github.com/ehabkost/qemu.git 
> (x86-next).
>  Changes from v4.
>  1. Re-arranged the patches 2 and 4 as suggested by Igor.
>  2. Kept the apicid handler functions inside X86MachineState as discussed.
>     These handlers are loaded from X86CPUDefinitions.
>  3. Removed unnecessary X86CPUstate initialization from x86_cpu_new. Suggested
>     by Igor.
>  4. And other minor changes related to patch format.
> 
> v4:
>  https://lore.kernel.org/qemu-devel/address@hidden/
>  Changes from v3.
>  1. Moved the arch_id calculation inside the function x86_cpus_init. With 
> this change,
>     we dont need to change common numa code.(suggested by Igor)
>  2. Introduced the model specific handlers inside X86CPUDefinitions.
>     These handlers are loaded into X86MachineState during the init.
>  3. Removed llc_id from x86CPU.
>  4. Removed init_apicid_fn hanlder from MachineClass. Kept all the code 
> changes
>     inside the x86.
>  5. Added new handler function apicid_pkg_offset for pkg_offset calculation.
>  6. And some Other minor changes.
> 
> v3:
>   https://lore.kernel.org/qemu-devel/address@hidden/ 
>   1. Consolidated the topology information in structure X86CPUTopoInfo.
>   2. Changed the ccx_id to llc_id as commented by upstream.
>   3. Generalized the apic id decoding. It is mostly similar to current apic id
>      except that it adds new field llc_id when numa configured. Removes all 
> the
>      hardcoded values.
>   4. Removed the earlier parse_numa split. And moved the numa node 
> initialization
>      inside the numa_complete_configuration. This is bit cleaner as commented 
> by 
>      Eduardo.
>   5. Added new function init_apicid_fn inside machine_class structure. This
>      will be used to update the apic id handler specific to cpu model.
>   6. Updated the cpuid unit tests.
>   7. TODO : Need to figure out how to dynamically update the handlers using 
> cpu models.
>      I might some guidance on that.
> 
> v2:
>   
> https://lore.kernel.org/qemu-devel/156779689013.21957.1631551572950676212.stgit@localhost.localdomain/
>   1. Introduced the new property epyc to enable new epyc mode.
>   2. Separated the epyc mode and non epyc mode function.
>   3. Introduced function pointers in PCMachineState to handle the
>      differences.
>   4. Mildly tested different combinations to make things are working as 
> expected.
>   5. TODO : Setting the epyc feature bit needs to be worked out. This feature 
> is
>      supported only on AMD EPYC models. I may need some guidance on that.
> 
> v1:
>   https://lore.kernel.org/qemu-devel/address@hidden/

There will be soft freeze soon,
if you respin it soon, I'll review it ASAP.
I hope there won't be anything to amend, so
we could try merging it this week (otherwise
it will be postponed till next release).

I guess it should go via Eduardo's tree.

> 
> ---
> 
> Babu Moger (16):
>       hw/i386: Rename X86CPUTopoInfo structure to X86CPUTopoIDs
>       hw/i386: Introduce X86CPUTopoInfo to contain topology info
>       hw/i386: Consolidate topology functions
>       machine: Add SMP Sockets in CpuTopology
>       hw/i386: Remove unnecessary initialization in x86_cpu_new
>       hw/i386: Update structures to save the number of nodes per package
>       hw/i386: Rename apicid_from_topo_ids to x86_apicid_from_topo_ids
>       hw/386: Add EPYC mode topology decoding functions
>       target/i386: Cleanup and use the EPYC mode topology functions
>       hw/i386: Introduce apicid functions inside X86MachineState
>       target/i386: Load apicid model specific handlers from X86CPUDefinition
>       hw/i386: Use the apicid handlers from X86MachineState
>       target/i386: Add EPYC model specific handlers
>       hw/i386: Move arch_id decode inside x86_cpus_init
>       i386: Fix pkg_id offset for EPYC cpu models
>       tests: Update the Unit tests
> 
> 
>  hw/core/machine.c          |    1 
>  hw/i386/pc.c               |   54 ++++++-----
>  hw/i386/x86.c              |   72 ++++++++++----
>  include/hw/boards.h        |    2 
>  include/hw/i386/topology.h |  215 ++++++++++++++++++++++++++++++------------
>  include/hw/i386/x86.h      |   12 ++
>  target/i386/cpu.c          |  224 
> +++++++++++++++++---------------------------
>  target/i386/cpu.h          |    3 +
>  tests/test-x86-cpuid.c     |  115 +++++++++++++----------
>  vl.c                       |    1 
>  10 files changed, 403 insertions(+), 296 deletions(-)
> 
> --
> Signature
>