Re: [Qemu-devel] [PATCH v3 1/4] nvdimm acpi: prebuild nvdimm devices for available slots

[Igor Mammedov]

On Sat, 29 Oct 2016 00:35:37 +0800
Xiao Guangrong <address@hidden> wrote:

> For each NVDIMM present or intended to be supported by platform,
> platform firmware also exposes an ACPI Namespace Device under
> the root device
> 
> So it builds nvdimm devices for all slots to support vNVDIMM hotplug
> 
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Xiao Guangrong <address@hidden>
> ---
>  hw/acpi/nvdimm.c        | 41 ++++++++++++++++++++++++-----------------
>  hw/i386/acpi-build.c    |  2 +-
>  include/hw/mem/nvdimm.h |  3 ++-
>  3 files changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index bb896c9..b8a2e62 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -961,12 +961,11 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t 
> handle)
>      aml_append(dev, method);
>  }
>  
> -static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
> +static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  {
> -    for (; device_list; device_list = device_list->next) {
> -        DeviceState *dev = device_list->data;
> -        int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> -                                           NULL);
> +    uint32_t slot;
> +
> +    for (slot = 0; slot < ram_slots; slot++) {
>          uint32_t handle = nvdimm_slot_to_handle(slot);
>          Aml *nvdimm_dev;
>  
> @@ -987,9 +986,9 @@ static void nvdimm_build_nvdimm_devices(GSList 
> *device_list, Aml *root_dev)
>      }
>  }
>  
> -static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> -                              GArray *table_data, BIOSLinker *linker,
> -                              GArray *dsm_dma_arrea)
> +static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> +                              BIOSLinker *linker, GArray *dsm_dma_arrea,
> +                              uint32_t ram_slots)
>  {
>      Aml *ssdt, *sb_scope, *dev;
>      int mem_addr_offset, nvdimm_ssdt;
> @@ -1021,7 +1020,7 @@ static void nvdimm_build_ssdt(GSList *device_list, 
> GArray *table_offsets,
>      /* 0 is reserved for root device. */
>      nvdimm_build_device_dsm(dev, 0);
>  
> -    nvdimm_build_nvdimm_devices(device_list, dev);
> +    nvdimm_build_nvdimm_devices(dev, ram_slots);
>  
>      aml_append(sb_scope, dev);
>      aml_append(ssdt, sb_scope);
> @@ -1046,17 +1045,25 @@ static void nvdimm_build_ssdt(GSList *device_list, 
> GArray *table_offsets,
>  }
>  
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> -                       BIOSLinker *linker, GArray *dsm_dma_arrea)
> +                       BIOSLinker *linker, GArray *dsm_dma_arrea,
> +                       uint32_t ram_slots)
>  {
>      GSList *device_list;
>  
> -    /* no NVDIMM device is plugged. */
>      device_list = nvdimm_get_plugged_device_list();
> -    if (!device_list) {
> -        return;
> +
> +    /* NVDIMM device is plugged. */
> +    if (device_list) {
> +        nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> +        g_slist_free(device_list);
> +    }
> +
> +    /*
> +     * NVDIMM device is allowed to be plugged only if there is available
> +     * slot.
> +     */
> +    if (ram_slots) {
put this check before assigning to device_list
and bail out early if ram slots 0 as there is no point in proceeding further

> +        nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
> +                          ram_slots);
>      }

after that you'd just keep the hunk below unchanged,
which would simplify the patch

> -    nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> -    nvdimm_build_ssdt(device_list, table_offsets, table_data, linker,
> -                      dsm_dma_arrea);
> -    g_slist_free(device_list);
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e999654..6ae4769 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
> *machine)
>      }
>      if (pcms->acpi_nvdimm_state.is_enabled) {
>          nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> -                          pcms->acpi_nvdimm_state.dsm_mem);
> +                          pcms->acpi_nvdimm_state.dsm_mem, 
> machine->ram_slots);
>      }
>  
>      /* Add tables supplied by user (if any) */
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 1cfe9e0..63a2b20 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -112,5 +112,6 @@ typedef struct AcpiNVDIMMState AcpiNVDIMMState;
>  void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>                              FWCfgState *fw_cfg, Object *owner);
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> -                       BIOSLinker *linker, GArray *dsm_dma_arrea);
> +                       BIOSLinker *linker, GArray *dsm_dma_arrea,
> +                       uint32_t ram_slots);
>  #endif