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

[Igor Mammedov]

On Wed, 29 Jun 2022 11:49:14 +1200
Kai Huang <kai.huang@xxxxxxxxx> 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@xxxxxxxxxx> 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
> > > > 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@xxxxxxxxx>
> > > > ---
> > > >  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!
>