Re: [PATCH v5 02/22] cc_platform: Add new attribute to prevent ACPI CPU hotplug

[Rafael J. Wysocki]

On Wed, Jun 22, 2022 at 1:16 PM Kai Huang <kai.huang@xxxxxxxxx> wrote:
>
> Platforms with confidential computing technology may not support ACPI
> CPU hotplug when such technology is enabled by the BIOS.  Examples
> include Intel platforms which support Intel Trust Domain Extensions
> (TDX).
>
> If the kernel ever receives ACPI CPU hotplug event, it is likely a BIOS
> bug.  For ACPI CPU hot-add, the kernel should speak out this is a BIOS
> bug and reject the new CPU.  For hot-removal, for simplicity just assume
> the kernel cannot continue to work normally, and BUG().
>
> Add a new attribute CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED to indicate the
> platform doesn't support ACPI CPU hotplug, so that kernel can handle
> ACPI CPU hotplug events for such platform.  The existing attribute
> CC_ATTR_HOTPLUG_DISABLED is for software CPU hotplug thus doesn't fit.
>
> In acpi_processor_{add|remove}(), add early check against this attribute
> and handle accordingly if it is set.
>
> Also take this chance to rename existing CC_ATTR_HOTPLUG_DISABLED to
> CC_ATTR_CPU_HOTPLUG_DISABLED as it is for software CPU hotplug.
>
> Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
> ---
>  arch/x86/coco/core.c          |  2 +-
>  drivers/acpi/acpi_processor.c | 23 +++++++++++++++++++++++
>  include/linux/cc_platform.h   | 15 +++++++++++++--
>  kernel/cpu.c                  |  2 +-
>  4 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index 4320fadae716..1bde1af75296 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -20,7 +20,7 @@ static bool 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_CPU_HOTPLUG_DISABLED:
>         case CC_ATTR_GUEST_MEM_ENCRYPT:
>         case CC_ATTR_MEM_ENCRYPT:
>                 return true;
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 6737b1cbf6d6..b960db864cd4 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -15,6 +15,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/cc_platform.h>
>
>  #include <acpi/processor.h>
>
> @@ -357,6 +358,17 @@ static int acpi_processor_add(struct acpi_device *device,
>         struct device *dev;
>         int result = 0;
>
> +       /*
> +        * If the confidential computing platform doesn't support ACPI
> +        * memory hotplug, the BIOS should never deliver such event to
> +        * the kernel.  Report ACPI CPU hot-add as a BIOS bug and ignore
> +        * the new CPU.
> +        */
> +       if (cc_platform_has(CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED)) {

This will affect initialization, not just hotplug AFAICS.

You should reset the .hotplug.enabled flag in processor_handler to
false instead.

> +               dev_err(&device->dev, "[BIOS bug]: Platform doesn't support ACPI CPU hotplug.  New CPU ignored.\n");
> +               return -EINVAL;
> +       }
> +
>         pr = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL);
>         if (!pr)
>                 return -ENOMEM;
> @@ -434,6 +446,17 @@ static void acpi_processor_remove(struct acpi_device *device)
>         if (!device || !acpi_driver_data(device))
>                 return;
>
> +       /*
> +        * The confidential computing platform is broken if ACPI memory
> +        * hot-removal isn't supported but it happened anyway.  Assume
> +        * it's not guaranteed that the kernel can continue to work
> +        * normally.  Just BUG().
> +        */
> +       if (cc_platform_has(CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED)) {
> +               dev_err(&device->dev, "Platform doesn't support ACPI CPU hotplug. BUG().\n");
> +               BUG();
> +       }
> +
>         pr = acpi_driver_data(device);
>         if (pr->id >= nr_cpu_ids)
>                 goto out;
> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
> index 691494bbaf5a..9ce9256facc8 100644
> --- a/include/linux/cc_platform.h
> +++ b/include/linux/cc_platform.h
> @@ -74,14 +74,25 @@ enum cc_attr {
>         CC_ATTR_GUEST_UNROLL_STRING_IO,
>
>         /**
> -        * @CC_ATTR_HOTPLUG_DISABLED: Hotplug is not supported or disabled.
> +        * @CC_ATTR_CPU_HOTPLUG_DISABLED: CPU 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_CPU_HOTPLUG_DISABLED,
> +
> +       /**
> +        * @CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED: ACPI CPU hotplug is not
> +        *                                     supported.
> +        *
> +        * The platform/OS does not support ACPI CPU hotplug.
> +        *
> +        * Examples include TDX platform.
> +        */
> +       CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED,
>  };
>
>  #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index edb8c199f6a3..966772cce063 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1191,7 +1191,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))
> +       if (cc_platform_has(CC_ATTR_CPU_HOTPLUG_DISABLED))
>                 return -EOPNOTSUPP;
>         if (cpu_hotplug_disabled)
>                 return -EBUSY;
> --
> 2.36.1
>