From: Paul Menzel <pmenzel@molgen.mpg.de>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: "Sean Christopherson" <seanjc@google.com>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
linux-kernel@vger.kernel.org, x86@kernel.org,
"David Woodhouse" <dwmw2@infradead.org>,
"Brian Gerst" <brgerst@gmail.com>,
"Arjan van de Veen" <arjan@linux.intel.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Paul McKenney" <paulmck@kernel.org>,
"Tom Lendacky" <thomas.lendacky@amd.com>,
"Oleksandr Natalenko" <oleksandr@natalenko.name>,
"Guilherme G. Piccoli" <gpiccoli@igalia.com>,
"Piotr Gorski" <lucjan.lucjanov@gmail.com>,
"David Woodhouse" <dwmw@amazon.co.uk>,
"Usama Arif" <usama.arif@bytedance.com>,
"Jürgen Groß" <jgross@suse.com>,
"Boris Ostrovsky" <boris.ostrovsky@oracle.com>,
xen-devel@lists.xenproject.org,
"Russell King" <linux@armlinux.org.uk>,
"Arnd Bergmann" <arnd@arndb.de>,
linux-arm-kernel@lists.infradead.org,
"Catalin Marinas" <catalin.marinas@arm.com>,
"Will Deacon" <will@kernel.org>, "Guo Ren" <guoren@kernel.org>,
linux-csky@vger.kernel.org,
"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
linux-mips@vger.kernel.org,
"James E. J. Bottomley" <James.Bottomley@hansenpartnership.com>,
"Helge Deller" <deller@gmx.de>,
linux-parisc@vger.kernel.org,
"Paul Walmsley" <paul.walmsley@sifive.com>,
"Palmer Dabbelt" <palmer@dabbelt.com>,
linux-riscv@lists.infradead.org,
"Mark Rutland" <mark.rutland@arm.com>,
"Sabin Rapan" <sabrapan@amazon.com>
Subject: Re: [patch 00/37] cpu/hotplug, x86: Reworked parallel CPU bringup
Date: Thu, 20 Apr 2023 18:47:31 +0200 [thread overview]
Message-ID: <56e59a4d-a47f-4bfe-7db5-5f921062ad69@molgen.mpg.de> (raw)
In-Reply-To: <87v8hq35sk.ffs@tglx>
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
next prev parent reply other threads:[~2023-04-20 16:47 UTC|newest]
Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-14 23:44 [patch 00/37] cpu/hotplug, x86: Reworked parallel CPU bringup Thomas Gleixner
2023-04-14 23:44 ` [patch 01/37] x86/smpboot: Cleanup topology_phys_to_logical_pkg()/die() Thomas Gleixner
2023-04-14 23:44 ` [patch 02/37] cpu/hotplug: Mark arch_disable_smp_support() and bringup_nonboot_cpus() __init Thomas Gleixner
2023-04-14 23:44 ` [patch 03/37] x86/smpboot: Avoid pointless delay calibration is TSC is synchronized Thomas Gleixner
2023-04-14 23:44 ` [patch 04/37] x86/smpboot: Rename start_cpu0() to soft_restart_cpu() Thomas Gleixner
2023-04-14 23:44 ` [patch 05/37] x86/topology: Remove CPU0 hotplug option Thomas Gleixner
2023-06-21 16:50 ` Paul E. McKenney
2023-04-14 23:44 ` [patch 06/37] x86/smpboot: Remove the CPU0 hotplug kludge Thomas Gleixner
2023-04-14 23:44 ` [patch 07/37] x86/smpboot: Restrict soft_restart_cpu() to SEV Thomas Gleixner
2023-04-14 23:44 ` [patch 08/37] x86/smpboot: Split up native_cpu_up() into separate phases and document them Thomas Gleixner
2023-04-14 23:44 ` [patch 09/37] x86/smpboot: Get rid of cpu_init_secondary() Thomas Gleixner
2023-04-14 23:44 ` [patch 10/37] x86/cpu/cacheinfo: Remove cpu_callout_mask dependency Thomas Gleixner
2023-04-14 23:44 ` [patch 11/37] x86/smpboot: Move synchronization masks to SMP boot code Thomas Gleixner
2023-04-14 23:44 ` [patch 12/37] x86/smpboot: Make TSC synchronization function call based Thomas Gleixner
2023-04-14 23:44 ` [patch 13/37] x86/smpboot: Remove cpu_callin_mask Thomas Gleixner
2023-04-14 23:44 ` [patch 14/37] cpu/hotplug: Rework sparse_irq locking in bringup_cpu() Thomas Gleixner
2023-04-14 23:44 ` [patch 15/37] x86/smpboot: Remove wait for cpu_online() Thomas Gleixner
2023-04-14 23:44 ` [patch 16/37] x86/xen/smp_pv: Remove wait for CPU online Thomas Gleixner
2023-04-17 20:46 ` Boris Ostrovsky
2023-04-14 23:44 ` [patch 17/37] x86/xen/hvm: Get rid of DEAD_FROZEN handling Thomas Gleixner
2023-04-14 23:44 ` [patch 18/37] cpu/hotplug: Add CPU state tracking and synchronization Thomas Gleixner
2023-04-14 23:44 ` [patch 19/37] x86/smpboot: Switch to hotplug core state synchronization Thomas Gleixner
2023-04-15 12:58 ` Brian Gerst
2023-04-15 21:04 ` Thomas Gleixner
2023-04-14 23:44 ` [patch 20/37] cpu/hotplug: Remove cpu_report_state() and related unused cruft Thomas Gleixner
2023-04-14 23:44 ` [patch 21/37] ARM: smp: Switch to hotplug core state synchronization Thomas Gleixner
2023-04-14 23:44 ` [patch 22/37] arm64: " Thomas Gleixner
2023-04-17 15:50 ` Mark Rutland
2023-04-25 19:51 ` Thomas Gleixner
2023-04-26 7:59 ` Mark Rutland
2023-04-26 8:15 ` Thomas Gleixner
2023-04-14 23:44 ` [patch 23/37] csky/smp: " Thomas Gleixner
2023-04-14 23:44 ` [patch 24/37] MIPS: SMP_CPS: " Thomas Gleixner
2023-04-14 23:44 ` [patch 25/37] parisc: " Thomas Gleixner
2023-04-14 23:44 ` [patch 26/37] riscv: " Thomas Gleixner
2023-05-01 23:55 ` Palmer Dabbelt
2023-04-14 23:44 ` [patch 27/37] cpu/hotplug: Remove unused state functions Thomas Gleixner
2023-04-14 23:44 ` [patch 28/37] cpu/hotplug: Reset task stack state in _cpu_up() Thomas Gleixner
2023-04-14 23:45 ` [patch 29/37] cpu/hotplug: Provide a split up CPUHP_BRINGUP mechanism Thomas Gleixner
2023-04-14 23:45 ` [patch 30/37] x86/smpboot: Enable split CPU startup Thomas Gleixner
2023-04-14 23:45 ` [patch 31/37] x86/apic: Provide cpu_primary_thread mask Thomas Gleixner
2023-04-14 23:45 ` [patch 32/37] cpu/hotplug: Allow "parallel" bringup up to CPUHP_BP_KICK_AP_STATE Thomas Gleixner
2023-04-14 23:45 ` [patch 33/37] x86/topology: Store extended topology leaf information Thomas Gleixner
2023-04-14 23:45 ` [patch 34/37] x86/cpu/amd; Invoke detect_extended_topology_early() on boot CPU Thomas Gleixner
2023-04-14 23:45 ` [patch 35/37] x86/smpboot: Support parallel startup of secondary CPUs Thomas Gleixner
2023-04-15 13:22 ` Brian Gerst
2023-04-15 21:06 ` Thomas Gleixner
2023-04-24 17:58 ` Thomas Gleixner
2023-04-14 23:45 ` [patch 36/37] x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it Thomas Gleixner
2023-04-14 23:45 ` [patch 37/37] x86/smpboot: Allow parallel bringup for SEV-ES Thomas Gleixner
2023-04-17 8:35 ` [patch 00/37] cpu/hotplug, x86: Reworked parallel CPU bringup Juergen Gross
2023-04-17 10:30 ` Peter Zijlstra
2023-04-17 10:44 ` Andrew Cooper
2023-04-17 11:19 ` Paul Menzel
2023-04-17 11:24 ` Paul Menzel
2023-04-17 14:48 ` Thomas Gleixner
2023-04-17 17:40 ` Paul Menzel
2023-04-18 6:58 ` Thomas Gleixner
2023-04-18 8:40 ` Thomas Gleixner
2023-04-18 20:10 ` Paul Menzel
2023-04-19 9:38 ` Thomas Gleixner
2023-04-19 12:38 ` Thomas Gleixner
2023-04-19 13:32 ` David Woodhouse
2023-04-19 13:43 ` Thomas Gleixner
2023-04-19 13:50 ` Andrew Cooper
2023-04-19 16:21 ` Andrew Cooper
2023-04-20 8:32 ` Thomas Gleixner
2023-04-20 9:23 ` Andrew Cooper
2023-04-20 11:17 ` Thomas Gleixner
2023-04-20 14:51 ` Sean Christopherson
2023-04-20 15:57 ` Thomas Gleixner
2023-04-20 16:47 ` Paul Menzel [this message]
2023-04-20 19:10 ` Thomas Gleixner
2023-04-21 16:36 ` Thomas Gleixner
2023-04-24 18:46 ` Paul Menzel
2023-04-25 20:07 ` Thomas Gleixner
2023-04-27 14:48 ` Michael Kelley (LINUX)
2023-05-04 18:46 ` Thomas Gleixner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56e59a4d-a47f-4bfe-7db5-5f921062ad69@molgen.mpg.de \
--to=pmenzel@molgen.mpg.de \
--cc=James.Bottomley@hansenpartnership.com \
--cc=andrew.cooper3@citrix.com \
--cc=arjan@linux.intel.com \
--cc=arnd@arndb.de \
--cc=boris.ostrovsky@oracle.com \
--cc=brgerst@gmail.com \
--cc=catalin.marinas@arm.com \
--cc=deller@gmx.de \
--cc=dwmw2@infradead.org \
--cc=dwmw@amazon.co.uk \
--cc=gpiccoli@igalia.com \
--cc=guoren@kernel.org \
--cc=jgross@suse.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-csky@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux@armlinux.org.uk \
--cc=lucjan.lucjanov@gmail.com \
--cc=mark.rutland@arm.com \
--cc=oleksandr@natalenko.name \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=paulmck@kernel.org \
--cc=pbonzini@redhat.com \
--cc=sabrapan@amazon.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=tsbogend@alpha.franken.de \
--cc=usama.arif@bytedance.com \
--cc=will@kernel.org \
--cc=x86@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).