On Thu, 23 Jun 2022 12:01:48 +1200
Kai Huang <kai.huang@xxxxxxxxx> wrote:
> 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?
The thing is that by the time ACPI machinery kicks in, physical hotplug
has already happened and in case of (kvm+qemu+ovmf hypervisor combo)
firmware has already handled it somehow and handed it over to ACPI.
If you say it's architectural thing then cpu hotplug is platform/firmware
bug and should be disabled there instead of working around it in the kernel.
Perhaps instead of 'preventing' hotplug, complain/panic and be done with it.
> --- 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;
> > > + }
> > > +
>
