ACPI MADT doesn't allow to offline CPU after it got woke up.
Currently hotplug 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.
Mark CPU hotplug as "not supported" on ACPI MADT wakeup enumeration.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/coco/core.c | 1 -
arch/x86/kernel/acpi/madt_wakeup.c | 4 ++++
include/linux/cc_platform.h | 10 ----------
kernel/cpu.c | 2 +-
4 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index eeec9986570e..f07c3bb7deab 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -20,7 +20,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 1b9747bfd5b9..15bdf10b1393 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -1,4 +1,5 @@
#include <linux/acpi.h>
+#include <linux/cpu.h>
#include <asm/apic.h>
/* Physical address of the Multiprocessor Wakeup Structure mailbox */
@@ -74,6 +75,9 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
+ /* Disable CPU onlining/offlining */
+ cpu_hotplug_not_supported();
+
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 cb0d6cd1c12f..d08dd65b5c43 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -80,16 +80,6 @@ enum cc_attr {
* using AMD SEV-SNP features.
*/
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,
};
#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
diff --git a/kernel/cpu.c b/kernel/cpu.c
index cf536fe1a88a..9d4279476b40 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1522,7 +1522,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_supported)
+ if (!cpu_hotplug_supported)
return -EOPNOTSUPP;
if (cpu_hotplug_disabled)
return -EBUSY;
--
2.41.0
> /* Physical address of the Multiprocessor Wakeup Structure mailbox */
> @@ -74,6 +75,9 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
>
> acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
>
> + /* Disable CPU onlining/offlining */
> + cpu_hotplug_not_supported();
> +
Both onlining/offlining are prevented, or just offlining?
The previous patch says:
It does not prevent the initial bring up of the CPU, but it stops
subsequent offlining.
And ...
[...]
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1522,7 +1522,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_supported)
> + if (!cpu_hotplug_supported)
> return -EOPNOTSUPP;
> if (cpu_hotplug_disabled)
> return -EBUSY;
... here cpu_down_maps_locked() only prevents offlining if I am reading
correctly.
Also, can we rename cpu_hotplug_supported to cpu_offline_supported to match the
behaviour better?
Anyway, isn't it a little bit odd to have:
if (!cpu_hotplug_supported)
return -EOPNOTSUPP;
if (cpu_hotplug_disabled)
return -EBUSY;
?
On 10/5/2023 6:13 AM, Kirill A. Shutemov wrote:
> ACPI MADT doesn't allow to offline CPU after it got woke up.
>
I think you can use the term "CPU hotplug" instead of just offline.
> Currently hotplug 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.
>
> Mark CPU hotplug as "not supported" on ACPI MADT wakeup enumeration.
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> ---
> arch/x86/coco/core.c | 1 -
> arch/x86/kernel/acpi/madt_wakeup.c | 4 ++++
> include/linux/cc_platform.h | 10 ----------
> kernel/cpu.c | 2 +-
> 4 files changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index eeec9986570e..f07c3bb7deab 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -20,7 +20,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 1b9747bfd5b9..15bdf10b1393 100644
> --- a/arch/x86/kernel/acpi/madt_wakeup.c
> +++ b/arch/x86/kernel/acpi/madt_wakeup.c
> @@ -1,4 +1,5 @@
> #include <linux/acpi.h>
> +#include <linux/cpu.h>
> #include <asm/apic.h>
>
> /* Physical address of the Multiprocessor Wakeup Structure mailbox */
> @@ -74,6 +75,9 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
>
> acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
>
> + /* Disable CPU onlining/offlining */
> + cpu_hotplug_not_supported();
> +
> 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 cb0d6cd1c12f..d08dd65b5c43 100644
> --- a/include/linux/cc_platform.h
> +++ b/include/linux/cc_platform.h
> @@ -80,16 +80,6 @@ enum cc_attr {
> * using AMD SEV-SNP features.
> */
> 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,
> };
>
> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index cf536fe1a88a..9d4279476b40 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1522,7 +1522,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_supported)
> + if (!cpu_hotplug_supported)
> return -EOPNOTSUPP;
> if (cpu_hotplug_disabled)
> return -EBUSY;
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Thu, Oct 05 2023 at 16:13, Kirill A. Shutemov wrote:
>
> + /* Disable CPU onlining/offlining */
That's not what the function does.
> + cpu_hotplug_not_supported();
Thanks,
tglx
On Tue, 2023-10-10 at 10:24 +0000, Huang, Kai wrote:
> > /* Physical address of the Multiprocessor Wakeup Structure mailbox */
> > @@ -74,6 +75,9 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> >
> >
> > acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
> >
> >
> > + /* Disable CPU onlining/offlining */
> > + cpu_hotplug_not_supported();
> > +
>
> Both onlining/offlining are prevented, or just offlining?
>
> The previous patch says:
>
> It does not prevent the initial bring up of the CPU, but it stops
> subsequent offlining.
>
> And ...
>
> [...]
>
>
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -1522,7 +1522,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_supported)
> > + if (!cpu_hotplug_supported)
> > return -EOPNOTSUPP;
> > if (cpu_hotplug_disabled)
> > return -EBUSY;
>
> ... here cpu_down_maps_locked() only prevents offlining if I am reading
> correctly.
>
> Also, can we rename cpu_hotplug_supported to cpu_offline_supported to match the
> behaviour better?
>
> Anyway, isn't it a little bit odd to have:
>
> if (!cpu_hotplug_supported)
> return -EOPNOTSUPP;
> if (cpu_hotplug_disabled)
> return -EBUSY;
>
> ?
I probably have missed something important, but I don't quite understand what's
the reason to have the CC_ATTR_HOTPLUG_DISABLED at the beginning, and now
replace it with cpu_hotplug_not_supported()?
From the changelog:
Currently hotplug 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.
CC_ATTR_HOTPLUG_DISABLED is only used by TDX guest, but MADT can be used by non-
TDX guest too.
Anyway, if the purpose is just to prevent CPU from going offline, can this be
done by registering a cpuhp callback?
static int madt_wakeup_offline_cpu(unsigned int cpu)
{
return -EOPNOTSUPP;
}
...
err = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "madt-wakeup",
NULL, madt_wakeup_offline_cpu);
if (err) {
pr_err("Register CPU hotplug callback failed.\n");
/* BUG() ??? */
}
This doesn't pollute the common CPU hotplug code, thus to me looks more clear?
On Fri, Oct 20, 2023 at 11:58:58AM +0000, Huang, Kai wrote:
> On Tue, 2023-10-10 at 10:24 +0000, Huang, Kai wrote:
> > > ?/* Physical address of the Multiprocessor Wakeup Structure mailbox */
> > > @@ -74,6 +75,9 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> > > ?
> > >
> > > ? acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
> > > ?
> > >
> > > + /* Disable CPU onlining/offlining */
> > > + cpu_hotplug_not_supported();
> > > +
> >
> > Both onlining/offlining are prevented, or just offlining?
> >
> > The previous patch says:
> >
> > It does not prevent the initial bring up of the CPU, but it stops?
> > subsequent offlining.
> >
> > And ...
> >
> > [...]
> >
> >
> > > --- a/kernel/cpu.c
> > > +++ b/kernel/cpu.c
> > > @@ -1522,7 +1522,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_supported)
> > > + if (!cpu_hotplug_supported)
> > > ? return -EOPNOTSUPP;
> > > ? if (cpu_hotplug_disabled)
> > > ? return -EBUSY;
> >
> > ... here cpu_down_maps_locked() only prevents offlining if I am reading
> > correctly.
> >
> > Also, can we rename cpu_hotplug_supported to cpu_offline_supported to match the
> > behaviour better?
> >
> > Anyway, isn't it a little bit odd to have:
> >
> > if (!cpu_hotplug_supported)
> > ? return -EOPNOTSUPP;
> > ? if (cpu_hotplug_disabled)
> > ? return -EBUSY;
> >
> > ?
>
> I probably have missed something important, but I don't quite understand what's
> the reason to have the CC_ATTR_HOTPLUG_DISABLED at the beginning, and now
> replace it with cpu_hotplug_not_supported()?
CC_ATTR_HOTPLUG_DISABLED was a mistake. And now obvious when we only need
to disable offlining dynamically, based on supported MADT MP WP version.
> From the changelog:
>
> Currently hotplug 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.
>
> CC_ATTR_HOTPLUG_DISABLED is only used by TDX guest, but MADT can be used by non-
> TDX guest too.
>
> Anyway, if the purpose is just to prevent CPU from going offline, can this be
> done by registering a cpuhp callback?
>
> static int madt_wakeup_offline_cpu(unsigned int cpu)
> {
> return -EOPNOTSUPP;
> }
>
> ...
>
> err = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "madt-wakeup",
> NULL, madt_wakeup_offline_cpu);
> if (err) {
> pr_err("Register CPU hotplug callback failed.\n");
> /* BUG() ??? */
> }
>
> This doesn't pollute the common CPU hotplug code, thus to me looks more clear?
Thomas seems fine with cpu_hotplug_disable_offlining().
--
Kiryl Shutsemau / Kirill A. Shutemov