Re: [PATCH v12 3/4] hw/i386: hook sp-mem into the pc machine plug path

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 16 Jun 2026 17:08:07 +0800
fanhuang <FangSheng.Huang@amd.com> wrote:

> Add the pc machine hookup for TYPE_SP_MEM so each sp-mem instance is
> placed by the memory-device framework and reported to the guest as
> E820_SOFT_RESERVED.
> 
> Signed-off-by: FangSheng Huang <FangSheng.Huang@amd.com>

see below comment below wrt testing but otherwise:

Reviewed-by: Igor Mammedov <imammedo@redhat.com>


> ---
>  hw/i386/e820_memory_layout.h | 11 ++++++-----
>  hw/i386/pc.c                 | 36 ++++++++++++++++++++++++++++++++++++
>  hw/i386/Kconfig              |  2 ++
>  3 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/e820_memory_layout.h b/hw/i386/e820_memory_layout.h
> index b50acfa201..6ef169db9c 100644
> --- a/hw/i386/e820_memory_layout.h
> +++ b/hw/i386/e820_memory_layout.h
> @@ -10,11 +10,12 @@
>  #define HW_I386_E820_MEMORY_LAYOUT_H
>  
>  /* e820 types */
> -#define E820_RAM        1
> -#define E820_RESERVED   2
> -#define E820_ACPI       3
> -#define E820_NVS        4
> -#define E820_UNUSABLE   5
> +#define E820_RAM           1
> +#define E820_RESERVED      2
> +#define E820_ACPI          3
> +#define E820_NVS           4
> +#define E820_UNUSABLE      5
> +#define E820_SOFT_RESERVED 0xefffffff
>  
>  struct e820_entry {
>      uint64_t address;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7b6ad97e5a..ec3459389b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -63,6 +63,7 @@
>  #include "hw/i386/kvm/xen_gnttab.h"
>  #include "hw/i386/kvm/xen_xenstore.h"
>  #include "hw/mem/memory-device.h"
> +#include "hw/mem/sp-mem.h"
>  #include "e820_memory_layout.h"
>  #include "trace.h"
>  #include "sev.h"
> @@ -1283,11 +1284,43 @@ static void pc_hv_balloon_plug(HotplugHandler *hotplug_dev,
>      memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>  }
>  
> +static void pc_sp_mem_pre_plug(HotplugHandler *hotplug_dev,
> +                               DeviceState *dev, Error **errp)
> +{
> +    MachineState *ms = MACHINE(hotplug_dev);
> +    SpMemDevice *spm = SP_MEM(dev);
> +
> +    if (ms->numa_state && spm->node >= ms->numa_state->num_nodes) {
> +        error_setg(errp,
> +                   "'node' property value %" PRIu32
> +                   " exceeds the number of NUMA nodes (%d)",
> +                   spm->node, ms->numa_state->num_nodes);
> +        return;
> +    }
> +    memory_device_pre_plug(MEMORY_DEVICE(dev), ms, errp);
> +}
> +
> +static void pc_sp_mem_plug(HotplugHandler *hotplug_dev,
> +                               DeviceState *dev, Error **errp)
> +{
> +    SpMemDevice *spm = SP_MEM(dev);
> +    MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(MEMORY_DEVICE(dev));
> +    uint64_t addr, size;
> +
> +    memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
> +
> +    addr = mdc->get_addr(MEMORY_DEVICE(dev));
> +    size = memory_region_size(host_memory_backend_get_memory(spm->hostmem));
> +    e820_add_entry(addr, size, E820_SOFT_RESERVED);

We haven't been testing e820 (lack of coverage mostly historical due to
it being mostly static not touched code).
But I think we should add tests for it. (something similar to bios-tables-test)
 
1st add clean slate test before your change, and then build/add on top to
make sure we do not regress, whatever existed before.

> +}
> +
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                            DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          pc_memory_pre_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SP_MEM)) {
> +        pc_sp_mem_pre_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          x86_cpu_pre_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> @@ -1324,6 +1357,8 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          pc_memory_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SP_MEM)) {
> +        pc_sp_mem_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          x86_cpu_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> @@ -1368,6 +1403,7 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
>                                               DeviceState *dev)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_SP_MEM) ||
>          object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
>          object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI) ||
>          object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI) ||
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index 12473acaa7..e27d8816e5 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -84,6 +84,7 @@ config I440FX
>      select PCI_I440FX
>      select PIIX
>      select DIMM
> +    select SP_MEM
>      select SMBIOS
>      select SMBIOS_LEGACY
>      select FW_CFG_DMA
> @@ -113,6 +114,7 @@ config Q35
>      select LPC_ICH9
>      select AHCI_ICH9
>      select DIMM
> +    select SP_MEM
>      select SMBIOS
>      select FW_CFG_DMA
>