Re: [PATCH v2 2/8] nvdimm acpi: prebuild nvdimm devices for available slots

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 12 Aug 2016 14:54:04 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> 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
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  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 5454c0f..0e2b9f0 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -886,12 +886,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;
>  
> @@ -912,9 +911,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, *field;
>      int mem_addr_offset, nvdimm_ssdt;
> @@ -1003,7 +1002,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);
> @@ -1028,17 +1027,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 has available
s/has/is/
> +     * slot.
> +     */
> +    if (ram_slots) {
another question:
 Is NFIT table generated above sufficient without below nvdim SSDT?

maybe you should put
  if (!ram_slots) {
    return;
  }
at the function start?

> +        nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
> +                          ram_slots);
>      }
> -    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 a26a4bb..b1d0ced 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2712,7 +2712,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