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

[Babu Moger]

On 3/8/20 8:25 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 03, 2020 at 01:56:51PM -0600, Babu Moger wrote:
>> This series fixes APIC ID encoding problem reported on AMD EPYC cpu models.
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1728166&data=02%7C01%7Cbabu.moger%40amd.com%7C9176c1cef91b4ece2b5e08d7c3643a0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637192707563033288&sdata=r%2Frpma42Ms356HFtNthqJNI3CiQBEfI%2BNWsWCr%2BabV0%3D&reserved=0
>>
>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.moger%40amd.com%7C9176c1cef91b4ece2b5e08d7c3643a0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637192707563033288&sdata=PWj8%2FEcBYPmcJVg4qgbf%2BPZ1VbLzqXgAiQ7ujtfFp90%3D&reserved=0
>>
>> 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]}
> 
> 
> Looks reasonable:
> 
> Acked-by: Michael S. Tsirkin <address@hidden>

Michael.Thanks
> 
> belongs in Eduardo's tree I guess.
> 
>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F158161767653.48948.10578064482878399556.stgit%40naples-babu.amd.com%2F&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C9176c1cef91b4ece2b5e08d7c3643a0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637192707563033288&amp;sdata=bKJ2d%2Blhxm%2Bht7hZBsQpHtFOaee3FHwA0ploz%2BNpPuw%3D&amp;reserved=0
>>  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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F157541968844.46157.17994918142533791313.stgit%40naples-babu.amd.com%2F&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C9176c1cef91b4ece2b5e08d7c3643a0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637192707563043283&amp;sdata=90dNqhJMpQiyv4KdBfuf9hxX8EHFesJUT5jxSnuU84E%3D&amp;reserved=0
>>  
>>   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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F156779689013.21957.1631551572950676212.stgit%40localhost.localdomain%2F&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C9176c1cef91b4ece2b5e08d7c3643a0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637192707563043283&amp;sdata=tKtt4Kz599IkFhyObyg9%2FVC3BEKPjo4KAGi%2BsdVe9zM%3D&amp;reserved=0
>>   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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F20190731232032.51786-1-babu.moger%40amd.com%2F&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C9176c1cef91b4ece2b5e08d7c3643a0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637192707563043283&amp;sdata=biydl3JXU0YGLK%2FY7sXtSbdCcb7P4Qjl40Kr0Fp5CEc%3D&amp;reserved=0
>>
>> ---
>>
>> 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
>