On Tue, Jun 28, 2022 at 12:04 PM Kai Huang <kai.huang@xxxxxxxxx> wrote: > > On Mon, 2022-06-27 at 10:01 +0200, Igor Mammedov wrote: > > 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. > > Hi Igor, > > Thanks for feedback. Yes the current implementation actually reports CPU hot- > add as BIOS bug. I think I can report BIOS bug for hot-removal too. And > currently I actually used BUG() for the hot-removal case. For hot-add I didn't > use BUG() but rejected the new CPU as the latter is more conservative. > > Hi Rafael, > > I am not sure I got what you mean by "This will affect initialization, not just > hotplug AFAICS", could you elaborate a little bit? Thanks. So acpi_processor_add() is called for CPUs that are already present at init time, not just for the hot-added ones. One of the things it does is to associate an ACPI companion with the given CPU. Don't you need that to happen?