Messages in this thread | | | Date | Thu, 20 Apr 2023 18:47:31 +0200 | Subject | Re: [patch 00/37] cpu/hotplug, x86: Reworked parallel CPU bringup | From | Paul Menzel <> |
| |
Dear Thomas,
Am 20.04.23 um 17:57 schrieb Thomas Gleixner: > On Thu, Apr 20 2023 at 07:51, Sean Christopherson wrote: >> On Thu, Apr 20, 2023, Thomas Gleixner wrote: >>> On Thu, Apr 20 2023 at 10:23, Andrew Cooper wrote: >>>> On 20/04/2023 9:32 am, Thomas Gleixner wrote: >>>>> On Wed, Apr 19, 2023, Andrew Cooper wrote: >>>>>> This was changed in x2APIC, which made the x2APIC_ID immutable. >>> >>>>> I'm pondering to simply deny parallel mode if x2APIC is not there. >>>> >>>> I'm not sure if that will help much. >>> >>> Spoilsport. >> >> LOL, well let me pile on then. x2APIC IDs aren't immutable on AMD hardware. The >> ID is read-only when the CPU is in x2APIC mode, but any changes made to the ID >> while the CPU is in xAPIC mode survive the transition to x2APIC. From the APM: >> >> A value previously written by software to the 8-bit APIC_ID register (MMIO offset >> 30h) is converted by hardware into the appropriate format and reflected into the >> 32-bit x2APIC_ID register (MSR 802h). >> >> FWIW, my observations from testing on bare metal are that the xAPIC ID is effectively >> read-only (writes are dropped) on Intel CPUs as far back as Haswell, while the above >> behavior described in the APM holds true on at least Rome and Milan. >> >> My guess is that Intel's uArch specific behavior of the xAPIC ID being read-only >> was introduced when x2APIC came along, but I didn't test farther back than Haswell. > > I'm not so worried about modern hardware. The horrorshow is the old muck > as demonstrated and of course there is virt :) > > Something like the completely untested below should just work whatever > APIC ID the BIOS decided to dice. > > That might just work on SEV too without that GHCB muck, but what do I > know. > > Thanks, > > tglx > --- > --- a/arch/x86/include/asm/apicdef.h > +++ b/arch/x86/include/asm/apicdef.h > @@ -138,7 +138,8 @@ > #define APIC_EILVT_MASKED (1 << 16) > > #define APIC_BASE (fix_to_virt(FIX_APIC_BASE)) > -#define APIC_BASE_MSR 0x800 > +#define APIC_BASE_MSR 0x800 > +#define APIC_X2APIC_ID_MSR 0x802 > #define XAPIC_ENABLE (1UL << 11) > #define X2APIC_ENABLE (1UL << 10) > > @@ -162,6 +163,7 @@ > #define APIC_CPUID(apicid) ((apicid) & XAPIC_DEST_CPUS_MASK) > #define NUM_APIC_CLUSTERS ((BAD_APICID + 1) >> XAPIC_DEST_CPUS_SHIFT) > > +#ifndef __ASSEMBLY__ > /* > * the local APIC register structure, memory mapped. Not terribly well > * tested, but we might eventually use this one in the future - the > @@ -435,4 +437,5 @@ enum apic_delivery_modes { > APIC_DELIVERY_MODE_EXTINT = 7, > }; > > +#endif /* !__ASSEMBLY__ */ > #endif /* _ASM_X86_APICDEF_H */ > --- a/arch/x86/include/asm/smp.h > +++ b/arch/x86/include/asm/smp.h > @@ -195,14 +195,13 @@ extern void nmi_selftest(void); > #endif > > extern unsigned int smpboot_control; > +extern unsigned long apic_mmio_base; > > #endif /* !__ASSEMBLY__ */ > > /* Control bits for startup_64 */ > -#define STARTUP_APICID_CPUID_1F 0x80000000 > -#define STARTUP_APICID_CPUID_0B 0x40000000 > -#define STARTUP_APICID_CPUID_01 0x20000000 > -#define STARTUP_APICID_SEV_ES 0x10000000 > +#define STARTUP_READ_APICID 0x80000000 > +#define STARTUP_APICID_SEV_ES 0x40000000 > > /* Top 8 bits are reserved for control */ > #define STARTUP_PARALLEL_MASK 0xFF000000 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -101,6 +101,8 @@ static int apic_extnmi __ro_after_init = > */ > static bool virt_ext_dest_id __ro_after_init; > > +unsigned long apic_mmio_base __ro_after_init; > + > /* > * Map cpu index to physical APIC ID > */ > @@ -2164,6 +2166,7 @@ void __init register_lapic_address(unsig > > if (!x2apic_mode) { > set_fixmap_nocache(FIX_APIC_BASE, address); > + apic_mmio_base = APIC_BASE; > apic_printk(APIC_VERBOSE, "mapped APIC to %16lx (%16lx)\n", > APIC_BASE, address); > } > --- a/arch/x86/kernel/head_64.S > +++ b/arch/x86/kernel/head_64.S > @@ -24,8 +24,10 @@ > #include "../entry/calling.h" > #include <asm/export.h> > #include <asm/nospec-branch.h> > +#include <asm/apicdef.h> > #include <asm/fixmap.h> > #include <asm/smp.h> > + > #include <asm/sev-common.h> > > /* > @@ -237,37 +239,24 @@ SYM_INNER_LABEL(secondary_startup_64_no_ > > #ifdef CONFIG_SMP > /* > - * For parallel boot, the APIC ID is retrieved from CPUID, and then > - * used to look up the CPU number. For booting a single CPU, the > - * CPU number is encoded in smpboot_control. > + * For parallel boot, the APIC ID is either retrieved the APIC or > + * from CPUID, and then used to look up the CPU number. > + * For booting a single CPU, the CPU number is encoded in > + * smpboot_control. > * > - * Bit 31 STARTUP_APICID_CPUID_1F flag (use CPUID 0x1f) > - * Bit 30 STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b) > - * Bit 29 STARTUP_APICID_CPUID_01 flag (use CPUID 0x01) > - * Bit 28 STARTUP_APICID_SEV_ES flag (CPUID 0x0b via GHCB MSR) > + * Bit 31 STARTUP_APICID_READ (Read APICID from APIC) > + * Bit 30 STARTUP_APICID_SEV_ES flag (CPUID 0x0b via GHCB MSR) > * Bit 0-23 CPU# if STARTUP_APICID_CPUID_xx flags are not set > */ > movl smpboot_control(%rip), %ecx > + testl $STARTUP_READ_APICID, %ecx > #ifdef CONFIG_AMD_MEM_ENCRYPT > testl $STARTUP_APICID_SEV_ES, %ecx > jnz .Luse_sev_cpuid_0b > #endif > - testl $STARTUP_APICID_CPUID_1F, %ecx > - jnz .Luse_cpuid_1f > - testl $STARTUP_APICID_CPUID_0B, %ecx > - jnz .Luse_cpuid_0b > - testl $STARTUP_APICID_CPUID_01, %ecx > - jnz .Luse_cpuid_01 > andl $(~STARTUP_PARALLEL_MASK), %ecx > jmp .Lsetup_cpu > > -.Luse_cpuid_01: > - mov $0x01, %eax > - cpuid > - mov %ebx, %edx > - shr $24, %edx > - jmp .Lsetup_AP > - > #ifdef CONFIG_AMD_MEM_ENCRYPT > .Luse_sev_cpuid_0b: > /* Set the GHCB MSR to request CPUID 0x0B_EDX */ > @@ -292,24 +281,30 @@ SYM_INNER_LABEL(secondary_startup_64_no_ > jmp .Lsetup_AP > #endif > > -.Luse_cpuid_0b: > - mov $0x0B, %eax > - xorl %ecx, %ecx > - cpuid > - jmp .Lsetup_AP > +.Lread_apicid: > + mov $MSR_IA32_APICBASE, %ecx > + rdmsr > + testl $X2APIC_ENABLE, %eax > + jnz read_apicid_msr > + > + /* Read the APIC ID from the fix-mapped MMIO space. */ > + movq apic_mmio_base(%rip), %rcx > + addq $APIC_ID, %rcx > + movl (%rcx), %eax > + shr $24, %eax > + jnz .Lread_apicid > > -.Luse_cpuid_1f: > - mov $0x1f, %eax > - xorl %ecx, %ecx > - cpuid > +.Lread_apicid_msr: > + mov $APIC_X2APIC_ID_MSR, %ecx > + rdmsr > > .Lsetup_AP: > - /* EDX contains the APIC ID of the current CPU */ > + /* EAX contains the APIC ID of the current CPU */ > xorq %rcx, %rcx > leaq cpuid_to_apicid(%rip), %rbx > > .Lfind_cpunr: > - cmpl (%rbx,%rcx,4), %edx > + cmpl (%rbx,%rcx,4), %eax > jz .Lsetup_cpu > inc %ecx > #ifdef CONFIG_FORCE_NR_CPUS > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1253,41 +1253,22 @@ bool __init arch_cpuhp_init_parallel_bri > return false; > } > > - /* Encrypted guests require special CPUID handling. */ > + /* Encrypted guests require special handling. */ > if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) { > switch (cc_get_vendor()) { > case CC_VENDOR_AMD: > ctrl = STARTUP_APICID_SEV_ES; > if (topology_extended_leaf == 0x0b) > - goto setup; > + break; > fallthrough; > default: > pr_info("Parallel CPU startup disabled due to guest state encryption\n"); > return false; > } > + } else { > + ctrl = STARTUP_READ_APICID; > } > > - switch (topology_extended_leaf) { > - case 0x0b: > - ctrl = STARTUP_APICID_CPUID_0B; > - break; > - case 0x1f: > - ctrl = STARTUP_APICID_CPUID_1F; > - break; > - case 0x00: > - /* For !x2APIC mode 8 bits from leaf 0x01 are sufficient. */ > - if (!x2apic_mode) { > - ctrl = STARTUP_APICID_CPUID_01; > - break; > - } > - fallthrough; > - default: > - pr_info("Parallel CPU startup disabled. Unsupported topology leaf %u\n", > - topology_extended_leaf); > - return false; > - } > - > -setup: > pr_debug("Parallel CPU startup enabled: 0x%08x\n", ctrl); > smpboot_control = ctrl; > return true;
I quickly applied it on top of your branch, but I am getting:
``` $ wget https://lore.kernel.org/lkml/87v8hq35sk.ffs@tglx/raw $ patch -p1 < raw $ make […] LD .tmp_vmlinux.kallsyms1 ld: arch/x86/kernel/head_64.o: in function `secondary_startup_64_no_verify': (.head.text+0xbf): undefined reference to `read_apicid_msr' make[1]: *** [scripts/Makefile.vmlinux:35: vmlinux] Error 1 make: *** [Makefile:1249: vmlinux] Error 2 ```
Kind regards,
Paul
| |