Re: [PATCH v7 00/13] APIC ID fixes for AMD EPYC CPU model

[Eduardo Habkost]

On Tue, Mar 17, 2020 at 07:22:06PM -0400, Eduardo Habkost wrote:
> On Thu, Mar 12, 2020 at 11:28:47AM -0500, Babu Moger wrote:
> > Eduardo, Can you please queue the series if there are no concerns.
> > Thanks
> 
> I had queued it for today's pull request, but it looks like it
> breaks "make check".  See 
> https://travis-ci.org/github/ehabkost/qemu/jobs/663529282
> 
>   PASS 4 bios-tables-test /x86_64/acpi/piix4/ipmi
>   Could not access KVM kernel module: No such file or directory
>   qemu-system-x86_64: -accel kvm: failed to initialize kvm: No such file or 
> directory
>   qemu-system-x86_64: falling back to tcg
>   qemu-system-x86_64: Invalid CPU [socket: 0, die: 0, core: 1, thread: 0] 
> with APIC ID 1, valid index range 0:5
>   Broken pipe
>   /home/travis/build/ehabkost/qemu/tests/qtest/libqtest.c:166: kill_qemu() 
> tried to terminate QEMU process but encountered exit status 1 (expected 0)
>   Aborted (core dumped)
>   ERROR - too few tests run (expected 17, got 4)
>   /home/travis/build/ehabkost/qemu/tests/Makefile.include:633: recipe for 
> target 'check-qtest-x86_64' failed
>   make: *** [check-qtest-x86_64] Error 1

Failure is at the /x86_64/acpi/piix4/cpuhp test case:

  $ QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img 
tests/qtest/bios-tables-test -m=quick --verbose --debug-log
  [...]
  {*LOG(start):{/x86_64/acpi/piix4/cpuhp}:LOG*}
  # starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest 
unix:/tmp/qtest-2052313.sock -qtest-log /dev/null -chardev 
socket,path=/tmp/qtest-2052313.qmp,id=char0 -mon chardev=char0,mode=control 
-display none -machine pc,kernel-irqchip=off -accel kvm -accel tcg -net none 
-display none -smp 2,cores=3,sockets=2,maxcpus=6 -object 
memory-backend-ram,id=ram0,size=64M -object memory-backend-ram,id=ram1,size=64M 
-numa node,memdev=ram0 -numa node,memdev=ram1 -numa dist,src=0,dst=1,val=21 
-drive id=hd0,if=none,file=tests/acpi-test-disk-PVjFru,format=raw -device 
ide-hd,drive=hd0  -accel qtest
  {*LOG(message):{starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest 
unix:/tmp/qtest-2052313.sock -qtest-log /dev/null -chardev 
socket,path=/tmp/qtest-2052313.qmp,id=char0 -mon chardev=char0,mode=control 
-display none -machine pc,kernel-irqchip=off -accel kvm -accel tcg -net none 
-display none -smp 2,cores=3,sockets=2,maxcpus=6 -object 
memory-backend-ram,id=ram0,size=64M -object memory-backend-ram,id=ram1,size=64M 
-numa node,memdev=ram0 -numa node,memdev=ram1 -numa dist,src=0,dst=1,val=21 
-drive id=hd0,if=none,file=tests/acpi-test-disk-PVjFru,format=raw -device 
ide-hd,drive=hd0  -accel qtest}:LOG*}
  qemu-system-x86_64: Invalid CPU [socket: 0, die: 0, core: 1, thread: 0] with 
APIC ID 1, valid index range 0:5
  Broken pipe


> 
> 
> > 
> > On 3/11/20 5:52 PM, Babu Moger 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]}
> > > 
> > > v7:
> > >  Generated the patches on top of git://github.com/ehabkost/qemu.git 
> > > (x86-next).
> > >  Changes from v6.
> > >  1. Added new function x86_set_epyc_topo_handlers to override the apic id
> > >     encoding handlers.
> > >  2. Separated the code to set use_epyc_apic_id_encoding and added as a 
> > > new patch
> > >     as it looked more logical.
> > >  3. Fixed minor typos.
> > > 
> > > v6:
> > >  https://lore.kernel.org/qemu-devel/address@hidden/
> > >  Generated the patches on top of git://github.com/ehabkost/qemu.git 
> > > (x86-next).
> > >  Changes from v5.
> > >  1. Eduardo has already queued couple of patches, submitting the rest 
> > > here.
> > >  2. Major change is how the EPYC mode apic id encoding handlers are 
> > > loaded.
> > >     Added a boolean variable use_epyc_apic_id_encoding in 
> > > X86CPUDefinition. 
> > >     The variable is will be used to tell if we need to use EPYC mode 
> > > encoding.
> > >  3. Eduardo reported bysectability problem with x86 unit test code.
> > >     Quashed the patches in 1 and 2 to resolve it. Problem was change in 
> > > calling
> > >     conventions of topology related functions.
> > >  4. Also set the use_epyc_apic_id_encoding for EPYC-Rome. This model is
> > >     added recently to the cpu table.
> > > 
> > > v5:
> > >  https://lore.kernel.org/qemu-devel/address@hidden/
> > >  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/
> > > ---
> > > 
> > > Babu Moger (13):
> > >       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
> > >       i386: Introduce use_epyc_apic_id_encoding in X86CPUDefinition
> > >       hw/i386: Move arch_id decode inside x86_cpus_init
> > >       target/i386: Enable new apic id encoding for EPYC based cpus models
> > >       i386: Fix pkg_id offset for EPYC cpu models
> > > 
> > > 
> > >  hw/core/machine.c          |    1 
> > >  hw/i386/pc.c               |   15 ++-
> > >  hw/i386/x86.c              |   73 ++++++++++++----
> > >  include/hw/boards.h        |    2 
> > >  include/hw/i386/topology.h |  195 
> > > ++++++++++++++++++++++++++++++------------
> > >  include/hw/i386/x86.h      |   12 +++
> > >  softmmu/vl.c               |    1 
> > >  target/i386/cpu.c          |  203 
> > > ++++++++++++++------------------------------
> > >  target/i386/cpu.h          |    3 +
> > >  tests/test-x86-cpuid.c     |  116 +++++++++++++++----------
> > >  10 files changed, 358 insertions(+), 263 deletions(-)
> > > 
> > > --
> > > Signature
> > > 
> > 
> 
> -- 
> Eduardo

-- 
Eduardo