| Message ID | 87dc19c47bad73509359c8e1e3a81d51d1681e4c.1655894131.git.kai.huang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | TDX host kernel support | expand |
On Wed, 2022-06-22 at 13:45 +0200, Rafael J. Wysocki wrote: > On Wed, Jun 22, 2022 at 1:16 PM Kai Huang <kai.huang@intel.com> wrote: > > > > Platforms with confidential computing technology may not support ACPI > > memory 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 memory hotplug event, it is likely a > > BIOS bug. For ACPI memory hot-add, the kernel should speak out this is > > a BIOS bug and reject the new memory. For hot-removal, for simplicity > > just assume the kernel cannot continue to work normally, and just BUG(). > > > > Add a new attribute CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED to indicate the > > platform doesn't support ACPI memory hotplug, so that kernel can handle > > ACPI memory hotplug events for such platform. > > > > In acpi_memory_device_{add|remove}(), add early check against this > > attribute and handle accordingly if it is set. > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > drivers/acpi/acpi_memhotplug.c | 23 +++++++++++++++++++++++ > > include/linux/cc_platform.h | 10 ++++++++++ > > 2 files changed, 33 insertions(+) > > > > diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c > > index 24f662d8bd39..94d6354ea453 100644 > > --- a/drivers/acpi/acpi_memhotplug.c > > +++ b/drivers/acpi/acpi_memhotplug.c > > @@ -15,6 +15,7 @@ > > #include <linux/acpi.h> > > #include <linux/memory.h> > > #include <linux/memory_hotplug.h> > > +#include <linux/cc_platform.h> > > > > #include "internal.h" > > > > @@ -291,6 +292,17 @@ static int acpi_memory_device_add(struct acpi_device *device, > > if (!device) > > return -EINVAL; > > > > + /* > > + * 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 memory device. > > + */ > > + if (cc_platform_has(CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED)) { > > Same comment as for the acpi_processor driver: this will affect the > initialization too and it would be cleaner to reset the > .hotplug.enabled flag of the scan handler. > > Hi Rafael, Thanks for review. The same to the ACPI CPU hotplug handling, this is illegal also during kernel boot. If we just want to disable, then perhaps something like below? --- a/drivers/acpi/acpi_memhotplug.c +++ b/drivers/acpi/acpi_memhotplug.c @@ -366,6 +366,9 @@ static bool __initdata acpi_no_memhotplug; void __init acpi_memory_hotplug_init(void) { + if (cc_platform_has(CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED)) + acpi_no_memhotplug = true; + if (acpi_no_memhotplug) { memory_device_handler.attach = NULL; acpi_scan_add_handler(&memory_device_handler);
On Wed, 22 Jun 2022 13:45:01 +0200 "Rafael J. Wysocki" <rafael@kernel.org> wrote: > On Wed, Jun 22, 2022 at 1:16 PM Kai Huang <kai.huang@intel.com> wrote: > > > > Platforms with confidential computing technology may not support ACPI > > memory 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 memory hotplug event, it is likely a > > BIOS bug. For ACPI memory hot-add, the kernel should speak out this is > > a BIOS bug and reject the new memory. For hot-removal, for simplicity > > just assume the kernel cannot continue to work normally, and just BUG(). > > > > Add a new attribute CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED to indicate the > > platform doesn't support ACPI memory hotplug, so that kernel can handle > > ACPI memory hotplug events for such platform. > > > > In acpi_memory_device_{add|remove}(), add early check against this > > attribute and handle accordingly if it is set. > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > drivers/acpi/acpi_memhotplug.c | 23 +++++++++++++++++++++++ > > include/linux/cc_platform.h | 10 ++++++++++ > > 2 files changed, 33 insertions(+) > > > > diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c > > index 24f662d8bd39..94d6354ea453 100644 > > --- a/drivers/acpi/acpi_memhotplug.c > > +++ b/drivers/acpi/acpi_memhotplug.c > > @@ -15,6 +15,7 @@ > > #include <linux/acpi.h> > > #include <linux/memory.h> > > #include <linux/memory_hotplug.h> > > +#include <linux/cc_platform.h> > > > > #include "internal.h" > > > > @@ -291,6 +292,17 @@ static int acpi_memory_device_add(struct acpi_device *device, > > if (!device) > > return -EINVAL; > > > > + /* > > + * 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 memory device. > > + */ > > + if (cc_platform_has(CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED)) { > > Same comment as for the acpi_processor driver: this will affect the > initialization too and it would be cleaner to reset the > .hotplug.enabled flag of the scan handler. with QEMU, it is likely broken when memory is added as '-device pc-dimm' on CLI since it's advertised only as device node in DSDT. > > > + dev_err(&device->dev, "[BIOS bug]: Platform doesn't support ACPI memory hotplug. New memory device ignored.\n"); > > + return -EINVAL; > > + } > > + > > mem_device = kzalloc(sizeof(struct acpi_memory_device), GFP_KERNEL); > > if (!mem_device) > > return -ENOMEM; > > @@ -334,6 +346,17 @@ static void acpi_memory_device_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 is 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 memory hotplug. BUG().\n"); > > + BUG(); > > + } > > + > > mem_device = acpi_driver_data(device); > > acpi_memory_remove_memory(mem_device); > > acpi_memory_device_free(mem_device); > > diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h > > index 9ce9256facc8..b831c24bd7f6 100644 > > --- a/include/linux/cc_platform.h > > +++ b/include/linux/cc_platform.h > > @@ -93,6 +93,16 @@ enum cc_attr { > > * Examples include TDX platform. > > */ > > CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED, > > + > > + /** > > + * @CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED: ACPI memory hotplug is > > + * not supported. > > + * > > + * The platform/os does not support ACPI memory hotplug. > > + * > > + * Examples include TDX platform. > > + */ > > + CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED, > > }; > > > > #ifdef CONFIG_ARCH_HAS_CC_PLATFORM > > -- > > 2.36.1 > > >
On Thu, Jun 23, 2022 at 2:09 AM Kai Huang <kai.huang@intel.com> wrote: > > On Wed, 2022-06-22 at 13:45 +0200, Rafael J. Wysocki wrote: > > On Wed, Jun 22, 2022 at 1:16 PM Kai Huang <kai.huang@intel.com> wrote: > > > > > > Platforms with confidential computing technology may not support ACPI > > > memory 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 memory hotplug event, it is likely a > > > BIOS bug. For ACPI memory hot-add, the kernel should speak out this is > > > a BIOS bug and reject the new memory. For hot-removal, for simplicity > > > just assume the kernel cannot continue to work normally, and just BUG(). > > > > > > Add a new attribute CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED to indicate the > > > platform doesn't support ACPI memory hotplug, so that kernel can handle > > > ACPI memory hotplug events for such platform. > > > > > > In acpi_memory_device_{add|remove}(), add early check against this > > > attribute and handle accordingly if it is set. > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > --- > > > drivers/acpi/acpi_memhotplug.c | 23 +++++++++++++++++++++++ > > > include/linux/cc_platform.h | 10 ++++++++++ > > > 2 files changed, 33 insertions(+) > > > > > > diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c > > > index 24f662d8bd39..94d6354ea453 100644 > > > --- a/drivers/acpi/acpi_memhotplug.c > > > +++ b/drivers/acpi/acpi_memhotplug.c > > > @@ -15,6 +15,7 @@ > > > #include <linux/acpi.h> > > > #include <linux/memory.h> > > > #include <linux/memory_hotplug.h> > > > +#include <linux/cc_platform.h> > > > > > > #include "internal.h" > > > > > > @@ -291,6 +292,17 @@ static int acpi_memory_device_add(struct acpi_device *device, > > > if (!device) > > > return -EINVAL; > > > > > > + /* > > > + * 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 memory device. > > > + */ > > > + if (cc_platform_has(CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED)) { > > > > Same comment as for the acpi_processor driver: this will affect the > > initialization too and it would be cleaner to reset the > > .hotplug.enabled flag of the scan handler. > > > > > > Hi Rafael, > > Thanks for review. The same to the ACPI CPU hotplug handling, this is illegal > also during kernel boot. What do you mean? Is it not correct to enumerate any memory device through ACPI at all? > If we just want to disable, then perhaps something like below? > > --- a/drivers/acpi/acpi_memhotplug.c > +++ b/drivers/acpi/acpi_memhotplug.c > @@ -366,6 +366,9 @@ static bool __initdata acpi_no_memhotplug; > > void __init acpi_memory_hotplug_init(void) > { > + if (cc_platform_has(CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED)) > + acpi_no_memhotplug = true; > + This looks fine to me if the above is the case, but you need to modify the changelog to match. > if (acpi_no_memhotplug) { > memory_device_handler.attach = NULL; > acpi_scan_add_handler(&memory_device_handler); > > > --
On Tue, 2022-06-28 at 14:01 +0200, Igor Mammedov wrote: > On Wed, 22 Jun 2022 13:45:01 +0200 > "Rafael J. Wysocki" <rafael@kernel.org> wrote: > > > On Wed, Jun 22, 2022 at 1:16 PM Kai Huang <kai.huang@intel.com> wrote: > > > > > > Platforms with confidential computing technology may not support ACPI > > > memory 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 memory hotplug event, it is likely a > > > BIOS bug. For ACPI memory hot-add, the kernel should speak out this is > > > a BIOS bug and reject the new memory. For hot-removal, for simplicity > > > just assume the kernel cannot continue to work normally, and just BUG(). > > > > > > Add a new attribute CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED to indicate the > > > platform doesn't support ACPI memory hotplug, so that kernel can handle > > > ACPI memory hotplug events for such platform. > > > > > > In acpi_memory_device_{add|remove}(), add early check against this > > > attribute and handle accordingly if it is set. > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > --- > > > drivers/acpi/acpi_memhotplug.c | 23 +++++++++++++++++++++++ > > > include/linux/cc_platform.h | 10 ++++++++++ > > > 2 files changed, 33 insertions(+) > > > > > > diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c > > > index 24f662d8bd39..94d6354ea453 100644 > > > --- a/drivers/acpi/acpi_memhotplug.c > > > +++ b/drivers/acpi/acpi_memhotplug.c > > > @@ -15,6 +15,7 @@ > > > #include <linux/acpi.h> > > > #include <linux/memory.h> > > > #include <linux/memory_hotplug.h> > > > +#include <linux/cc_platform.h> > > > > > > #include "internal.h" > > > > > > @@ -291,6 +292,17 @@ static int acpi_memory_device_add(struct acpi_device *device, > > > if (!device) > > > return -EINVAL; > > > > > > + /* > > > + * 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 memory device. > > > + */ > > > + if (cc_platform_has(c)) { > > > > Same comment as for the acpi_processor driver: this will affect the > > initialization too and it would be cleaner to reset the > > .hotplug.enabled flag of the scan handler. > > with QEMU, it is likely broken when memory is added as > '-device pc-dimm' > on CLI since it's advertised only as device node in DSDT. > > Hi Rafael, Igor, On my test machine, the acpi_memory_device_add() is not called for system memory. It probably because my machine doesn't have memory device in ACPI. I don't know whether we can have any memory device in ACPI if such memory is present during boot? Any comments here? And CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED is only true on TDX bare-metal system, but cannot be true in Qemu guest. But yes if this flag ever becomes true in guest, then I think we may have problem here. I will do more study around ACPI. Thanks for comments!
On Wed, 29 Jun 2022 11:49:14 +1200 Kai Huang <kai.huang@intel.com> wrote: > On Tue, 2022-06-28 at 14:01 +0200, Igor Mammedov wrote: > > On Wed, 22 Jun 2022 13:45:01 +0200 > > "Rafael J. Wysocki" <rafael@kernel.org> wrote: > > > > > On Wed, Jun 22, 2022 at 1:16 PM Kai Huang <kai.huang@intel.com> wrote: > > > > > > > > Platforms with confidential computing technology may not support ACPI > > > > memory 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 memory hotplug event, it is likely a > > > > BIOS bug. For ACPI memory hot-add, the kernel should speak out this is > > > > a BIOS bug and reject the new memory. For hot-removal, for simplicity > > > > just assume the kernel cannot continue to work normally, and just BUG(). > > > > > > > > Add a new attribute CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED to indicate the > > > > platform doesn't support ACPI memory hotplug, so that kernel can handle > > > > ACPI memory hotplug events for such platform. > > > > > > > > In acpi_memory_device_{add|remove}(), add early check against this > > > > attribute and handle accordingly if it is set. > > > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > --- > > > > drivers/acpi/acpi_memhotplug.c | 23 +++++++++++++++++++++++ > > > > include/linux/cc_platform.h | 10 ++++++++++ > > > > 2 files changed, 33 insertions(+) > > > > > > > > diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c > > > > index 24f662d8bd39..94d6354ea453 100644 > > > > --- a/drivers/acpi/acpi_memhotplug.c > > > > +++ b/drivers/acpi/acpi_memhotplug.c > > > > @@ -15,6 +15,7 @@ > > > > #include <linux/acpi.h> > > > > #include <linux/memory.h> > > > > #include <linux/memory_hotplug.h> > > > > +#include <linux/cc_platform.h> > > > > > > > > #include "internal.h" > > > > > > > > @@ -291,6 +292,17 @@ static int acpi_memory_device_add(struct acpi_device *device, > > > > if (!device) > > > > return -EINVAL; > > > > > > > > + /* > > > > + * 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 memory device. > > > > + */ > > > > + if (cc_platform_has(c)) { > > > > > > Same comment as for the acpi_processor driver: this will affect the > > > initialization too and it would be cleaner to reset the > > > .hotplug.enabled flag of the scan handler. > > > > with QEMU, it is likely broken when memory is added as > > '-device pc-dimm' > > on CLI since it's advertised only as device node in DSDT. > > > > > > Hi Rafael, Igor, > > On my test machine, the acpi_memory_device_add() is not called for system > memory. It probably because my machine doesn't have memory device in ACPI. > > I don't know whether we can have any memory device in ACPI if such memory is > present during boot? Any comments here? I don't see anything in ACPI spec that forbids memory device being present at boot. Such memory may also be present in E820, but in QEMU is not done as linux used to online all E820 memory as normal which breaks hotplug. And I don't know if it still true. Also NVDIMMs also use memory device, so they may be affected by this patch as well. > > And CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED is only true on TDX bare-metal system, > but cannot be true in Qemu guest. But yes if this flag ever becomes true in that's temporary, once TDX support lands in KVM/QEMU, this patch will silently break usecase. > guest, then I think we may have problem here. I will do more study around ACPI. > Thanks for comments! >
On Wed, 2022-06-29 at 10:48 +0200, Igor Mammedov wrote: > > Hi Rafael, Igor, > > > > On my test machine, the acpi_memory_device_add() is not called for system > > memory. It probably because my machine doesn't have memory device in ACPI. > > > > I don't know whether we can have any memory device in ACPI if such memory is > > present during boot? Any comments here? > > I don't see anything in ACPI spec that forbids memory device being present at > boot. > Such memory may also be present in E820, but in QEMU is not done as linux used > to > online all E820 memory as normal which breaks hotplug. And I don't know if it > still true. > > Also NVDIMMs also use memory device, so they may be affected by this patch as > well. AFAICT NVDIMM uses different device ID so won't be impacted. But right there's no specification around "whether firmware will create ACPI memory device for boot-time present memory", so I guess we need to treat it is possible. So I agree having the check at the beginning of acpi_memory_device_add() looks incorrect. Also as Christoph commented I'll give up introducing new CC attribute. > > > > > And CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED is only true on TDX bare-metal > > system, > > but cannot be true in Qemu guest. But yes if this flag ever becomes true in > > that's temporary, once TDX support lands in KVM/QEMU, this patch will silently > break usecase. I don't think so. KVM/Qemu won't expose TDX to guest, so this code won't be true in guest.
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c index 24f662d8bd39..94d6354ea453 100644 --- a/drivers/acpi/acpi_memhotplug.c +++ b/drivers/acpi/acpi_memhotplug.c @@ -15,6 +15,7 @@ #include <linux/acpi.h> #include <linux/memory.h> #include <linux/memory_hotplug.h> +#include <linux/cc_platform.h> #include "internal.h" @@ -291,6 +292,17 @@ static int acpi_memory_device_add(struct acpi_device *device, if (!device) return -EINVAL; + /* + * 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 memory device. + */ + if (cc_platform_has(CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED)) { + dev_err(&device->dev, "[BIOS bug]: Platform doesn't support ACPI memory hotplug. New memory device ignored.\n"); + return -EINVAL; + } + mem_device = kzalloc(sizeof(struct acpi_memory_device), GFP_KERNEL); if (!mem_device) return -ENOMEM; @@ -334,6 +346,17 @@ static void acpi_memory_device_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 is 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 memory hotplug. BUG().\n"); + BUG(); + } + mem_device = acpi_driver_data(device); acpi_memory_remove_memory(mem_device); acpi_memory_device_free(mem_device); diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h index 9ce9256facc8..b831c24bd7f6 100644 --- a/include/linux/cc_platform.h +++ b/include/linux/cc_platform.h @@ -93,6 +93,16 @@ enum cc_attr { * Examples include TDX platform. */ CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED, + + /** + * @CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED: ACPI memory hotplug is + * not supported. + * + * The platform/os does not support ACPI memory hotplug. + * + * Examples include TDX platform. + */ + CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED, }; #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
Platforms with confidential computing technology may not support ACPI memory 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 memory hotplug event, it is likely a BIOS bug. For ACPI memory hot-add, the kernel should speak out this is a BIOS bug and reject the new memory. For hot-removal, for simplicity just assume the kernel cannot continue to work normally, and just BUG(). Add a new attribute CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED to indicate the platform doesn't support ACPI memory hotplug, so that kernel can handle ACPI memory hotplug events for such platform. In acpi_memory_device_{add|remove}(), add early check against this attribute and handle accordingly if it is set. Signed-off-by: Kai Huang <kai.huang@intel.com> --- drivers/acpi/acpi_memhotplug.c | 23 +++++++++++++++++++++++ include/linux/cc_platform.h | 10 ++++++++++ 2 files changed, 33 insertions(+)