lkml.org 
[lkml]   [2023]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [patch 00/37] cpu/hotplug, x86: Reworked parallel CPU bringup
    From
    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

    \
     
     \ /
      Last update: 2023-04-20 18:48    [W:4.190 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site