[PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup

Kirill A. Shutemov posted 18 patches 1 year, 10 months ago
There is a newer version of this series
[PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
Posted by Kirill A. Shutemov 1 year, 10 months ago
ACPI MADT doesn't allow to offline CPU after it got woke up.

Currently CPU hotplug is prevented based on the confidential computing
attribute which is set for Intel TDX. But TDX is not the only possible
user of the wake up method.

Disable CPU offlining on ACPI MADT wakeup enumeration.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Tao Liu <ltao@redhat.com>
---
 arch/x86/coco/core.c               |  1 -
 arch/x86/kernel/acpi/madt_wakeup.c |  3 +++
 include/linux/cc_platform.h        | 10 ----------
 kernel/cpu.c                       |  3 +--
 4 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index b31ef2424d19..0f81f70aca82 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -29,7 +29,6 @@ static bool noinstr intel_cc_platform_has(enum cc_attr attr)
 {
 	switch (attr) {
 	case CC_ATTR_GUEST_UNROLL_STRING_IO:
-	case CC_ATTR_HOTPLUG_DISABLED:
 	case CC_ATTR_GUEST_MEM_ENCRYPT:
 	case CC_ATTR_MEM_ENCRYPT:
 		return true;
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index cf79ea6f3007..d222be8d7a07 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 #include <linux/acpi.h>
+#include <linux/cpu.h>
 #include <linux/io.h>
 #include <asm/apic.h>
 #include <asm/barrier.h>
@@ -76,6 +77,8 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
 
 	acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
 
+	cpu_hotplug_disable_offlining();
+
 	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
 
 	return 0;
diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
index 60693a145894..caa4b4430634 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -81,16 +81,6 @@ enum cc_attr {
 	 */
 	CC_ATTR_GUEST_SEV_SNP,
 
-	/**
-	 * @CC_ATTR_HOTPLUG_DISABLED: Hotplug is not supported or disabled.
-	 *
-	 * The platform/OS is running as a guest/virtual machine does not
-	 * support CPU hotplug feature.
-	 *
-	 * Examples include TDX Guest.
-	 */
-	CC_ATTR_HOTPLUG_DISABLED,
-
 	/**
 	 * @CC_ATTR_HOST_SEV_SNP: AMD SNP enabled on the host.
 	 *
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 08860baa6ce0..a70767aee9d0 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1528,8 +1528,7 @@ static int cpu_down_maps_locked(unsigned int cpu, enum cpuhp_state target)
 	 * If the platform does not support hotplug, report it explicitly to
 	 * differentiate it from a transient offlining failure.
 	 */
-	if (cc_platform_has(CC_ATTR_HOTPLUG_DISABLED) ||
-	    cpu_hotplug_offline_disabled)
+	if (cpu_hotplug_offline_disabled)
 		return -EOPNOTSUPP;
 	if (cpu_hotplug_disabled)
 		return -EBUSY;
-- 
2.43.0
Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
Posted by Borislav Petkov 1 year, 9 months ago
On Tue, Apr 09, 2024 at 02:29:56PM +0300, Kirill A. Shutemov wrote:
> ACPI MADT doesn't allow to offline CPU after it got woke up.

In all your text: s/woke/woken/g

> 
> Currently CPU hotplug is prevented based on the confidential computing
> attribute which is set for Intel TDX. But TDX is not the only possible
> user of the wake up method.
> 
> Disable CPU offlining on ACPI MADT wakeup enumeration.

Something's missing in that "justification". It should explain why
CC_ATTR_HOTPLUG_DISABLED is not needed anymore.

And looking at patch 3, I'm still unclear as to why this change is done.
Is it that the "ACPI MADT mailbox wakeup method" is going to be used by
TDX guests now too so that you don't need CC_ATTR_HOTPLUG_DISABLED
anymore?

It seems that if acpi_parse_mp_wake() finds an ok wakeup entry, then
offlining is disabled...

Or is it something else?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
Posted by Kirill A. Shutemov 1 year, 9 months ago
On Tue, Apr 23, 2024 at 06:02:58PM +0200, Borislav Petkov wrote:
> > 
> > Currently CPU hotplug is prevented based on the confidential computing
> > attribute which is set for Intel TDX. But TDX is not the only possible
> > user of the wake up method.
> > 
> > Disable CPU offlining on ACPI MADT wakeup enumeration.
> 
> Something's missing in that "justification". It should explain why
> CC_ATTR_HOTPLUG_DISABLED is not needed anymore.

It was wrong from beginning. If ACPI MADT wake up method is used on the
platform, we cannot handle offline, regardless if it is TDX or not.

> And looking at patch 3, I'm still unclear as to why this change is done.
> Is it that the "ACPI MADT mailbox wakeup method" is going to be used by
> TDX guests now too so that you don't need CC_ATTR_HOTPLUG_DISABLED
> anymore?

ACPI MADT is the only wakeup method supported in TDX guests. But offline
is broken is because of ACPI MADT, not because of TDX.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov
Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
Posted by Borislav Petkov 1 year, 9 months ago
On Wed, Apr 24, 2024 at 11:38:42AM +0300, Kirill A. Shutemov wrote:
> It was wrong from beginning. If ACPI MADT wake up method is used on the
> platform, we cannot handle offline, regardless if it is TDX or not.

Sounds to me like this fact should be a prominent part of the commit
message and these 1-4 patches should be carved out as a separate set
fixing that ACPI MADT thing and I should take them separately now...?

Also, does this need to go to stable although it is kinda big for
stable. If stable, do we need a smaller fix first which is backportable?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
Posted by Kirill A. Shutemov 1 year, 9 months ago
On Wed, Apr 24, 2024 at 03:50:52PM +0200, Borislav Petkov wrote:
> On Wed, Apr 24, 2024 at 11:38:42AM +0300, Kirill A. Shutemov wrote:
> > It was wrong from beginning. If ACPI MADT wake up method is used on the
> > platform, we cannot handle offline, regardless if it is TDX or not.
> 
> Sounds to me like this fact should be a prominent part of the commit
> message and these 1-4 patches should be carved out as a separate set
> fixing that ACPI MADT thing and I should take them separately now...?
> 
> Also, does this need to go to stable although it is kinda big for
> stable. If stable, do we need a smaller fix first which is backportable?

Correct me, if I am wrong, but I believe TDX guest is the only user of
ACPI MADT wake up method. At least it was added into kernel for TDX guest.
So it wouldn't fix anything user-visible. It might affect a future
platform that uses this wake up method, but it is a guessing game.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov
Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
Posted by Dave Hansen 1 year, 9 months ago
On 4/24/24 07:35, Kirill A. Shutemov wrote:
>> Also, does this need to go to stable although it is kinda big for
>> stable. If stable, do we need a smaller fix first which is backportable?
> Correct me, if I am wrong, but I believe TDX guest is the only user of
> ACPI MADT wake up method. At least it was added into kernel for TDX guest.
> So it wouldn't fix anything user-visible. It might affect a future
> platform that uses this wake up method, but it is a guessing game.

Yeah, the MADT wakeup is pretty funky, highly TDX specific, and not in
use or planned to be _put_ into use anywhere else.  The X86S "fix" for
running in real mode does something else entirely:

	https://cdrdv2.intel.com/v1/dl/getContent/776648
Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
Posted by Borislav Petkov 1 year, 9 months ago
On Wed, Apr 24, 2024 at 07:40:26AM -0700, Dave Hansen wrote:
> On 4/24/24 07:35, Kirill A. Shutemov wrote:
> >> Also, does this need to go to stable although it is kinda big for
> >> stable. If stable, do we need a smaller fix first which is backportable?
> > Correct me, if I am wrong, but I believe TDX guest is the only user of
> > ACPI MADT wake up method. At least it was added into kernel for TDX guest.
> > So it wouldn't fix anything user-visible. It might affect a future
> > platform that uses this wake up method, but it is a guessing game.
> 
> Yeah, the MADT wakeup is pretty funky, highly TDX specific, and not in
> use or planned to be _put_ into use anywhere else.

Then please make sure all that info is in the commit messages in the
next revision. Because as they are now, they're not even beginning to
explain what exactly is this fixing. /me thinking that this is some
generic fix needed in stable is case-in-point.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
[PATCHv10.1 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
Posted by Kirill A. Shutemov 1 year, 9 months ago
ACPI MADT doesn't allow to offline CPU after it got woke up.

Currently CPU hotplug is prevented based on the confidential computing
attribute which is set for Intel TDX. But TDX is not the only possible
user of the wake up method. Any platform that uses ACPI MADT wakeup
method cannot offline CPU.

Disable CPU offlining on ACPI MADT wakeup enumeration.

The change has no visible effects for users: currently, TDX guest is the
only platform that uses the ACPI MADT wakeup method.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Tao Liu <ltao@redhat.com>
---

  v10.1:
    - Update commit message.

---
 arch/x86/coco/core.c               |  1 -
 arch/x86/kernel/acpi/madt_wakeup.c |  3 +++
 include/linux/cc_platform.h        | 10 ----------
 kernel/cpu.c                       |  3 +--
 4 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index b31ef2424d19..0f81f70aca82 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -29,7 +29,6 @@ static bool noinstr intel_cc_platform_has(enum cc_attr attr)
 {
 	switch (attr) {
 	case CC_ATTR_GUEST_UNROLL_STRING_IO:
-	case CC_ATTR_HOTPLUG_DISABLED:
 	case CC_ATTR_GUEST_MEM_ENCRYPT:
 	case CC_ATTR_MEM_ENCRYPT:
 		return true;
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index cf79ea6f3007..d222be8d7a07 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 #include <linux/acpi.h>
+#include <linux/cpu.h>
 #include <linux/io.h>
 #include <asm/apic.h>
 #include <asm/barrier.h>
@@ -76,6 +77,8 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
 
 	acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
 
+	cpu_hotplug_disable_offlining();
+
 	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
 
 	return 0;
diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
index 60693a145894..caa4b4430634 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -81,16 +81,6 @@ enum cc_attr {
 	 */
 	CC_ATTR_GUEST_SEV_SNP,
 
-	/**
-	 * @CC_ATTR_HOTPLUG_DISABLED: Hotplug is not supported or disabled.
-	 *
-	 * The platform/OS is running as a guest/virtual machine does not
-	 * support CPU hotplug feature.
-	 *
-	 * Examples include TDX Guest.
-	 */
-	CC_ATTR_HOTPLUG_DISABLED,
-
 	/**
 	 * @CC_ATTR_HOST_SEV_SNP: AMD SNP enabled on the host.
 	 *
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6e1a9157d09c..09765f628b8b 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1528,8 +1528,7 @@ static int cpu_down_maps_locked(unsigned int cpu, enum cpuhp_state target)
 	 * If the platform does not support hotplug, report it explicitly to
 	 * differentiate it from a transient offlining failure.
 	 */
-	if (cc_platform_has(CC_ATTR_HOTPLUG_DISABLED) ||
-	    cpu_hotplug_offline_disabled)
+	if (cpu_hotplug_offline_disabled)
 		return -EOPNOTSUPP;
 	if (cpu_hotplug_disabled)
 		return -EBUSY;
-- 
2.43.0