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

[Kai Huang]

On Wed, 2022-06-22 at 13:42 +0200, Rafael J. Wysocki wrote:
> 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.

Hi Rafael,

Thanks for the review.  By "affect initialization" did you mean this
acpi_processor_add() is also called during kernel boot when any logical cpu is
brought up?  Or do you mean ACPI CPU hotplug can also happen during kernel boot
(after acpi_processor_init())?

I see acpi_processor_init() calls acpi_processor_check_duplicates() which calls
acpi_evaluate_object() but I don't know details of ACPI so I don't know whether
this would trigger acpi_processor_add().

One thing is TDX doesn't support ACPI CPU hotplug is an architectural thing, so
it is illegal even if it happens during kernel boot.  Dave's idea is the kernel
should  speak out loudly if physical CPU hotplug indeed happened on (BIOS) TDX-
enabled platforms.  Otherwise perhaps we can just give up initializing the ACPI
CPU hotplug in acpi_processor_init(), something like below?

--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -707,6 +707,10 @@ bool acpi_duplicate_processor_id(int proc_id)
 void __init acpi_processor_init(void)
 {
        acpi_processor_check_duplicates();
+
+       if (cc_platform_has(CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED))
+               return;
+
        acpi_scan_add_handler_with_hotplug(&processor_handler, "processor");
        acpi_scan_add_handler(&processor_container_handler);
 }


> 
> > +               dev_err(&device->dev, "[BIOS bug]: Platform doesn't support ACPI CPU hotplug.  New CPU ignored.\n");
> > +               return -EINVAL;
> > +       }
> > +

-- 
Thanks,
-Kai