[Qemu-devel] [PATCH 0/4] nvdimm: clean up

[Qemu-devel] [PATCH 0/4] nvdimm: clean up

[Wei Yang]

Here are 4 cleanup patches for nvdimm.

The first three are trivial clean on argument/variable.
The last one move setup place of the FIT table.

Wei Yang (4):
  nvdimm: fix typo in nvdimm_build_nvdimm_devices argument
  nvdimm: use *function* directly instead of allocating it again
  nvdimm: use NVDIMM_ACPI_IO_LEN for the proper IO size
  nvdimm: build FIT in nvdimm_build_acpi

 hw/acpi/nvdimm.c        | 14 +++++---------
 hw/i386/pc.c            |  5 -----
 include/hw/mem/nvdimm.h |  1 -
 3 files changed, 5 insertions(+), 15 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/4] nvdimm: fix typo in nvdimm_build_nvdimm_devices argument

[Wei Yang]

>From dsm_dma_arrea to dsm_dma_area.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/nvdimm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index e53b2cb681..39af8cdba8 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1260,7 +1260,7 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
 }
 
 static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
-                              BIOSLinker *linker, GArray *dsm_dma_arrea,
+                              BIOSLinker *linker, GArray *dsm_dma_area,
                               uint32_t ram_slots)
 {
     Aml *ssdt, *sb_scope, *dev;
@@ -1307,7 +1307,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
                                                NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker,
-                             NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
+                             NVDIMM_DSM_MEM_FILE, dsm_dma_area,
                              sizeof(NvdimmDsmIn), false /* high memory */);
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
-- 
2.19.1

[Qemu-devel] [PATCH 2/4] nvdimm: use *function* directly instead of allocating it again

[Wei Yang]

At the beginning or nvdimm_build_common_dsm(), variable *function* is
already allocated for Arg2.

This patch reuse variable *function* instead of allocating it again.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/nvdimm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 39af8cdba8..e63a1ef15d 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1086,7 +1086,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
      */
     aml_append(method, aml_store(handle, aml_name(NVDIMM_DSM_HANDLE)));
     aml_append(method, aml_store(aml_arg(1), aml_name(NVDIMM_DSM_REVISION)));
-    aml_append(method, aml_store(aml_arg(2), aml_name(NVDIMM_DSM_FUNCTION)));
+    aml_append(method, aml_store(function, aml_name(NVDIMM_DSM_FUNCTION)));
 
     /*
      * The fourth parameter (Arg3) of _DSM is a package which contains
-- 
2.19.1

[Qemu-devel] [PATCH 3/4] nvdimm: use NVDIMM_ACPI_IO_LEN for the proper IO size

[Wei Yang]

The IO range is defined to 4 bytes with NVDIMM_ACPI_IO_LEN, so it is
more proper to use this macro instead of calculating it by sizeof.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/nvdimm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index e63a1ef15d..2457c1aa44 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -992,7 +992,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
     field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
                       AML_PRESERVE);
     aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
-               sizeof(uint32_t) * BITS_PER_BYTE));
+               NVDIMM_ACPI_IO_LEN * BITS_PER_BYTE));
     aml_append(method, field);
 
     /*
-- 
2.19.1

[Qemu-devel] [PATCH 4/4] nvdimm: build FIT in nvdimm_build_acpi

[Wei Yang]

Currently we initialize nvdimm device like below:

    device_set_realized
        dc->realize
	nvdimm_plug
	    nvdimm_build_fit_buffer
    acpi_build
        nvdimm_build_acpi

This shows nvdimm's acpi stuff is prepared in two places:

    * device plug stage
    * acpi_build stage

It would be more proper to build fit buffer in nvdimm_build_acpi, which
consolidate all acpi related initialization together.

This patch moves FIT build in nvdimm_build_acpi.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/nvdimm.c        | 6 +-----
 hw/i386/pc.c            | 5 -----
 include/hw/mem/nvdimm.h | 1 -
 3 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 2457c1aa44..6fc0c65ee0 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -425,11 +425,6 @@ static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
     fit_buf->dirty = true;
 }
 
-void nvdimm_plug(AcpiNVDIMMState *state)
-{
-    nvdimm_build_fit_buffer(state);
-}
-
 static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
                               GArray *table_data, BIOSLinker *linker)
 {
@@ -1338,6 +1333,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
         return;
     }
 
+    nvdimm_build_fit_buffer(state);
     nvdimm_build_nfit(state, table_offsets, table_data, linker);
     g_slist_free(device_list);
 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 64d9e756fa..686b610c50 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2099,17 +2099,12 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
 {
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
     pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
     if (local_err) {
         goto out;
     }
 
-    if (is_nvdimm) {
-        nvdimm_plug(&pcms->acpi_nvdimm_state);
-    }
-
     hotplug_handler_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
 out:
     error_propagate(errp, local_err);
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index c5c9b3c7f8..30869b4834 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -148,6 +148,5 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                        BIOSLinker *linker, AcpiNVDIMMState *state,
                        uint32_t ram_slots);
-void nvdimm_plug(AcpiNVDIMMState *state);
 void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
 #endif
-- 
2.19.1

Re: [Qemu-devel] [PATCH 1/4] nvdimm: fix typo in nvdimm_build_nvdimm_devices argument

[Igor Mammedov]

On Wed, 27 Feb 2019 15:50:58 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> From dsm_dma_arrea to dsm_dma_area.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

> ---
>  hw/acpi/nvdimm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index e53b2cb681..39af8cdba8 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1260,7 +1260,7 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  }
>  
>  static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> -                              BIOSLinker *linker, GArray *dsm_dma_arrea,
> +                              BIOSLinker *linker, GArray *dsm_dma_area,
>                                uint32_t ram_slots)
>  {
>      Aml *ssdt, *sb_scope, *dev;
> @@ -1307,7 +1307,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>                                                 NVDIMM_ACPI_MEM_ADDR);
>  
>      bios_linker_loader_alloc(linker,
> -                             NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
> +                             NVDIMM_DSM_MEM_FILE, dsm_dma_area,
>                               sizeof(NvdimmDsmIn), false /* high memory */);
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),

Re: [Qemu-devel] [PATCH 2/4] nvdimm: use *function* directly instead of allocating it again

[Igor Mammedov]

On Wed, 27 Feb 2019 15:50:59 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> At the beginning or nvdimm_build_common_dsm(), variable *function* is
> already allocated for Arg2.
> 
> This patch reuse variable *function* instead of allocating it again.

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

> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 39af8cdba8..e63a1ef15d 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1086,7 +1086,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
>       */
>      aml_append(method, aml_store(handle, aml_name(NVDIMM_DSM_HANDLE)));
>      aml_append(method, aml_store(aml_arg(1), aml_name(NVDIMM_DSM_REVISION)));
> -    aml_append(method, aml_store(aml_arg(2), aml_name(NVDIMM_DSM_FUNCTION)));
> +    aml_append(method, aml_store(function, aml_name(NVDIMM_DSM_FUNCTION)));
>  
>      /*
>       * The fourth parameter (Arg3) of _DSM is a package which contains

Re: [Qemu-devel] [PATCH 3/4] nvdimm: use NVDIMM_ACPI_IO_LEN for the proper IO size

[Igor Mammedov]

On Wed, 27 Feb 2019 15:51:00 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> The IO range is defined to 4 bytes with NVDIMM_ACPI_IO_LEN, so it is
> more proper to use this macro instead of calculating it by sizeof.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

> ---
>  hw/acpi/nvdimm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index e63a1ef15d..2457c1aa44 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -992,7 +992,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
>      field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
>                        AML_PRESERVE);
>      aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> -               sizeof(uint32_t) * BITS_PER_BYTE));
> +               NVDIMM_ACPI_IO_LEN * BITS_PER_BYTE));
>      aml_append(method, field);
>  
>      /*

Re: [Qemu-devel] [PATCH 4/4] nvdimm: build FIT in nvdimm_build_acpi

[Igor Mammedov]

On Wed, 27 Feb 2019 15:51:01 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> Currently we initialize nvdimm device like below:
> 
>     device_set_realized
>         dc->realize
> 	nvdimm_plug
> 	    nvdimm_build_fit_buffer
>     acpi_build
>         nvdimm_build_acpi
> 
> This shows nvdimm's acpi stuff is prepared in two places:
> 
>     * device plug stage
>     * acpi_build stage
> 
> It would be more proper to build fit buffer in nvdimm_build_acpi, which
> consolidate all acpi related initialization together.
> 
> This patch moves FIT build in nvdimm_build_acpi.

Does hotplug work after this change?

> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c        | 6 +-----
>  hw/i386/pc.c            | 5 -----
>  include/hw/mem/nvdimm.h | 1 -
>  3 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 2457c1aa44..6fc0c65ee0 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -425,11 +425,6 @@ static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
>      fit_buf->dirty = true;
>  }
>  
> -void nvdimm_plug(AcpiNVDIMMState *state)
> -{
> -    nvdimm_build_fit_buffer(state);
> -}
> -
>  static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
>                                GArray *table_data, BIOSLinker *linker)
>  {
> @@ -1338,6 +1333,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>          return;
>      }
>  
> +    nvdimm_build_fit_buffer(state);
>      nvdimm_build_nfit(state, table_offsets, table_data, linker);
>      g_slist_free(device_list);
>  }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 64d9e756fa..686b610c50 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2099,17 +2099,12 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>  {
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
>      pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
>      if (local_err) {
>          goto out;
>      }
>  
> -    if (is_nvdimm) {
> -        nvdimm_plug(&pcms->acpi_nvdimm_state);
> -    }
> -
>      hotplug_handler_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
>  out:
>      error_propagate(errp, local_err);
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index c5c9b3c7f8..30869b4834 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -148,6 +148,5 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         BIOSLinker *linker, AcpiNVDIMMState *state,
>                         uint32_t ram_slots);
> -void nvdimm_plug(AcpiNVDIMMState *state);
>  void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
>  #endif

Re: [Qemu-devel] [PATCH 4/4] nvdimm: build FIT in nvdimm_build_acpi

[Wei Yang]

On Wed, Mar 06, 2019 at 05:14:06PM +0100, Igor Mammedov wrote:
>On Wed, 27 Feb 2019 15:51:01 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> Currently we initialize nvdimm device like below:
>> 
>>     device_set_realized
>>         dc->realize
>> 	nvdimm_plug
>> 	    nvdimm_build_fit_buffer
>>     acpi_build
>>         nvdimm_build_acpi
>> 
>> This shows nvdimm's acpi stuff is prepared in two places:
>> 
>>     * device plug stage
>>     * acpi_build stage
>> 
>> It would be more proper to build fit buffer in nvdimm_build_acpi, which
>> consolidate all acpi related initialization together.
>> 
>> This patch moves FIT build in nvdimm_build_acpi.
>
>Does hotplug work after this change?
>

Looks no. I missed to test this case.

>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  hw/acpi/nvdimm.c        | 6 +-----
>>  hw/i386/pc.c            | 5 -----
>>  include/hw/mem/nvdimm.h | 1 -
>>  3 files changed, 1 insertion(+), 11 deletions(-)
>> 
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 2457c1aa44..6fc0c65ee0 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -425,11 +425,6 @@ static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
>>      fit_buf->dirty = true;
>>  }
>>  
>> -void nvdimm_plug(AcpiNVDIMMState *state)
>> -{
>> -    nvdimm_build_fit_buffer(state);
>> -}
>> -
>>  static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
>>                                GArray *table_data, BIOSLinker *linker)
>>  {
>> @@ -1338,6 +1333,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>>          return;
>>      }
>>  
>> +    nvdimm_build_fit_buffer(state);
>>      nvdimm_build_nfit(state, table_offsets, table_data, linker);
>>      g_slist_free(device_list);
>>  }
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 64d9e756fa..686b610c50 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2099,17 +2099,12 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>>  {
>>      Error *local_err = NULL;
>>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> -    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>  
>>      pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
>>      if (local_err) {
>>          goto out;
>>      }
>>  
>> -    if (is_nvdimm) {
>> -        nvdimm_plug(&pcms->acpi_nvdimm_state);
>> -    }
>> -
>>      hotplug_handler_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
>>  out:
>>      error_propagate(errp, local_err);
>> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
>> index c5c9b3c7f8..30869b4834 100644
>> --- a/include/hw/mem/nvdimm.h
>> +++ b/include/hw/mem/nvdimm.h
>> @@ -148,6 +148,5 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>>                         BIOSLinker *linker, AcpiNVDIMMState *state,
>>                         uint32_t ram_slots);
>> -void nvdimm_plug(AcpiNVDIMMState *state);
>>  void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
>>  #endif

-- 
Wei Yang
Help you, Help me