diff mbox

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

Message ID 1470984850-66891-3-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xiao Guangrong Aug. 12, 2016, 6:54 a.m. UTC
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(-)

Comments

Igor Mammedov Sept. 21, 2016, 11:48 a.m. UTC | #1
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
Xiao Guangrong Sept. 22, 2016, 2:43 a.m. UTC | #2
On 09/21/2016 07:48 PM, Igor Mammedov wrote:
> 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/

Will fix. Thank you for pointing it out.

>> +     * 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?
>

Not needed.

As the NFIT table is created only if there is nvdimm device already plugged
that means QEMU must have available ram-slots.
diff mbox

Patch

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
+     * slot.
+     */
+    if (ram_slots) {
+        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