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

[Babu Moger]

On 3/10/20 3:40 AM, Igor Mammedov wrote:
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1728166&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C45a40d6e3f3e43db7f1c08d7c4ceb6ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637194264422137307&amp;sdata=aK22ORKQVO91LQg8S5HLIkwokhSr1gZtbiPBT6f1xfY%3D&amp;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&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C45a40d6e3f3e43db7f1c08d7c4ceb6ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637194264422137307&amp;sdata=yRvpeioi434loufborv7ZemhIezNEF2yv9Lt1740Gn4%3D&amp;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]}
>>
>> 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%7C45a40d6e3f3e43db7f1c08d7c4ceb6ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637194264422137307&amp;sdata=Mk0f4lBq8dqfowge9Bqab8b%2B89eGFJBU6XJs6S%2F5CKQ%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%7C45a40d6e3f3e43db7f1c08d7c4ceb6ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637194264422137307&amp;sdata=Ha6v2Bwa%2BiIChPxIeJhuzsr%2F9VEa5%2BB73l90ABPhpgg%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%7C45a40d6e3f3e43db7f1c08d7c4ceb6ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637194264422137307&amp;sdata=q1KrQrdSHBul%2F36gx5eBZZTeK%2Bv3FQWxOMi7SfVcFgY%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%7C45a40d6e3f3e43db7f1c08d7c4ceb6ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637194264422137307&amp;sdata=0EVG%2Ba3uJHkTtYlcOyU%2F%2BMLOM7nksU3pkSG1ocZIZdU%3D&amp;reserved=0
> 
> 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).

Ok. Will send it ASAP.

> 
> 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
>>
>