[PATCH v2 0/8] nvdimm: hotplug support

[PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
rx queue siz) on pci branch of Michael's git tree and can be found at:
      https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2

Changelog in v2:
   Fixed signed integer overflow pointed out by Stefan Hajnoczi

This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
for example, a new nvdimm device can be plugged as follows:
object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
device_add nvdimm,id=nvdimm3,memdev=mem3

and unplug it as follows:
device_del nvdimm3
object_del mem3

Xiao Guangrong (8):
  acpi nvdimm: fix wrong buffer size returned by DSM method
  nvdimm acpi: prebuild nvdimm devices for available slots
  nvdimm acpi: introduce _FIT
  nvdimm acpi: implement Read FIT function
  pc-dimm: introduce prepare_unplug() callback
  pc: memhp: do not export nvdimm's memory via _CRS
  pc: acpi: memhp: nvdimm hotplug support
  nvdimm docs: add nvdimm Read FIT function

 docs/specs/acpi_mem_hotplug.txt  |   4 +-
 docs/specs/acpi_nvdimm.txt       |  38 +++++++-
 hw/acpi/ich9.c                   |   3 +
 hw/acpi/memory_hotplug.c         |  21 +++--
 hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
 hw/acpi/piix4.c                  |   3 +
 hw/i386/acpi-build.c             |  28 +++++-
 hw/mem/nvdimm.c                  |  12 ++-
 hw/mem/pc-dimm.c                 |   5 +
 include/hw/acpi/memory_hotplug.h |   1 +
 include/hw/acpi/pc-hotplug.h     |   1 +
 include/hw/mem/nvdimm.h          |   6 +-
 include/hw/mem/pc-dimm.h         |   1 +
 13 files changed, 278 insertions(+), 40 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
rx queue siz) on pci branch of Michael's git tree and can be found at:
      https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2

Changelog in v2:
   Fixed signed integer overflow pointed out by Stefan Hajnoczi

This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
for example, a new nvdimm device can be plugged as follows:
object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
device_add nvdimm,id=nvdimm3,memdev=mem3

and unplug it as follows:
device_del nvdimm3
object_del mem3

Xiao Guangrong (8):
  acpi nvdimm: fix wrong buffer size returned by DSM method
  nvdimm acpi: prebuild nvdimm devices for available slots
  nvdimm acpi: introduce _FIT
  nvdimm acpi: implement Read FIT function
  pc-dimm: introduce prepare_unplug() callback
  pc: memhp: do not export nvdimm's memory via _CRS
  pc: acpi: memhp: nvdimm hotplug support
  nvdimm docs: add nvdimm Read FIT function

 docs/specs/acpi_mem_hotplug.txt  |   4 +-
 docs/specs/acpi_nvdimm.txt       |  38 +++++++-
 hw/acpi/ich9.c                   |   3 +
 hw/acpi/memory_hotplug.c         |  21 +++--
 hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
 hw/acpi/piix4.c                  |   3 +
 hw/i386/acpi-build.c             |  28 +++++-
 hw/mem/nvdimm.c                  |  12 ++-
 hw/mem/pc-dimm.c                 |   5 +
 include/hw/acpi/memory_hotplug.h |   1 +
 include/hw/acpi/pc-hotplug.h     |   1 +
 include/hw/mem/nvdimm.h          |   6 +-
 include/hw/mem/pc-dimm.h         |   1 +
 13 files changed, 278 insertions(+), 40 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Xiao Guangrong]

Currently, 'RLEN' is the totally buffer size written by QEMU and it is
ACPI internally used only. The buffer size returned to guest should
not include 'RLEN' itself

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index e486128..5454c0f 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -863,6 +863,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
 
     result_size = aml_local(1);
     aml_append(method, aml_store(aml_name("RLEN"), result_size));
+    /* RLEN is not included in the payload returned to guest. */
+    aml_append(method, aml_subtract(result_size, aml_int(4), result_size));
     aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
                                  result_size));
     aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Xiao Guangrong]

Currently, 'RLEN' is the totally buffer size written by QEMU and it is
ACPI internally used only. The buffer size returned to guest should
not include 'RLEN' itself

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index e486128..5454c0f 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -863,6 +863,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
 
     result_size = aml_local(1);
     aml_append(method, aml_store(aml_name("RLEN"), result_size));
+    /* RLEN is not included in the payload returned to guest. */
+    aml_append(method, aml_subtract(result_size, aml_int(4), result_size));
     aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
                                  result_size));
     aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
-- 
1.8.3.1

Re: [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:03 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Currently, 'RLEN' is the totally buffer size written by QEMU and it is
> ACPI internally used only. The buffer size returned to guest should
> not include 'RLEN' itself
Do you see any errors in guest with this bug present?
It would be nice to put error messages here so that fix could be found
later just by searching git log and qemu-devel for errors user sees
in guest.


> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index e486128..5454c0f 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -863,6 +863,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
>  
>      result_size = aml_local(1);
>      aml_append(method, aml_store(aml_name("RLEN"), result_size));
> +    /* RLEN is not included in the payload returned to guest. */
> +    aml_append(method, aml_subtract(result_size, aml_int(4), result_size));
you can merge above store with subtract like this:
  aml_subtract(aml_name("RLEN"), foo, result_size)

Style nit: try not to use magic numbers,
look at how RLEN is defined earlier, extract it into macro and reuse in both places


>      aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
instead of shiftleft, I'd suggest use here multiply operator and BITS_PER_BYTE
so it would obvious what's going on and rewrite following without intermediate store.

>                                   result_size));
>      aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
aml_create_field(aml_name("ODAT"),
                 aml_int(0),
                 aml_multiply(result_size, aml_int(BITS_PER_BYTE), NULL),
                 "OBUF"))

BTW:
dsm_out_buf_size is more descriptive than result_size

also NCAL later uses Arg6 when method has only 5 arguments which doesn't seem right
instead of arg6 you should make/use local variable 'dsm_out_buf'

As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back.
Currently I can't compile it back which mean it's really broken.

Re: [Qemu-devel] [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:03 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Currently, 'RLEN' is the totally buffer size written by QEMU and it is
> ACPI internally used only. The buffer size returned to guest should
> not include 'RLEN' itself
Do you see any errors in guest with this bug present?
It would be nice to put error messages here so that fix could be found
later just by searching git log and qemu-devel for errors user sees
in guest.


> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index e486128..5454c0f 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -863,6 +863,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
>  
>      result_size = aml_local(1);
>      aml_append(method, aml_store(aml_name("RLEN"), result_size));
> +    /* RLEN is not included in the payload returned to guest. */
> +    aml_append(method, aml_subtract(result_size, aml_int(4), result_size));
you can merge above store with subtract like this:
  aml_subtract(aml_name("RLEN"), foo, result_size)

Style nit: try not to use magic numbers,
look at how RLEN is defined earlier, extract it into macro and reuse in both places


>      aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
instead of shiftleft, I'd suggest use here multiply operator and BITS_PER_BYTE
so it would obvious what's going on and rewrite following without intermediate store.

>                                   result_size));
>      aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
aml_create_field(aml_name("ODAT"),
                 aml_int(0),
                 aml_multiply(result_size, aml_int(BITS_PER_BYTE), NULL),
                 "OBUF"))

BTW:
dsm_out_buf_size is more descriptive than result_size

also NCAL later uses Arg6 when method has only 5 arguments which doesn't seem right
instead of arg6 you should make/use local variable 'dsm_out_buf'

As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back.
Currently I can't compile it back which mean it's really broken.

Re: [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Michael S. Tsirkin]

On Tue, Sep 20, 2016 at 04:07:57PM +0200, Igor Mammedov wrote:
> As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back.
> Currently I can't compile it back which mean it's really broken.

Not always true, disassembler is sometimes producing weird code.
But it's a strong hint.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Michael S. Tsirkin]

On Tue, Sep 20, 2016 at 04:07:57PM +0200, Igor Mammedov wrote:
> As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back.
> Currently I can't compile it back which mean it's really broken.

Not always true, disassembler is sometimes producing weird code.
But it's a strong hint.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Igor Mammedov]

On Tue, 20 Sep 2016 18:14:48 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 20, 2016 at 04:07:57PM +0200, Igor Mammedov wrote:
> > As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back.
> > Currently I can't compile it back which mean it's really broken.  
> 
> Not always true, disassembler is sometimes producing weird code.
> But it's a strong hint.
Unfortunately in current master it's broken as keyword ARG_FOO is used as field name

Re: [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Xiao Guangrong]

On 09/20/2016 10:07 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:03 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> Currently, 'RLEN' is the totally buffer size written by QEMU and it is
>> ACPI internally used only. The buffer size returned to guest should
>> not include 'RLEN' itself
> Do you see any errors in guest with this bug present?
> It would be nice to put error messages here so that fix could be found
> later just by searching git log and qemu-devel for errors user sees
> in guest.
>

No, i did not see any error log in vm. I guess kernel nvdimm driver uses
the buffer based on the 'length' field. I will improve the code to check
whether the buffer size is matched with this field in vm.

>
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>  hw/acpi/nvdimm.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index e486128..5454c0f 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -863,6 +863,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
>>
>>      result_size = aml_local(1);
>>      aml_append(method, aml_store(aml_name("RLEN"), result_size));
>> +    /* RLEN is not included in the payload returned to guest. */
>> +    aml_append(method, aml_subtract(result_size, aml_int(4), result_size));
> you can merge above store with subtract like this:
>   aml_subtract(aml_name("RLEN"), foo, result_size)

Yes, it is better indeed.

>
> Style nit: try not to use magic numbers,
> look at how RLEN is defined earlier, extract it into macro and reuse in both places

Okay.

>
>
>>      aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
> instead of shiftleft, I'd suggest use here multiply operator and BITS_PER_BYTE
> so it would obvious what's going on and rewrite following without intermediate store.
>

I agree. However, qemu does not implement multiply primitive, i'd make a
separate patchset for these cleanups you suggested.

>>                                   result_size));
>>      aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
> aml_create_field(aml_name("ODAT"),
>                  aml_int(0),
>                  aml_multiply(result_size, aml_int(BITS_PER_BYTE), NULL),
>                  "OBUF"))
>
> BTW:
> dsm_out_buf_size is more descriptive than result_size

Yes, indeed.

>
> also NCAL later uses Arg6 when method has only 5 arguments which doesn't seem right
> instead of arg6 you should make/use local variable 'dsm_out_buf'

Sorry, my typo. Will fix.

>
> As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back.
> Currently I can't compile it back which mean it's really broken.
>

Good suggestion, i will try it.

Re: [Qemu-devel] [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Xiao Guangrong]

On 09/20/2016 10:07 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:03 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> Currently, 'RLEN' is the totally buffer size written by QEMU and it is
>> ACPI internally used only. The buffer size returned to guest should
>> not include 'RLEN' itself
> Do you see any errors in guest with this bug present?
> It would be nice to put error messages here so that fix could be found
> later just by searching git log and qemu-devel for errors user sees
> in guest.
>

No, i did not see any error log in vm. I guess kernel nvdimm driver uses
the buffer based on the 'length' field. I will improve the code to check
whether the buffer size is matched with this field in vm.

>
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>  hw/acpi/nvdimm.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index e486128..5454c0f 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -863,6 +863,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
>>
>>      result_size = aml_local(1);
>>      aml_append(method, aml_store(aml_name("RLEN"), result_size));
>> +    /* RLEN is not included in the payload returned to guest. */
>> +    aml_append(method, aml_subtract(result_size, aml_int(4), result_size));
> you can merge above store with subtract like this:
>   aml_subtract(aml_name("RLEN"), foo, result_size)

Yes, it is better indeed.

>
> Style nit: try not to use magic numbers,
> look at how RLEN is defined earlier, extract it into macro and reuse in both places

Okay.

>
>
>>      aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
> instead of shiftleft, I'd suggest use here multiply operator and BITS_PER_BYTE
> so it would obvious what's going on and rewrite following without intermediate store.
>

I agree. However, qemu does not implement multiply primitive, i'd make a
separate patchset for these cleanups you suggested.

>>                                   result_size));
>>      aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
> aml_create_field(aml_name("ODAT"),
>                  aml_int(0),
>                  aml_multiply(result_size, aml_int(BITS_PER_BYTE), NULL),
>                  "OBUF"))
>
> BTW:
> dsm_out_buf_size is more descriptive than result_size

Yes, indeed.

>
> also NCAL later uses Arg6 when method has only 5 arguments which doesn't seem right
> instead of arg6 you should make/use local variable 'dsm_out_buf'

Sorry, my typo. Will fix.

>
> As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back.
> Currently I can't compile it back which mean it's really broken.
>

Good suggestion, i will try it.

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

[Xiao Guangrong]

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

[Qemu-devel] [PATCH v2 2/8] nvdimm acpi: prebuild nvdimm devices for available slots

[Xiao Guangrong]

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

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

[Igor Mammedov]

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

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

[Igor Mammedov]

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

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

[Xiao Guangrong]

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.

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

[Xiao Guangrong]

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.

[PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

_FIT is required for hotplug support, guest will inquire the updated
device info from it if a hotplug event is received

As FIT buffer is not completely mapped into guest address space, so a
new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
by QEMU to read the piece of FIT buffer. The buffer is concatenated
before _FIT return

Refer to docs/specs/acpi-nvdimm.txt for detailed design

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 0e2b9f0..4bbd1e7 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
     aml_append(dev, method);
 }
 
+static void nvdimm_build_fit(Aml *dev)
+{
+    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
+    Aml *whilectx, *ifcond, *ifctx, *fit;
+
+    buf = aml_local(0);
+    buf_size = aml_local(1);
+    fit = aml_local(2);
+
+    /* build helper function, RFIT. */
+    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);
+    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
+                                              aml_int(0), "OFST"));
+
+    /* prepare input package. */
+    pkg = aml_package(1);
+    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
+    aml_append(pkg, aml_name("OFST"));
+
+    /* call Read_FIT function. */
+    call_result = aml_call5(NVDIMM_COMMON_DSM,
+                            aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
+                            /* UUID for NVDIMM Root Device */),
+                            aml_int(1) /* Revision 1 */,
+                            aml_int(0xFFFFFFFF) /* Read FIT. */,
+                            pkg, aml_int(0) /* for root device. */);
+    aml_append(method, aml_store(call_result, buf));
+
+    /* handle _DSM result. */
+    aml_append(method, aml_create_dword_field(buf,
+               aml_int(0) /* offset at byte 0 */, "STAU"));
+
+     /* if something is wrong during _DSM. */
+    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
+    ifctx = aml_if(aml_lnot(ifcond));
+    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
+    aml_append(method, ifctx);
+
+    aml_append(method, aml_store(aml_sizeof(buf), buf_size));
+    aml_append(method, aml_subtract(buf_size,
+                                    aml_int(4) /* the size of "STAU" */,
+                                    buf_size));
+
+    /* if we read the end of fit. */
+    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
+    aml_append(method, ifctx);
+
+    aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)),
+                                 buf_size));
+    aml_append(method, aml_create_field(buf,
+                            aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
+                            buf_size, "BUFF"));
+    aml_append(method, aml_return(aml_name("BUFF")));
+    aml_append(dev, method);
+
+    /* build _FIT. */
+    method = aml_method("_FIT", 0, AML_NOTSERIALIZED);
+    offset = aml_local(3);
+
+    aml_append(method, aml_store(aml_buffer(0, NULL), fit));
+    aml_append(method, aml_store(aml_int(0), offset));
+
+    whilectx = aml_while(aml_int(1));
+    aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
+    aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
+
+    /* finish fit read if no data is read out. */
+    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+    aml_append(ifctx, aml_return(fit));
+    aml_append(whilectx, ifctx);
+
+    /* update the offset. */
+    aml_append(whilectx, aml_add(offset, buf_size, offset));
+    /* append the data we read out to the fit buffer. */
+    aml_append(whilectx, aml_concatenate(fit, buf, fit));
+    aml_append(method, whilectx);
+
+    aml_append(dev, method);
+}
+
 static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
 {
     uint32_t slot;
@@ -1001,6 +1082,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
 
     /* 0 is reserved for root device. */
     nvdimm_build_device_dsm(dev, 0);
+    nvdimm_build_fit(dev);
 
     nvdimm_build_nvdimm_devices(dev, ram_slots);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

_FIT is required for hotplug support, guest will inquire the updated
device info from it if a hotplug event is received

As FIT buffer is not completely mapped into guest address space, so a
new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
by QEMU to read the piece of FIT buffer. The buffer is concatenated
before _FIT return

Refer to docs/specs/acpi-nvdimm.txt for detailed design

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 0e2b9f0..4bbd1e7 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
     aml_append(dev, method);
 }
 
+static void nvdimm_build_fit(Aml *dev)
+{
+    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
+    Aml *whilectx, *ifcond, *ifctx, *fit;
+
+    buf = aml_local(0);
+    buf_size = aml_local(1);
+    fit = aml_local(2);
+
+    /* build helper function, RFIT. */
+    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);
+    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
+                                              aml_int(0), "OFST"));
+
+    /* prepare input package. */
+    pkg = aml_package(1);
+    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
+    aml_append(pkg, aml_name("OFST"));
+
+    /* call Read_FIT function. */
+    call_result = aml_call5(NVDIMM_COMMON_DSM,
+                            aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
+                            /* UUID for NVDIMM Root Device */),
+                            aml_int(1) /* Revision 1 */,
+                            aml_int(0xFFFFFFFF) /* Read FIT. */,
+                            pkg, aml_int(0) /* for root device. */);
+    aml_append(method, aml_store(call_result, buf));
+
+    /* handle _DSM result. */
+    aml_append(method, aml_create_dword_field(buf,
+               aml_int(0) /* offset at byte 0 */, "STAU"));
+
+     /* if something is wrong during _DSM. */
+    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
+    ifctx = aml_if(aml_lnot(ifcond));
+    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
+    aml_append(method, ifctx);
+
+    aml_append(method, aml_store(aml_sizeof(buf), buf_size));
+    aml_append(method, aml_subtract(buf_size,
+                                    aml_int(4) /* the size of "STAU" */,
+                                    buf_size));
+
+    /* if we read the end of fit. */
+    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
+    aml_append(method, ifctx);
+
+    aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)),
+                                 buf_size));
+    aml_append(method, aml_create_field(buf,
+                            aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
+                            buf_size, "BUFF"));
+    aml_append(method, aml_return(aml_name("BUFF")));
+    aml_append(dev, method);
+
+    /* build _FIT. */
+    method = aml_method("_FIT", 0, AML_NOTSERIALIZED);
+    offset = aml_local(3);
+
+    aml_append(method, aml_store(aml_buffer(0, NULL), fit));
+    aml_append(method, aml_store(aml_int(0), offset));
+
+    whilectx = aml_while(aml_int(1));
+    aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
+    aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
+
+    /* finish fit read if no data is read out. */
+    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+    aml_append(ifctx, aml_return(fit));
+    aml_append(whilectx, ifctx);
+
+    /* update the offset. */
+    aml_append(whilectx, aml_add(offset, buf_size, offset));
+    /* append the data we read out to the fit buffer. */
+    aml_append(whilectx, aml_concatenate(fit, buf, fit));
+    aml_append(method, whilectx);
+
+    aml_append(dev, method);
+}
+
 static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
 {
     uint32_t slot;
@@ -1001,6 +1082,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
 
     /* 0 is reserved for root device. */
     nvdimm_build_device_dsm(dev, 0);
+    nvdimm_build_fit(dev);
 
     nvdimm_build_nvdimm_devices(dev, ram_slots);
 
-- 
1.8.3.1

Re: [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:05 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> _FIT is required for hotplug support, guest will inquire the updated
> device info from it if a hotplug event is received
> 
> As FIT buffer is not completely mapped into guest address space, so a
> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
> by QEMU to read the piece of FIT buffer. The buffer is concatenated
> before _FIT return
Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
0xFFFFFFFF for some purposes.
So spec should be amended first or custom generated UUID should be used.

> 
> Refer to docs/specs/acpi-nvdimm.txt for detailed design
and amend docs to reflect that.

> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 0e2b9f0..4bbd1e7 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>      aml_append(dev, method);
>  }
>  
> +static void nvdimm_build_fit(Aml *dev)
> +{
> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
> +    Aml *whilectx, *ifcond, *ifctx, *fit;
> +
> +    buf = aml_local(0);
> +    buf_size = aml_local(1);
> +    fit = aml_local(2);
> +
> +    /* build helper function, RFIT. */
> +    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);
since you create named fields (global variable) in method scope,
you should make method serialized. Same goes for _FIT method.


> +    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
> +                                              aml_int(0), "OFST"));
> +
> +    /* prepare input package. */
> +    pkg = aml_package(1);
> +    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
> +    aml_append(pkg, aml_name("OFST"));
> +
> +    /* call Read_FIT function. */
> +    call_result = aml_call5(NVDIMM_COMMON_DSM,
> +                            aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
> +                            /* UUID for NVDIMM Root Device */),
> +                            aml_int(1) /* Revision 1 */,
> +                            aml_int(0xFFFFFFFF) /* Read FIT. */,
> +                            pkg, aml_int(0) /* for root device. */);
> +    aml_append(method, aml_store(call_result, buf));
> +
> +    /* handle _DSM result. */
> +    aml_append(method, aml_create_dword_field(buf,
> +               aml_int(0) /* offset at byte 0 */, "STAU"));
> +
> +     /* if something is wrong during _DSM. */
> +    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
> +    ifctx = aml_if(aml_lnot(ifcond));
> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> +    aml_append(method, ifctx);
> +    aml_append(method, aml_store(aml_sizeof(buf), buf_size));
> +    aml_append(method, aml_subtract(buf_size,
> +                                    aml_int(4) /* the size of "STAU" */,
> +                                    buf_size));

Since you handle error case the same as EOF case you could replace
it with EOF case here and on qemu side of interface as well. That should
simplify code a bit as you won't need to strip out func_ret_status.

> +
> +    /* if we read the end of fit. */
> +    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> +    aml_append(method, ifctx);
> +
> +    aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)),
> +                                 buf_size));
> +    aml_append(method, aml_create_field(buf,
> +                            aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
> +                            buf_size, "BUFF"));
> +    aml_append(method, aml_return(aml_name("BUFF")));
> +    aml_append(dev, method);
> +
> +    /* build _FIT. */
> +    method = aml_method("_FIT", 0, AML_NOTSERIALIZED);
> +    offset = aml_local(3);
> +
> +    aml_append(method, aml_store(aml_buffer(0, NULL), fit));
> +    aml_append(method, aml_store(aml_int(0), offset));
> +
> +    whilectx = aml_while(aml_int(1));
> +    aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
> +    aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
> +
> +    /* finish fit read if no data is read out. */
> +    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> +    aml_append(ifctx, aml_return(fit));
> +    aml_append(whilectx, ifctx);
> +
> +    /* update the offset. */
> +    aml_append(whilectx, aml_add(offset, buf_size, offset));
> +    /* append the data we read out to the fit buffer. */
> +    aml_append(whilectx, aml_concatenate(fit, buf, fit));
> +    aml_append(method, whilectx);
> +
> +    aml_append(dev, method);
> +}
> +
>  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  {
>      uint32_t slot;
> @@ -1001,6 +1082,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>  
>      /* 0 is reserved for root device. */
>      nvdimm_build_device_dsm(dev, 0);
> +    nvdimm_build_fit(dev);
>  
>      nvdimm_build_nvdimm_devices(dev, ram_slots);
>  

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:05 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> _FIT is required for hotplug support, guest will inquire the updated
> device info from it if a hotplug event is received
> 
> As FIT buffer is not completely mapped into guest address space, so a
> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
> by QEMU to read the piece of FIT buffer. The buffer is concatenated
> before _FIT return
Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
0xFFFFFFFF for some purposes.
So spec should be amended first or custom generated UUID should be used.

> 
> Refer to docs/specs/acpi-nvdimm.txt for detailed design
and amend docs to reflect that.

> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 0e2b9f0..4bbd1e7 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>      aml_append(dev, method);
>  }
>  
> +static void nvdimm_build_fit(Aml *dev)
> +{
> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
> +    Aml *whilectx, *ifcond, *ifctx, *fit;
> +
> +    buf = aml_local(0);
> +    buf_size = aml_local(1);
> +    fit = aml_local(2);
> +
> +    /* build helper function, RFIT. */
> +    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);
since you create named fields (global variable) in method scope,
you should make method serialized. Same goes for _FIT method.


> +    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
> +                                              aml_int(0), "OFST"));
> +
> +    /* prepare input package. */
> +    pkg = aml_package(1);
> +    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
> +    aml_append(pkg, aml_name("OFST"));
> +
> +    /* call Read_FIT function. */
> +    call_result = aml_call5(NVDIMM_COMMON_DSM,
> +                            aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
> +                            /* UUID for NVDIMM Root Device */),
> +                            aml_int(1) /* Revision 1 */,
> +                            aml_int(0xFFFFFFFF) /* Read FIT. */,
> +                            pkg, aml_int(0) /* for root device. */);
> +    aml_append(method, aml_store(call_result, buf));
> +
> +    /* handle _DSM result. */
> +    aml_append(method, aml_create_dword_field(buf,
> +               aml_int(0) /* offset at byte 0 */, "STAU"));
> +
> +     /* if something is wrong during _DSM. */
> +    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
> +    ifctx = aml_if(aml_lnot(ifcond));
> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> +    aml_append(method, ifctx);
> +    aml_append(method, aml_store(aml_sizeof(buf), buf_size));
> +    aml_append(method, aml_subtract(buf_size,
> +                                    aml_int(4) /* the size of "STAU" */,
> +                                    buf_size));

Since you handle error case the same as EOF case you could replace
it with EOF case here and on qemu side of interface as well. That should
simplify code a bit as you won't need to strip out func_ret_status.

> +
> +    /* if we read the end of fit. */
> +    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> +    aml_append(method, ifctx);
> +
> +    aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)),
> +                                 buf_size));
> +    aml_append(method, aml_create_field(buf,
> +                            aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
> +                            buf_size, "BUFF"));
> +    aml_append(method, aml_return(aml_name("BUFF")));
> +    aml_append(dev, method);
> +
> +    /* build _FIT. */
> +    method = aml_method("_FIT", 0, AML_NOTSERIALIZED);
> +    offset = aml_local(3);
> +
> +    aml_append(method, aml_store(aml_buffer(0, NULL), fit));
> +    aml_append(method, aml_store(aml_int(0), offset));
> +
> +    whilectx = aml_while(aml_int(1));
> +    aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
> +    aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
> +
> +    /* finish fit read if no data is read out. */
> +    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> +    aml_append(ifctx, aml_return(fit));
> +    aml_append(whilectx, ifctx);
> +
> +    /* update the offset. */
> +    aml_append(whilectx, aml_add(offset, buf_size, offset));
> +    /* append the data we read out to the fit buffer. */
> +    aml_append(whilectx, aml_concatenate(fit, buf, fit));
> +    aml_append(method, whilectx);
> +
> +    aml_append(dev, method);
> +}
> +
>  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  {
>      uint32_t slot;
> @@ -1001,6 +1082,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>  
>      /* 0 is reserved for root device. */
>      nvdimm_build_device_dsm(dev, 0);
> +    nvdimm_build_fit(dev);
>  
>      nvdimm_build_nvdimm_devices(dev, ram_slots);
>  

Re: [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 09/30/2016 09:14 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:05 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _FIT is required for hotplug support, guest will inquire the updated
>> device info from it if a hotplug event is received
>>
>> As FIT buffer is not completely mapped into guest address space, so a
>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
>> before _FIT return
> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> 0xFFFFFFFF for some purposes.
> So spec should be amended first or custom generated UUID should be used.

Okay.

I will change the changelog to reflect this fact and move the spec update
to this patch.

>
>>
>> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> and amend docs to reflect that.

Already done in the spec, i will merge the spec changes into
this patch as you suggested later.

>
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>  hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 82 insertions(+)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 0e2b9f0..4bbd1e7 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>>      aml_append(dev, method);
>>  }
>>
>> +static void nvdimm_build_fit(Aml *dev)
>> +{
>> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
>> +    Aml *whilectx, *ifcond, *ifctx, *fit;
>> +
>> +    buf = aml_local(0);
>> +    buf_size = aml_local(1);
>> +    fit = aml_local(2);
>> +
>> +    /* build helper function, RFIT. */
>> +    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);
> since you create named fields (global variable) in method scope,
> you should make method serialized. Same goes for _FIT method.

Indeed, will fix.

>
>
>> +    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
>> +                                              aml_int(0), "OFST"));
>> +
>> +    /* prepare input package. */
>> +    pkg = aml_package(1);
>> +    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
>> +    aml_append(pkg, aml_name("OFST"));
>> +
>> +    /* call Read_FIT function. */
>> +    call_result = aml_call5(NVDIMM_COMMON_DSM,
>> +                            aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
>> +                            /* UUID for NVDIMM Root Device */),
>> +                            aml_int(1) /* Revision 1 */,
>> +                            aml_int(0xFFFFFFFF) /* Read FIT. */,
>> +                            pkg, aml_int(0) /* for root device. */);
>> +    aml_append(method, aml_store(call_result, buf));
>> +
>> +    /* handle _DSM result. */
>> +    aml_append(method, aml_create_dword_field(buf,
>> +               aml_int(0) /* offset at byte 0 */, "STAU"));
>> +
>> +     /* if something is wrong during _DSM. */
>> +    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
>> +    ifctx = aml_if(aml_lnot(ifcond));
>> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
>> +    aml_append(method, ifctx);
>> +    aml_append(method, aml_store(aml_sizeof(buf), buf_size));
>> +    aml_append(method, aml_subtract(buf_size,
>> +                                    aml_int(4) /* the size of "STAU" */,
>> +                                    buf_size));
>
> Since you handle error case the same as EOF case you could replace
> it with EOF case here and on qemu side of interface as well. That should
> simplify code a bit as you won't need to strip out func_ret_status.
>

You mean returning NULL buffer if errors happen? However, the buffer is
generated by NVDIMM_COMMON_DSM function which is also used by _DSM based
on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway
no matter is successful or failed.

Or i missed your idea?

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 09/30/2016 09:14 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:05 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _FIT is required for hotplug support, guest will inquire the updated
>> device info from it if a hotplug event is received
>>
>> As FIT buffer is not completely mapped into guest address space, so a
>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
>> before _FIT return
> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> 0xFFFFFFFF for some purposes.
> So spec should be amended first or custom generated UUID should be used.

Okay.

I will change the changelog to reflect this fact and move the spec update
to this patch.

>
>>
>> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> and amend docs to reflect that.

Already done in the spec, i will merge the spec changes into
this patch as you suggested later.

>
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>  hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 82 insertions(+)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 0e2b9f0..4bbd1e7 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>>      aml_append(dev, method);
>>  }
>>
>> +static void nvdimm_build_fit(Aml *dev)
>> +{
>> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
>> +    Aml *whilectx, *ifcond, *ifctx, *fit;
>> +
>> +    buf = aml_local(0);
>> +    buf_size = aml_local(1);
>> +    fit = aml_local(2);
>> +
>> +    /* build helper function, RFIT. */
>> +    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);
> since you create named fields (global variable) in method scope,
> you should make method serialized. Same goes for _FIT method.

Indeed, will fix.

>
>
>> +    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
>> +                                              aml_int(0), "OFST"));
>> +
>> +    /* prepare input package. */
>> +    pkg = aml_package(1);
>> +    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
>> +    aml_append(pkg, aml_name("OFST"));
>> +
>> +    /* call Read_FIT function. */
>> +    call_result = aml_call5(NVDIMM_COMMON_DSM,
>> +                            aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
>> +                            /* UUID for NVDIMM Root Device */),
>> +                            aml_int(1) /* Revision 1 */,
>> +                            aml_int(0xFFFFFFFF) /* Read FIT. */,
>> +                            pkg, aml_int(0) /* for root device. */);
>> +    aml_append(method, aml_store(call_result, buf));
>> +
>> +    /* handle _DSM result. */
>> +    aml_append(method, aml_create_dword_field(buf,
>> +               aml_int(0) /* offset at byte 0 */, "STAU"));
>> +
>> +     /* if something is wrong during _DSM. */
>> +    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
>> +    ifctx = aml_if(aml_lnot(ifcond));
>> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
>> +    aml_append(method, ifctx);
>> +    aml_append(method, aml_store(aml_sizeof(buf), buf_size));
>> +    aml_append(method, aml_subtract(buf_size,
>> +                                    aml_int(4) /* the size of "STAU" */,
>> +                                    buf_size));
>
> Since you handle error case the same as EOF case you could replace
> it with EOF case here and on qemu side of interface as well. That should
> simplify code a bit as you won't need to strip out func_ret_status.
>

You mean returning NULL buffer if errors happen? However, the buffer is
generated by NVDIMM_COMMON_DSM function which is also used by _DSM based
on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway
no matter is successful or failed.

Or i missed your idea?

Re: [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Sat, 8 Oct 2016 15:17:14 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 09/30/2016 09:14 PM, Igor Mammedov wrote:
> > On Fri, 12 Aug 2016 14:54:05 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> _FIT is required for hotplug support, guest will inquire the updated
> >> device info from it if a hotplug event is received
> >>
> >> As FIT buffer is not completely mapped into guest address space, so a
> >> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
> >> by QEMU to read the piece of FIT buffer. The buffer is concatenated
> >> before _FIT return  
> > Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> > 0xFFFFFFFF for some purposes.
> > So spec should be amended first or custom generated UUID should be used.  
> 
> Okay.
> 
> I will change the changelog to reflect this fact and move the spec update
> to this patch.
under spec, I've meant ACPI spec where this UUID is declared

> 
> >  
> >>
> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design  
> > and amend docs to reflect that.  
> 
> Already done in the spec, i will merge the spec changes into
> this patch as you suggested later.
> 
> >  
> >>
> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> ---
> >>  hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 82 insertions(+)
> >>
> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> >> index 0e2b9f0..4bbd1e7 100644
> >> --- a/hw/acpi/nvdimm.c
> >> +++ b/hw/acpi/nvdimm.c
> >> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
> >>      aml_append(dev, method);
> >>  }
> >>
> >> +static void nvdimm_build_fit(Aml *dev)
> >> +{
> >> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
> >> +    Aml *whilectx, *ifcond, *ifctx, *fit;
> >> +
> >> +    buf = aml_local(0);
> >> +    buf_size = aml_local(1);
> >> +    fit = aml_local(2);
> >> +
> >> +    /* build helper function, RFIT. */
> >> +    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);  
> > since you create named fields (global variable) in method scope,
> > you should make method serialized. Same goes for _FIT method.  
> 
> Indeed, will fix.
> 
> >
> >  
> >> +    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
> >> +                                              aml_int(0), "OFST"));
> >> +
> >> +    /* prepare input package. */
> >> +    pkg = aml_package(1);
> >> +    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
> >> +    aml_append(pkg, aml_name("OFST"));
> >> +
> >> +    /* call Read_FIT function. */
> >> +    call_result = aml_call5(NVDIMM_COMMON_DSM,
> >> +                            aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
> >> +                            /* UUID for NVDIMM Root Device */),
> >> +                            aml_int(1) /* Revision 1 */,
> >> +                            aml_int(0xFFFFFFFF) /* Read FIT. */,
> >> +                            pkg, aml_int(0) /* for root device. */);
> >> +    aml_append(method, aml_store(call_result, buf));
> >> +
> >> +    /* handle _DSM result. */
> >> +    aml_append(method, aml_create_dword_field(buf,
> >> +               aml_int(0) /* offset at byte 0 */, "STAU"));
> >> +
> >> +     /* if something is wrong during _DSM. */
> >> +    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
> >> +    ifctx = aml_if(aml_lnot(ifcond));
> >> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> >> +    aml_append(method, ifctx);
> >> +    aml_append(method, aml_store(aml_sizeof(buf), buf_size));
> >> +    aml_append(method, aml_subtract(buf_size,
> >> +                                    aml_int(4) /* the size of "STAU" */,
> >> +                                    buf_size));  
> >
> > Since you handle error case the same as EOF case you could replace
> > it with EOF case here and on qemu side of interface as well. That should
> > simplify code a bit as you won't need to strip out func_ret_status.
> >  
> 
> You mean returning NULL buffer if errors happen? However, the buffer is
> generated by NVDIMM_COMMON_DSM function which is also used by _DSM based
> on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway
> no matter is successful or failed.
> 
> Or i missed your idea?
ok, leave it as is.

> 
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Sat, 8 Oct 2016 15:17:14 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 09/30/2016 09:14 PM, Igor Mammedov wrote:
> > On Fri, 12 Aug 2016 14:54:05 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> _FIT is required for hotplug support, guest will inquire the updated
> >> device info from it if a hotplug event is received
> >>
> >> As FIT buffer is not completely mapped into guest address space, so a
> >> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
> >> by QEMU to read the piece of FIT buffer. The buffer is concatenated
> >> before _FIT return  
> > Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> > 0xFFFFFFFF for some purposes.
> > So spec should be amended first or custom generated UUID should be used.  
> 
> Okay.
> 
> I will change the changelog to reflect this fact and move the spec update
> to this patch.
under spec, I've meant ACPI spec where this UUID is declared

> 
> >  
> >>
> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design  
> > and amend docs to reflect that.  
> 
> Already done in the spec, i will merge the spec changes into
> this patch as you suggested later.
> 
> >  
> >>
> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> ---
> >>  hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 82 insertions(+)
> >>
> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> >> index 0e2b9f0..4bbd1e7 100644
> >> --- a/hw/acpi/nvdimm.c
> >> +++ b/hw/acpi/nvdimm.c
> >> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
> >>      aml_append(dev, method);
> >>  }
> >>
> >> +static void nvdimm_build_fit(Aml *dev)
> >> +{
> >> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
> >> +    Aml *whilectx, *ifcond, *ifctx, *fit;
> >> +
> >> +    buf = aml_local(0);
> >> +    buf_size = aml_local(1);
> >> +    fit = aml_local(2);
> >> +
> >> +    /* build helper function, RFIT. */
> >> +    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);  
> > since you create named fields (global variable) in method scope,
> > you should make method serialized. Same goes for _FIT method.  
> 
> Indeed, will fix.
> 
> >
> >  
> >> +    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
> >> +                                              aml_int(0), "OFST"));
> >> +
> >> +    /* prepare input package. */
> >> +    pkg = aml_package(1);
> >> +    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
> >> +    aml_append(pkg, aml_name("OFST"));
> >> +
> >> +    /* call Read_FIT function. */
> >> +    call_result = aml_call5(NVDIMM_COMMON_DSM,
> >> +                            aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
> >> +                            /* UUID for NVDIMM Root Device */),
> >> +                            aml_int(1) /* Revision 1 */,
> >> +                            aml_int(0xFFFFFFFF) /* Read FIT. */,
> >> +                            pkg, aml_int(0) /* for root device. */);
> >> +    aml_append(method, aml_store(call_result, buf));
> >> +
> >> +    /* handle _DSM result. */
> >> +    aml_append(method, aml_create_dword_field(buf,
> >> +               aml_int(0) /* offset at byte 0 */, "STAU"));
> >> +
> >> +     /* if something is wrong during _DSM. */
> >> +    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
> >> +    ifctx = aml_if(aml_lnot(ifcond));
> >> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> >> +    aml_append(method, ifctx);
> >> +    aml_append(method, aml_store(aml_sizeof(buf), buf_size));
> >> +    aml_append(method, aml_subtract(buf_size,
> >> +                                    aml_int(4) /* the size of "STAU" */,
> >> +                                    buf_size));  
> >
> > Since you handle error case the same as EOF case you could replace
> > it with EOF case here and on qemu side of interface as well. That should
> > simplify code a bit as you won't need to strip out func_ret_status.
> >  
> 
> You mean returning NULL buffer if errors happen? However, the buffer is
> generated by NVDIMM_COMMON_DSM function which is also used by _DSM based
> on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway
> no matter is successful or failed.
> 
> Or i missed your idea?
ok, leave it as is.

> 
> 
> 
> 

Re: [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 10/10/2016 08:51 PM, Igor Mammedov wrote:
> On Sat, 8 Oct 2016 15:17:14 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> On 09/30/2016 09:14 PM, Igor Mammedov wrote:
>>> On Fri, 12 Aug 2016 14:54:05 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>>> _FIT is required for hotplug support, guest will inquire the updated
>>>> device info from it if a hotplug event is received
>>>>
>>>> As FIT buffer is not completely mapped into guest address space, so a
>>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
>>>> before _FIT return
>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
>>> 0xFFFFFFFF for some purposes.
>>> So spec should be amended first or custom generated UUID should be used.
>>
>> Okay.
>>
>> I will change the changelog to reflect this fact and move the spec update
>> to this patch.
> under spec, I've meant ACPI spec where this UUID is declared

Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
in the future.

I'd prefer to custom-generated UUID, however, currently the UUID is checked
in OSPM, i.e, QEMU is not able to distinguish other UUIDs, so how about
drop the UUID check in ACPI and pass the UUID info to QEMU?

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 10/10/2016 08:51 PM, Igor Mammedov wrote:
> On Sat, 8 Oct 2016 15:17:14 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> On 09/30/2016 09:14 PM, Igor Mammedov wrote:
>>> On Fri, 12 Aug 2016 14:54:05 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>>> _FIT is required for hotplug support, guest will inquire the updated
>>>> device info from it if a hotplug event is received
>>>>
>>>> As FIT buffer is not completely mapped into guest address space, so a
>>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
>>>> before _FIT return
>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
>>> 0xFFFFFFFF for some purposes.
>>> So spec should be amended first or custom generated UUID should be used.
>>
>> Okay.
>>
>> I will change the changelog to reflect this fact and move the spec update
>> to this patch.
> under spec, I've meant ACPI spec where this UUID is declared

Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
in the future.

I'd prefer to custom-generated UUID, however, currently the UUID is checked
in OSPM, i.e, QEMU is not able to distinguish other UUIDs, so how about
drop the UUID check in ACPI and pass the UUID info to QEMU?

Re: [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Mon, 10 Oct 2016 21:09:30 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 10/10/2016 08:51 PM, Igor Mammedov wrote:
> > On Sat, 8 Oct 2016 15:17:14 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> On 09/30/2016 09:14 PM, Igor Mammedov wrote:  
> >>> On Fri, 12 Aug 2016 14:54:05 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>  
> >>>> _FIT is required for hotplug support, guest will inquire the updated
> >>>> device info from it if a hotplug event is received
> >>>>
> >>>> As FIT buffer is not completely mapped into guest address space, so a
> >>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
> >>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
> >>>> before _FIT return  
> >>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> >>> 0xFFFFFFFF for some purposes.
> >>> So spec should be amended first or custom generated UUID should be used.  
> >>
> >> Okay.
> >>
> >> I will change the changelog to reflect this fact and move the spec update
> >> to this patch.  
> > under spec, I've meant ACPI spec where this UUID is declared  
> 
> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
> in the future.
> 
> I'd prefer to custom-generated UUID, however, currently the UUID is checked
> in OSPM, i.e, QEMU is not able to distinguish other UUIDs,
I'd go with custom-generated UUID

> so how about
> drop the UUID check in ACPI and pass the UUID info to QEMU?
It's a bit late to do so as it would be qemu-guest ABI change and
one would need to maintain old and new protocol.

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Mon, 10 Oct 2016 21:09:30 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 10/10/2016 08:51 PM, Igor Mammedov wrote:
> > On Sat, 8 Oct 2016 15:17:14 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> On 09/30/2016 09:14 PM, Igor Mammedov wrote:  
> >>> On Fri, 12 Aug 2016 14:54:05 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>  
> >>>> _FIT is required for hotplug support, guest will inquire the updated
> >>>> device info from it if a hotplug event is received
> >>>>
> >>>> As FIT buffer is not completely mapped into guest address space, so a
> >>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
> >>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
> >>>> before _FIT return  
> >>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> >>> 0xFFFFFFFF for some purposes.
> >>> So spec should be amended first or custom generated UUID should be used.  
> >>
> >> Okay.
> >>
> >> I will change the changelog to reflect this fact and move the spec update
> >> to this patch.  
> > under spec, I've meant ACPI spec where this UUID is declared  
> 
> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
> in the future.
> 
> I'd prefer to custom-generated UUID, however, currently the UUID is checked
> in OSPM, i.e, QEMU is not able to distinguish other UUIDs,
I'd go with custom-generated UUID

> so how about
> drop the UUID check in ACPI and pass the UUID info to QEMU?
It's a bit late to do so as it would be qemu-guest ABI change and
one would need to maintain old and new protocol.

Re: [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 10/11/2016 07:49 PM, Igor Mammedov wrote:
> On Mon, 10 Oct 2016 21:09:30 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> On 10/10/2016 08:51 PM, Igor Mammedov wrote:
>>> On Sat, 8 Oct 2016 15:17:14 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote:
>>>>> On Fri, 12 Aug 2016 14:54:05 +0800
>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>>>
>>>>>> _FIT is required for hotplug support, guest will inquire the updated
>>>>>> device info from it if a hotplug event is received
>>>>>>
>>>>>> As FIT buffer is not completely mapped into guest address space, so a
>>>>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
>>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
>>>>>> before _FIT return
>>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
>>>>> 0xFFFFFFFF for some purposes.
>>>>> So spec should be amended first or custom generated UUID should be used.
>>>>
>>>> Okay.
>>>>
>>>> I will change the changelog to reflect this fact and move the spec update
>>>> to this patch.
>>> under spec, I've meant ACPI spec where this UUID is declared
>>
>> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
>> in the future.
>>
>> I'd prefer to custom-generated UUID, however, currently the UUID is checked
>> in OSPM, i.e, QEMU is not able to distinguish other UUIDs,
> I'd go with custom-generated UUID
>
>> so how about
>> drop the UUID check in ACPI and pass the UUID info to QEMU?
> It's a bit late to do so as it would be qemu-guest ABI change and
> one would need to maintain old and new protocol.
>

Okay.

How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use
it to indicate different UUIDs, i,e:
struct NvdimmDsmIn {
     uint16_t handle;
     uint16_t uuid_type;
     uint32_t revision;
     uint32_t function;
     /* the remaining size in the page is used by arg3. */
     union {
         uint8_t arg3[4084];
     };
} QEMU_PACKED;
typedef struct NvdimmDsmIn NvdimmDsmIn;

For this case, we set uuid_type = 1 to indicate the UUID is the one used
for RFIT.

u16 for 'handle' is large enough as QEMU supports 255 memory devices.

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 10/11/2016 07:49 PM, Igor Mammedov wrote:
> On Mon, 10 Oct 2016 21:09:30 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> On 10/10/2016 08:51 PM, Igor Mammedov wrote:
>>> On Sat, 8 Oct 2016 15:17:14 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote:
>>>>> On Fri, 12 Aug 2016 14:54:05 +0800
>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>>>
>>>>>> _FIT is required for hotplug support, guest will inquire the updated
>>>>>> device info from it if a hotplug event is received
>>>>>>
>>>>>> As FIT buffer is not completely mapped into guest address space, so a
>>>>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
>>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
>>>>>> before _FIT return
>>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
>>>>> 0xFFFFFFFF for some purposes.
>>>>> So spec should be amended first or custom generated UUID should be used.
>>>>
>>>> Okay.
>>>>
>>>> I will change the changelog to reflect this fact and move the spec update
>>>> to this patch.
>>> under spec, I've meant ACPI spec where this UUID is declared
>>
>> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
>> in the future.
>>
>> I'd prefer to custom-generated UUID, however, currently the UUID is checked
>> in OSPM, i.e, QEMU is not able to distinguish other UUIDs,
> I'd go with custom-generated UUID
>
>> so how about
>> drop the UUID check in ACPI and pass the UUID info to QEMU?
> It's a bit late to do so as it would be qemu-guest ABI change and
> one would need to maintain old and new protocol.
>

Okay.

How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use
it to indicate different UUIDs, i,e:
struct NvdimmDsmIn {
     uint16_t handle;
     uint16_t uuid_type;
     uint32_t revision;
     uint32_t function;
     /* the remaining size in the page is used by arg3. */
     union {
         uint8_t arg3[4084];
     };
} QEMU_PACKED;
typedef struct NvdimmDsmIn NvdimmDsmIn;

For this case, we set uuid_type = 1 to indicate the UUID is the one used
for RFIT.

u16 for 'handle' is large enough as QEMU supports 255 memory devices.

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Wed, 12 Oct 2016 16:20:07 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 10/11/2016 07:49 PM, Igor Mammedov wrote:
> > On Mon, 10 Oct 2016 21:09:30 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> On 10/10/2016 08:51 PM, Igor Mammedov wrote:  
> >>> On Sat, 8 Oct 2016 15:17:14 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>  
> >>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote:  
> >>>>> On Fri, 12 Aug 2016 14:54:05 +0800
> >>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>>>  
> >>>>>> _FIT is required for hotplug support, guest will inquire the updated
> >>>>>> device info from it if a hotplug event is received
> >>>>>>
> >>>>>> As FIT buffer is not completely mapped into guest address space, so a
> >>>>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
> >>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
> >>>>>> before _FIT return  
> >>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> >>>>> 0xFFFFFFFF for some purposes.
> >>>>> So spec should be amended first or custom generated UUID should be used.  
> >>>>
> >>>> Okay.
> >>>>
> >>>> I will change the changelog to reflect this fact and move the spec update
> >>>> to this patch.  
> >>> under spec, I've meant ACPI spec where this UUID is declared  
> >>
> >> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
> >> in the future.
> >>
> >> I'd prefer to custom-generated UUID, however, currently the UUID is checked
> >> in OSPM, i.e, QEMU is not able to distinguish other UUIDs,  
> > I'd go with custom-generated UUID
> >  
> >> so how about
> >> drop the UUID check in ACPI and pass the UUID info to QEMU?  
> > It's a bit late to do so as it would be qemu-guest ABI change and
> > one would need to maintain old and new protocol.
> >  
> 
> Okay.
> 
> How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use
> it to indicate different UUIDs, i,e:
> struct NvdimmDsmIn {
>      uint16_t handle;
>      uint16_t uuid_type;
>      uint32_t revision;
>      uint32_t function;
>      /* the remaining size in the page is used by arg3. */
>      union {
>          uint8_t arg3[4084];
>      };
> } QEMU_PACKED;
> typedef struct NvdimmDsmIn NvdimmDsmIn;
> 
> For this case, we set uuid_type = 1 to indicate the UUID is the one used
> for RFIT.
> 
> u16 for 'handle' is large enough as QEMU supports 255 memory devices.
I wouldn't do above as it needlessly complicates qemu<->OSPM
protocol.
Since handle is completely internal QEMU thing we can just reserve 0xFFFFFFFF value
in docs/specs/acpi_nvdimm.txt  for RFIT and keep UUID checks only on ASL side.

and on QEMU side add check to guaranty that auto generated handle for DIMMs
would never go into reserved range.

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 10/13/2016 09:33 PM, Igor Mammedov wrote:
> On Wed, 12 Oct 2016 16:20:07 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> On 10/11/2016 07:49 PM, Igor Mammedov wrote:
>>> On Mon, 10 Oct 2016 21:09:30 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>>> On 10/10/2016 08:51 PM, Igor Mammedov wrote:
>>>>> On Sat, 8 Oct 2016 15:17:14 +0800
>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>>>
>>>>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote:
>>>>>>> On Fri, 12 Aug 2016 14:54:05 +0800
>>>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>>>>>
>>>>>>>> _FIT is required for hotplug support, guest will inquire the updated
>>>>>>>> device info from it if a hotplug event is received
>>>>>>>>
>>>>>>>> As FIT buffer is not completely mapped into guest address space, so a
>>>>>>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
>>>>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
>>>>>>>> before _FIT return
>>>>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
>>>>>>> 0xFFFFFFFF for some purposes.
>>>>>>> So spec should be amended first or custom generated UUID should be used.
>>>>>>
>>>>>> Okay.
>>>>>>
>>>>>> I will change the changelog to reflect this fact and move the spec update
>>>>>> to this patch.
>>>>> under spec, I've meant ACPI spec where this UUID is declared
>>>>
>>>> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
>>>> in the future.
>>>>
>>>> I'd prefer to custom-generated UUID, however, currently the UUID is checked
>>>> in OSPM, i.e, QEMU is not able to distinguish other UUIDs,
>>> I'd go with custom-generated UUID
>>>
>>>> so how about
>>>> drop the UUID check in ACPI and pass the UUID info to QEMU?
>>> It's a bit late to do so as it would be qemu-guest ABI change and
>>> one would need to maintain old and new protocol.
>>>
>>
>> Okay.
>>
>> How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use
>> it to indicate different UUIDs, i,e:
>> struct NvdimmDsmIn {
>>      uint16_t handle;
>>      uint16_t uuid_type;
>>      uint32_t revision;
>>      uint32_t function;
>>      /* the remaining size in the page is used by arg3. */
>>      union {
>>          uint8_t arg3[4084];
>>      };
>> } QEMU_PACKED;
>> typedef struct NvdimmDsmIn NvdimmDsmIn;
>>
>> For this case, we set uuid_type = 1 to indicate the UUID is the one used
>> for RFIT.
>>
>> u16 for 'handle' is large enough as QEMU supports 255 memory devices.
> I wouldn't do above as it needlessly complicates qemu<->OSPM
> protocol.
> Since handle is completely internal QEMU thing we can just reserve 0xFFFFFFFF value
> in docs/specs/acpi_nvdimm.txt  for RFIT and keep UUID checks only on ASL side.
>
> and on QEMU side add check to guaranty that auto generated handle for DIMMs
> would never go into reserved range.
>

Okay, I agree. As we may need to support multiple UUIDs in the future. I'd
like to document the rule for handle reservation:
   The handle is completely QEMU internal thing, the value in range [0, 0xFFFF]
   indicates nvdimm device (O means nvdimm root device named NVDR), the values
   out of this region are reserved by other purpose.

   Current reserved handles:
   0x10000 is reserved for RFIT.

Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Fri, 14 Oct 2016 15:43:50 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 10/13/2016 09:33 PM, Igor Mammedov wrote:
> > On Wed, 12 Oct 2016 16:20:07 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> On 10/11/2016 07:49 PM, Igor Mammedov wrote:  
> >>> On Mon, 10 Oct 2016 21:09:30 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>  
> >>>> On 10/10/2016 08:51 PM, Igor Mammedov wrote:  
> >>>>> On Sat, 8 Oct 2016 15:17:14 +0800
> >>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>>>  
> >>>>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote:  
> >>>>>>> On Fri, 12 Aug 2016 14:54:05 +0800
> >>>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>>>>>  
> >>>>>>>> _FIT is required for hotplug support, guest will inquire the updated
> >>>>>>>> device info from it if a hotplug event is received
> >>>>>>>>
> >>>>>>>> As FIT buffer is not completely mapped into guest address space, so a
> >>>>>>>> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved
> >>>>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
> >>>>>>>> before _FIT return  
> >>>>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> >>>>>>> 0xFFFFFFFF for some purposes.
> >>>>>>> So spec should be amended first or custom generated UUID should be used.  
> >>>>>>
> >>>>>> Okay.
> >>>>>>
> >>>>>> I will change the changelog to reflect this fact and move the spec update
> >>>>>> to this patch.  
> >>>>> under spec, I've meant ACPI spec where this UUID is declared  
> >>>>
> >>>> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used
> >>>> in the future.
> >>>>
> >>>> I'd prefer to custom-generated UUID, however, currently the UUID is checked
> >>>> in OSPM, i.e, QEMU is not able to distinguish other UUIDs,  
> >>> I'd go with custom-generated UUID
> >>>  
> >>>> so how about
> >>>> drop the UUID check in ACPI and pass the UUID info to QEMU?  
> >>> It's a bit late to do so as it would be qemu-guest ABI change and
> >>> one would need to maintain old and new protocol.
> >>>  
> >>
> >> Okay.
> >>
> >> How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use
> >> it to indicate different UUIDs, i,e:
> >> struct NvdimmDsmIn {
> >>      uint16_t handle;
> >>      uint16_t uuid_type;
> >>      uint32_t revision;
> >>      uint32_t function;
> >>      /* the remaining size in the page is used by arg3. */
> >>      union {
> >>          uint8_t arg3[4084];
> >>      };
> >> } QEMU_PACKED;
> >> typedef struct NvdimmDsmIn NvdimmDsmIn;
> >>
> >> For this case, we set uuid_type = 1 to indicate the UUID is the one used
> >> for RFIT.
> >>
> >> u16 for 'handle' is large enough as QEMU supports 255 memory devices.  
> > I wouldn't do above as it needlessly complicates qemu<->OSPM
> > protocol.
> > Since handle is completely internal QEMU thing we can just reserve 0xFFFFFFFF value
> > in docs/specs/acpi_nvdimm.txt  for RFIT and keep UUID checks only on ASL side.
> >
> > and on QEMU side add check to guaranty that auto generated handle for DIMMs
> > would never go into reserved range.
> >  
> 
> Okay, I agree. As we may need to support multiple UUIDs in the future. I'd
> like to document the rule for handle reservation:
>    The handle is completely QEMU internal thing, the value in range [0, 0xFFFF]
>    indicates nvdimm device (O means nvdimm root device named NVDR), the values
>    out of this region are reserved by other purpose.
> 
>    Current reserved handles:
>    0x10000 is reserved for RFIT.
sounds fine to me

[PATCH v2 4/8] nvdimm acpi: implement Read FIT function

[Xiao Guangrong]

Read FIT whose function index is 0xFFFFFFFF is reserved by QEMU to read
the piece of FIT buffer. Please refer to docs/specs/acpi_nvdimm.txt for
detailed info

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 4bbd1e7..f6f8413 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -466,6 +466,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
                   offsetof(NvdimmDsmIn, arg3) > 4096);
 
+struct NvdimmFuncReadFITIn {
+    uint32_t offset; /* the offset of FIT buffer. */
+} QEMU_PACKED;
+typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
+                  offsetof(NvdimmDsmIn, arg3) > 4096);
+
+struct NvdimmFuncReadFITOut {
+    /* the size of buffer filled by QEMU. */
+    uint32_t len;
+    uint32_t func_ret_status; /* return status code. */
+    uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;
+typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
+
 static void
 nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
 {
@@ -486,6 +502,46 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
     cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
 }
 
+/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
+static void nvdimm_dsm_func_read_fit(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
+{
+    NvdimmFuncReadFITIn *read_fit;
+    NvdimmFuncReadFITOut *read_fit_out;
+    GSList *device_list = nvdimm_get_plugged_device_list();
+    GArray *fit = nvdimm_build_device_structure(device_list);
+    uint32_t read_len = 0, func_ret_status;
+    int size;
+
+    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
+    le32_to_cpus(&read_fit->offset);
+
+    nvdimm_debug("Read FIT: offset %#x FIT size %#x.\n", read_fit->offset,
+                 fit->len);
+
+    if (read_fit->offset > fit->len) {
+        func_ret_status = 3 /* Invalid Input Parameters */;
+        goto build_out;
+    }
+
+    func_ret_status = 0 /* Success */;
+    read_len = MIN(fit->len - read_fit->offset,
+                   4096 - sizeof(NvdimmFuncReadFITOut));
+
+build_out:
+    size = sizeof(NvdimmFuncReadFITOut) + read_len;
+    read_fit_out = g_malloc(size);
+
+    read_fit_out->len = cpu_to_le32(size);
+    read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
+    memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
+
+    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
+
+    g_slist_free(device_list);
+    g_array_free(fit, true);
+    g_free(read_fit_out);
+}
+
 static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
 {
     /*
@@ -498,6 +554,11 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
         return;
     }
 
+    if (in->function == 0xFFFFFFFF /* Read FIT */) {
+        nvdimm_dsm_func_read_fit(in, dsm_mem_addr);
+        return;
+    }
+
     /* No function except function 0 is supported yet. */
     nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/8] nvdimm acpi: implement Read FIT function

[Xiao Guangrong]

Read FIT whose function index is 0xFFFFFFFF is reserved by QEMU to read
the piece of FIT buffer. Please refer to docs/specs/acpi_nvdimm.txt for
detailed info

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 4bbd1e7..f6f8413 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -466,6 +466,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
                   offsetof(NvdimmDsmIn, arg3) > 4096);
 
+struct NvdimmFuncReadFITIn {
+    uint32_t offset; /* the offset of FIT buffer. */
+} QEMU_PACKED;
+typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
+                  offsetof(NvdimmDsmIn, arg3) > 4096);
+
+struct NvdimmFuncReadFITOut {
+    /* the size of buffer filled by QEMU. */
+    uint32_t len;
+    uint32_t func_ret_status; /* return status code. */
+    uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;
+typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
+
 static void
 nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
 {
@@ -486,6 +502,46 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
     cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
 }
 
+/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
+static void nvdimm_dsm_func_read_fit(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
+{
+    NvdimmFuncReadFITIn *read_fit;
+    NvdimmFuncReadFITOut *read_fit_out;
+    GSList *device_list = nvdimm_get_plugged_device_list();
+    GArray *fit = nvdimm_build_device_structure(device_list);
+    uint32_t read_len = 0, func_ret_status;
+    int size;
+
+    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
+    le32_to_cpus(&read_fit->offset);
+
+    nvdimm_debug("Read FIT: offset %#x FIT size %#x.\n", read_fit->offset,
+                 fit->len);
+
+    if (read_fit->offset > fit->len) {
+        func_ret_status = 3 /* Invalid Input Parameters */;
+        goto build_out;
+    }
+
+    func_ret_status = 0 /* Success */;
+    read_len = MIN(fit->len - read_fit->offset,
+                   4096 - sizeof(NvdimmFuncReadFITOut));
+
+build_out:
+    size = sizeof(NvdimmFuncReadFITOut) + read_len;
+    read_fit_out = g_malloc(size);
+
+    read_fit_out->len = cpu_to_le32(size);
+    read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
+    memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
+
+    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
+
+    g_slist_free(device_list);
+    g_array_free(fit, true);
+    g_free(read_fit_out);
+}
+
 static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
 {
     /*
@@ -498,6 +554,11 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
         return;
     }
 
+    if (in->function == 0xFFFFFFFF /* Read FIT */) {
+        nvdimm_dsm_func_read_fit(in, dsm_mem_addr);
+        return;
+    }
+
     /* No function except function 0 is supported yet. */
     nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
 }
-- 
1.8.3.1

Re: [PATCH v2 4/8] nvdimm acpi: implement Read FIT function

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:06 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Read FIT whose function index is 0xFFFFFFFF is reserved by QEMU to read
> the piece of FIT buffer. Please refer to docs/specs/acpi_nvdimm.txt for
> detailed info

Pls, squash this patch into 3/8

> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 4bbd1e7..f6f8413 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -466,6 +466,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>                    offsetof(NvdimmDsmIn, arg3) > 4096);
>  
> +struct NvdimmFuncReadFITIn {
> +    uint32_t offset; /* the offset of FIT buffer. */
> +} QEMU_PACKED;
> +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> +                  offsetof(NvdimmDsmIn, arg3) > 4096);
> +
> +struct NvdimmFuncReadFITOut {
> +    /* the size of buffer filled by QEMU. */
> +    uint32_t len;
> +    uint32_t func_ret_status; /* return status code. */
> +    uint8_t fit[0]; /* the FIT data. */
> +} QEMU_PACKED;
> +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
> +
>  static void
>  nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
>  {
> @@ -486,6 +502,46 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>  }
>  
> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
> +static void nvdimm_dsm_func_read_fit(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> +{
> +    NvdimmFuncReadFITIn *read_fit;
> +    NvdimmFuncReadFITOut *read_fit_out;
> +    GSList *device_list = nvdimm_get_plugged_device_list();
> +    GArray *fit = nvdimm_build_device_structure(device_list);
> +    uint32_t read_len = 0, func_ret_status;
> +    int size;
> +
> +    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> +    le32_to_cpus(&read_fit->offset);
> +
> +    nvdimm_debug("Read FIT: offset %#x FIT size %#x.\n", read_fit->offset,
> +                 fit->len);
> +
> +    if (read_fit->offset > fit->len) {
> +        func_ret_status = 3 /* Invalid Input Parameters */;
> +        goto build_out;
> +    }
> +
> +    func_ret_status = 0 /* Success */;
> +    read_len = MIN(fit->len - read_fit->offset,
> +                   4096 - sizeof(NvdimmFuncReadFITOut));
> +
> +build_out:
> +    size = sizeof(NvdimmFuncReadFITOut) + read_len;
> +    read_fit_out = g_malloc(size);
> +
> +    read_fit_out->len = cpu_to_le32(size);
> +    read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
> +    memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
> +
> +    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
> +
> +    g_slist_free(device_list);
> +    g_array_free(fit, true);
> +    g_free(read_fit_out);
> +}
> +
>  static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>  {
>      /*
> @@ -498,6 +554,11 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>          return;
>      }
>  
> +    if (in->function == 0xFFFFFFFF /* Read FIT */) {
> +        nvdimm_dsm_func_read_fit(in, dsm_mem_addr);
> +        return;
> +    }
> +
>      /* No function except function 0 is supported yet. */
>      nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
>  }

Re: [Qemu-devel] [PATCH v2 4/8] nvdimm acpi: implement Read FIT function

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:06 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Read FIT whose function index is 0xFFFFFFFF is reserved by QEMU to read
> the piece of FIT buffer. Please refer to docs/specs/acpi_nvdimm.txt for
> detailed info

Pls, squash this patch into 3/8

> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 4bbd1e7..f6f8413 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -466,6 +466,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>                    offsetof(NvdimmDsmIn, arg3) > 4096);
>  
> +struct NvdimmFuncReadFITIn {
> +    uint32_t offset; /* the offset of FIT buffer. */
> +} QEMU_PACKED;
> +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> +                  offsetof(NvdimmDsmIn, arg3) > 4096);
> +
> +struct NvdimmFuncReadFITOut {
> +    /* the size of buffer filled by QEMU. */
> +    uint32_t len;
> +    uint32_t func_ret_status; /* return status code. */
> +    uint8_t fit[0]; /* the FIT data. */
> +} QEMU_PACKED;
> +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
> +
>  static void
>  nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
>  {
> @@ -486,6 +502,46 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>  }
>  
> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
> +static void nvdimm_dsm_func_read_fit(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> +{
> +    NvdimmFuncReadFITIn *read_fit;
> +    NvdimmFuncReadFITOut *read_fit_out;
> +    GSList *device_list = nvdimm_get_plugged_device_list();
> +    GArray *fit = nvdimm_build_device_structure(device_list);
> +    uint32_t read_len = 0, func_ret_status;
> +    int size;
> +
> +    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> +    le32_to_cpus(&read_fit->offset);
> +
> +    nvdimm_debug("Read FIT: offset %#x FIT size %#x.\n", read_fit->offset,
> +                 fit->len);
> +
> +    if (read_fit->offset > fit->len) {
> +        func_ret_status = 3 /* Invalid Input Parameters */;
> +        goto build_out;
> +    }
> +
> +    func_ret_status = 0 /* Success */;
> +    read_len = MIN(fit->len - read_fit->offset,
> +                   4096 - sizeof(NvdimmFuncReadFITOut));
> +
> +build_out:
> +    size = sizeof(NvdimmFuncReadFITOut) + read_len;
> +    read_fit_out = g_malloc(size);
> +
> +    read_fit_out->len = cpu_to_le32(size);
> +    read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
> +    memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
> +
> +    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
> +
> +    g_slist_free(device_list);
> +    g_array_free(fit, true);
> +    g_free(read_fit_out);
> +}
> +
>  static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>  {
>      /*
> @@ -498,6 +554,11 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>          return;
>      }
>  
> +    if (in->function == 0xFFFFFFFF /* Read FIT */) {
> +        nvdimm_dsm_func_read_fit(in, dsm_mem_addr);
> +        return;
> +    }
> +
>      /* No function except function 0 is supported yet. */
>      nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
>  }

Re: [PATCH v2 4/8] nvdimm acpi: implement Read FIT function

[Xiao Guangrong]

On 09/30/2016 09:17 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:06 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> Read FIT whose function index is 0xFFFFFFFF is reserved by QEMU to read
>> the piece of FIT buffer. Please refer to docs/specs/acpi_nvdimm.txt for
>> detailed info
>
> Pls, squash this patch into 3/8

Okay.

Re: [Qemu-devel] [PATCH v2 4/8] nvdimm acpi: implement Read FIT function

[Xiao Guangrong]

On 09/30/2016 09:17 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:06 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> Read FIT whose function index is 0xFFFFFFFF is reserved by QEMU to read
>> the piece of FIT buffer. Please refer to docs/specs/acpi_nvdimm.txt for
>> detailed info
>
> Pls, squash this patch into 3/8

Okay.

[PATCH v2 5/8] pc-dimm: introduce prepare_unplug() callback

[Xiao Guangrong]

We should let nvdimm acpi know which nvdimm device is being unplugged
before QEMU interrupts the guest so that nvdimm acpi can update its
FIT properly

prepare_unplug() callback is introduced exactly for this purpose then
the being removed device is skipped when guest reads FIT

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/ich9.c           | 3 +++
 hw/acpi/nvdimm.c         | 9 +++++++++
 hw/acpi/piix4.c          | 3 +++
 hw/mem/nvdimm.c          | 8 ++++++++
 hw/mem/pc-dimm.c         | 5 +++++
 include/hw/mem/nvdimm.h  | 3 +++
 include/hw/mem/pc-dimm.h | 1 +
 7 files changed, 32 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index e5a3c18..af08471 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -511,6 +511,9 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 
     if (lpc->pm.acpi_memory_hotplug.is_enabled &&
         object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dev);
+
+        ddc->prepare_unplug(dev);
         acpi_memory_unplug_request_cb(hotplug_dev,
                                       &lpc->pm.acpi_memory_hotplug, dev,
                                       errp);
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index f6f8413..0f569c7 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -355,6 +355,15 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
 
     for (; device_list; device_list = device_list->next) {
         DeviceState *dev = device_list->data;
+        NVDIMMDevice *nvdimm = NVDIMM(dev);
+
+        /*
+         * the nvdimm device which is being removed should not be included
+         * in nfit/fit.
+         */
+        if (nvdimm->is_removing) {
+            continue;
+        }
 
         /* build System Physical Address Range Structure. */
         nvdimm_build_structure_spa(structures, dev);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2adc246..57e19e6 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -400,6 +400,9 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 
     if (s->acpi_memory_hotplug.is_enabled &&
         object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dev);
+
+        ddc->prepare_unplug(dev);
         acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
                                       dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7895805..5bc81fe 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -146,6 +146,13 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
     return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
 }
 
+static void nvdimm_prepare_unplug(DeviceState *dev)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(dev);
+
+    nvdimm->is_removing = true;
+}
+
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -158,6 +165,7 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
     ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
+    ddc->prepare_unplug = nvdimm_prepare_unplug;
 
     nvc->read_label_data = nvdimm_read_label_data;
     nvc->write_label_data = nvdimm_write_label_data;
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9e8dab0..09364c9 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -436,6 +436,10 @@ static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
     return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
 }
 
+static  void pc_dimm_prepare_unplug(DeviceState *dev)
+{
+}
+
 static void pc_dimm_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -448,6 +452,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
 
     ddc->get_memory_region = pc_dimm_get_memory_region;
     ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
+    ddc->prepare_unplug = pc_dimm_prepare_unplug;
 }
 
 static TypeInfo pc_dimm_info = {
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 63a2b20..e626199 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -71,6 +71,9 @@ struct NVDIMMDevice {
      * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
      */
     MemoryRegion nvdimm_mr;
+
+    /* the device is being unplugged. */
+    bool is_removing;
 };
 typedef struct NVDIMMDevice NVDIMMDevice;
 
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 1e483f2..34797ba 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -73,6 +73,7 @@ typedef struct PCDIMMDeviceClass {
     void (*realize)(PCDIMMDevice *dimm, Error **errp);
     MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
     MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
+    void (*prepare_unplug)(DeviceState *dev);
 } PCDIMMDeviceClass;
 
 /**
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/8] pc-dimm: introduce prepare_unplug() callback

[Xiao Guangrong]

We should let nvdimm acpi know which nvdimm device is being unplugged
before QEMU interrupts the guest so that nvdimm acpi can update its
FIT properly

prepare_unplug() callback is introduced exactly for this purpose then
the being removed device is skipped when guest reads FIT

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/ich9.c           | 3 +++
 hw/acpi/nvdimm.c         | 9 +++++++++
 hw/acpi/piix4.c          | 3 +++
 hw/mem/nvdimm.c          | 8 ++++++++
 hw/mem/pc-dimm.c         | 5 +++++
 include/hw/mem/nvdimm.h  | 3 +++
 include/hw/mem/pc-dimm.h | 1 +
 7 files changed, 32 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index e5a3c18..af08471 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -511,6 +511,9 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 
     if (lpc->pm.acpi_memory_hotplug.is_enabled &&
         object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dev);
+
+        ddc->prepare_unplug(dev);
         acpi_memory_unplug_request_cb(hotplug_dev,
                                       &lpc->pm.acpi_memory_hotplug, dev,
                                       errp);
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index f6f8413..0f569c7 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -355,6 +355,15 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
 
     for (; device_list; device_list = device_list->next) {
         DeviceState *dev = device_list->data;
+        NVDIMMDevice *nvdimm = NVDIMM(dev);
+
+        /*
+         * the nvdimm device which is being removed should not be included
+         * in nfit/fit.
+         */
+        if (nvdimm->is_removing) {
+            continue;
+        }
 
         /* build System Physical Address Range Structure. */
         nvdimm_build_structure_spa(structures, dev);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2adc246..57e19e6 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -400,6 +400,9 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 
     if (s->acpi_memory_hotplug.is_enabled &&
         object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dev);
+
+        ddc->prepare_unplug(dev);
         acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
                                       dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7895805..5bc81fe 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -146,6 +146,13 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
     return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
 }
 
+static void nvdimm_prepare_unplug(DeviceState *dev)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(dev);
+
+    nvdimm->is_removing = true;
+}
+
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -158,6 +165,7 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
     ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
+    ddc->prepare_unplug = nvdimm_prepare_unplug;
 
     nvc->read_label_data = nvdimm_read_label_data;
     nvc->write_label_data = nvdimm_write_label_data;
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9e8dab0..09364c9 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -436,6 +436,10 @@ static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
     return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
 }
 
+static  void pc_dimm_prepare_unplug(DeviceState *dev)
+{
+}
+
 static void pc_dimm_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -448,6 +452,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
 
     ddc->get_memory_region = pc_dimm_get_memory_region;
     ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
+    ddc->prepare_unplug = pc_dimm_prepare_unplug;
 }
 
 static TypeInfo pc_dimm_info = {
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 63a2b20..e626199 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -71,6 +71,9 @@ struct NVDIMMDevice {
      * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
      */
     MemoryRegion nvdimm_mr;
+
+    /* the device is being unplugged. */
+    bool is_removing;
 };
 typedef struct NVDIMMDevice NVDIMMDevice;
 
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 1e483f2..34797ba 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -73,6 +73,7 @@ typedef struct PCDIMMDeviceClass {
     void (*realize)(PCDIMMDevice *dimm, Error **errp);
     MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
     MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
+    void (*prepare_unplug)(DeviceState *dev);
 } PCDIMMDeviceClass;
 
 /**
-- 
1.8.3.1

Re: [PATCH v2 5/8] pc-dimm: introduce prepare_unplug() callback

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:07 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> We should let nvdimm acpi know which nvdimm device is being unplugged
> before QEMU interrupts the guest so that nvdimm acpi can update its
> FIT properly
> 
> prepare_unplug() callback is introduced exactly for this purpose then
> the being removed device is skipped when guest reads FIT
Hot-unplug for NVDIMMs isn't mentinoned in spec so I'd would drop it for now.

> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/ich9.c           | 3 +++
>  hw/acpi/nvdimm.c         | 9 +++++++++
>  hw/acpi/piix4.c          | 3 +++
>  hw/mem/nvdimm.c          | 8 ++++++++
>  hw/mem/pc-dimm.c         | 5 +++++
>  include/hw/mem/nvdimm.h  | 3 +++
>  include/hw/mem/pc-dimm.h | 1 +
>  7 files changed, 32 insertions(+)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e5a3c18..af08471 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -511,6 +511,9 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  
>      if (lpc->pm.acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dev);
> +
> +        ddc->prepare_unplug(dev);
>          acpi_memory_unplug_request_cb(hotplug_dev,
>                                        &lpc->pm.acpi_memory_hotplug, dev,
>                                        errp);
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index f6f8413..0f569c7 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -355,6 +355,15 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
>  
>      for (; device_list; device_list = device_list->next) {
>          DeviceState *dev = device_list->data;
> +        NVDIMMDevice *nvdimm = NVDIMM(dev);
> +
> +        /*
> +         * the nvdimm device which is being removed should not be included
> +         * in nfit/fit.
> +         */
> +        if (nvdimm->is_removing) {
> +            continue;
> +        }
>  
>          /* build System Physical Address Range Structure. */
>          nvdimm_build_structure_spa(structures, dev);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 2adc246..57e19e6 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -400,6 +400,9 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  
>      if (s->acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dev);
> +
> +        ddc->prepare_unplug(dev);
>          acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
>                                        dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7895805..5bc81fe 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -146,6 +146,13 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>      return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
>  }
>  
> +static void nvdimm_prepare_unplug(DeviceState *dev)
> +{
> +    NVDIMMDevice *nvdimm = NVDIMM(dev);
> +
> +    nvdimm->is_removing = true;
> +}
> +
>  static void nvdimm_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -158,6 +165,7 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
>      ddc->realize = nvdimm_realize;
>      ddc->get_memory_region = nvdimm_get_memory_region;
>      ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
> +    ddc->prepare_unplug = nvdimm_prepare_unplug;
>  
>      nvc->read_label_data = nvdimm_read_label_data;
>      nvc->write_label_data = nvdimm_write_label_data;
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 9e8dab0..09364c9 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -436,6 +436,10 @@ static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>      return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
>  }
>  
> +static  void pc_dimm_prepare_unplug(DeviceState *dev)
> +{
> +}
> +
>  static void pc_dimm_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -448,6 +452,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>  
>      ddc->get_memory_region = pc_dimm_get_memory_region;
>      ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
> +    ddc->prepare_unplug = pc_dimm_prepare_unplug;
>  }
>  
>  static TypeInfo pc_dimm_info = {
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 63a2b20..e626199 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -71,6 +71,9 @@ struct NVDIMMDevice {
>       * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
>       */
>      MemoryRegion nvdimm_mr;
> +
> +    /* the device is being unplugged. */
> +    bool is_removing;
>  };
>  typedef struct NVDIMMDevice NVDIMMDevice;
>  
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2..34797ba 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -73,6 +73,7 @@ typedef struct PCDIMMDeviceClass {
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
>      MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> +    void (*prepare_unplug)(DeviceState *dev);
>  } PCDIMMDeviceClass;
>  
>  /**

Re: [Qemu-devel] [PATCH v2 5/8] pc-dimm: introduce prepare_unplug() callback

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:07 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> We should let nvdimm acpi know which nvdimm device is being unplugged
> before QEMU interrupts the guest so that nvdimm acpi can update its
> FIT properly
> 
> prepare_unplug() callback is introduced exactly for this purpose then
> the being removed device is skipped when guest reads FIT
Hot-unplug for NVDIMMs isn't mentinoned in spec so I'd would drop it for now.

> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/ich9.c           | 3 +++
>  hw/acpi/nvdimm.c         | 9 +++++++++
>  hw/acpi/piix4.c          | 3 +++
>  hw/mem/nvdimm.c          | 8 ++++++++
>  hw/mem/pc-dimm.c         | 5 +++++
>  include/hw/mem/nvdimm.h  | 3 +++
>  include/hw/mem/pc-dimm.h | 1 +
>  7 files changed, 32 insertions(+)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e5a3c18..af08471 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -511,6 +511,9 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  
>      if (lpc->pm.acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dev);
> +
> +        ddc->prepare_unplug(dev);
>          acpi_memory_unplug_request_cb(hotplug_dev,
>                                        &lpc->pm.acpi_memory_hotplug, dev,
>                                        errp);
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index f6f8413..0f569c7 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -355,6 +355,15 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
>  
>      for (; device_list; device_list = device_list->next) {
>          DeviceState *dev = device_list->data;
> +        NVDIMMDevice *nvdimm = NVDIMM(dev);
> +
> +        /*
> +         * the nvdimm device which is being removed should not be included
> +         * in nfit/fit.
> +         */
> +        if (nvdimm->is_removing) {
> +            continue;
> +        }
>  
>          /* build System Physical Address Range Structure. */
>          nvdimm_build_structure_spa(structures, dev);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 2adc246..57e19e6 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -400,6 +400,9 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  
>      if (s->acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dev);
> +
> +        ddc->prepare_unplug(dev);
>          acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
>                                        dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7895805..5bc81fe 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -146,6 +146,13 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>      return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
>  }
>  
> +static void nvdimm_prepare_unplug(DeviceState *dev)
> +{
> +    NVDIMMDevice *nvdimm = NVDIMM(dev);
> +
> +    nvdimm->is_removing = true;
> +}
> +
>  static void nvdimm_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -158,6 +165,7 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
>      ddc->realize = nvdimm_realize;
>      ddc->get_memory_region = nvdimm_get_memory_region;
>      ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
> +    ddc->prepare_unplug = nvdimm_prepare_unplug;
>  
>      nvc->read_label_data = nvdimm_read_label_data;
>      nvc->write_label_data = nvdimm_write_label_data;
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 9e8dab0..09364c9 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -436,6 +436,10 @@ static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>      return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
>  }
>  
> +static  void pc_dimm_prepare_unplug(DeviceState *dev)
> +{
> +}
> +
>  static void pc_dimm_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -448,6 +452,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>  
>      ddc->get_memory_region = pc_dimm_get_memory_region;
>      ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
> +    ddc->prepare_unplug = pc_dimm_prepare_unplug;
>  }
>  
>  static TypeInfo pc_dimm_info = {
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 63a2b20..e626199 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -71,6 +71,9 @@ struct NVDIMMDevice {
>       * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
>       */
>      MemoryRegion nvdimm_mr;
> +
> +    /* the device is being unplugged. */
> +    bool is_removing;
>  };
>  typedef struct NVDIMMDevice NVDIMMDevice;
>  
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2..34797ba 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -73,6 +73,7 @@ typedef struct PCDIMMDeviceClass {
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
>      MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> +    void (*prepare_unplug)(DeviceState *dev);
>  } PCDIMMDeviceClass;
>  
>  /**

Re: [PATCH v2 5/8] pc-dimm: introduce prepare_unplug() callback

[Xiao Guangrong]

On 10/03/2016 05:45 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:07 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> We should let nvdimm acpi know which nvdimm device is being unplugged
>> before QEMU interrupts the guest so that nvdimm acpi can update its
>> FIT properly
>>
>> prepare_unplug() callback is introduced exactly for this purpose then
>> the being removed device is skipped when guest reads FIT
> Hot-unplug for NVDIMMs isn't mentinoned in spec so I'd would drop it for now.

Okay. Then i will stop hot-unplug and generate a error message if nvdimm device
is being un-plugged:
   "NVDIMM hot-unplug has not been supported yet."

Re: [Qemu-devel] [PATCH v2 5/8] pc-dimm: introduce prepare_unplug() callback

[Xiao Guangrong]

On 10/03/2016 05:45 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:07 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> We should let nvdimm acpi know which nvdimm device is being unplugged
>> before QEMU interrupts the guest so that nvdimm acpi can update its
>> FIT properly
>>
>> prepare_unplug() callback is introduced exactly for this purpose then
>> the being removed device is skipped when guest reads FIT
> Hot-unplug for NVDIMMs isn't mentinoned in spec so I'd would drop it for now.

Okay. Then i will stop hot-unplug and generate a error message if nvdimm device
is being un-plugged:
   "NVDIMM hot-unplug has not been supported yet."

[PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Xiao Guangrong]

nvdimm's memory info can not exported via _CRS, instead, it is reported
by NFIT/FIT

This patch let _CRS return zero for both memory address and memory size
if it is a nvdimm device inserted to the slot

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/memory_hotplug.c         | 16 ++++++++++++----
 include/hw/acpi/memory_hotplug.h |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ec4e64b..73fa62d 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -2,6 +2,7 @@
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/pc-hotplug.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 #include "hw/boards.h"
 #include "hw/qdev-core.h"
 #include "trace.h"
@@ -41,6 +42,7 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list)
 static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
                                          unsigned int size)
 {
+    uint64_t maddr, msize;
     uint32_t val = 0;
     MemHotplugState *mem_st = opaque;
     MemStatus *mdev;
@@ -53,21 +55,25 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
 
     mdev = &mem_st->devs[mem_st->selector];
     o = OBJECT(mdev->dimm);
+    maddr = o && !mdev->is_nvdimm ? object_property_get_int(o,
+                                        PC_DIMM_ADDR_PROP, NULL) : 0;
+    msize = o && !mdev->is_nvdimm ? object_property_get_int(o,
+                                        PC_DIMM_SIZE_PROP, NULL) : 0;
     switch (addr) {
     case 0x0: /* Lo part of phys address where DIMM is mapped */
-        val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) : 0;
+        val = maddr;
         trace_mhp_acpi_read_addr_lo(mem_st->selector, val);
         break;
     case 0x4: /* Hi part of phys address where DIMM is mapped */
-        val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) >> 32 : 0;
+        val = maddr >> 32;
         trace_mhp_acpi_read_addr_hi(mem_st->selector, val);
         break;
     case 0x8: /* Lo part of DIMM size */
-        val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) : 0;
+        val = msize;
         trace_mhp_acpi_read_size_lo(mem_st->selector, val);
         break;
     case 0xc: /* Hi part of DIMM size */
-        val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) >> 32 : 0;
+        val = msize >> 32;
         trace_mhp_acpi_read_size_hi(mem_st->selector, val);
         break;
     case 0x10: /* node proximity for _PXM method */
@@ -78,6 +84,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
         val |= mdev->is_enabled   ? 1 : 0;
         val |= mdev->is_inserting ? 2 : 0;
         val |= mdev->is_removing  ? 4 : 0;
+        val |= mdev->is_nvdimm    ? 16 : 0;
         trace_mhp_acpi_read_flags(mem_st->selector, val);
         break;
     default:
@@ -245,6 +252,7 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
 
     mdev->dimm = dev;
     mdev->is_enabled = true;
+    mdev->is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
     if (dev->hotplugged) {
         mdev->is_inserting = true;
         acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index d2c7452..69a05df 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -17,6 +17,7 @@ typedef struct MemStatus {
     bool is_enabled;
     bool is_inserting;
     bool is_removing;
+    bool is_nvdimm;
     uint32_t ost_event;
     uint32_t ost_status;
 } MemStatus;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Xiao Guangrong]

nvdimm's memory info can not exported via _CRS, instead, it is reported
by NFIT/FIT

This patch let _CRS return zero for both memory address and memory size
if it is a nvdimm device inserted to the slot

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/memory_hotplug.c         | 16 ++++++++++++----
 include/hw/acpi/memory_hotplug.h |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ec4e64b..73fa62d 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -2,6 +2,7 @@
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/pc-hotplug.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 #include "hw/boards.h"
 #include "hw/qdev-core.h"
 #include "trace.h"
@@ -41,6 +42,7 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list)
 static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
                                          unsigned int size)
 {
+    uint64_t maddr, msize;
     uint32_t val = 0;
     MemHotplugState *mem_st = opaque;
     MemStatus *mdev;
@@ -53,21 +55,25 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
 
     mdev = &mem_st->devs[mem_st->selector];
     o = OBJECT(mdev->dimm);
+    maddr = o && !mdev->is_nvdimm ? object_property_get_int(o,
+                                        PC_DIMM_ADDR_PROP, NULL) : 0;
+    msize = o && !mdev->is_nvdimm ? object_property_get_int(o,
+                                        PC_DIMM_SIZE_PROP, NULL) : 0;
     switch (addr) {
     case 0x0: /* Lo part of phys address where DIMM is mapped */
-        val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) : 0;
+        val = maddr;
         trace_mhp_acpi_read_addr_lo(mem_st->selector, val);
         break;
     case 0x4: /* Hi part of phys address where DIMM is mapped */
-        val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) >> 32 : 0;
+        val = maddr >> 32;
         trace_mhp_acpi_read_addr_hi(mem_st->selector, val);
         break;
     case 0x8: /* Lo part of DIMM size */
-        val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) : 0;
+        val = msize;
         trace_mhp_acpi_read_size_lo(mem_st->selector, val);
         break;
     case 0xc: /* Hi part of DIMM size */
-        val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) >> 32 : 0;
+        val = msize >> 32;
         trace_mhp_acpi_read_size_hi(mem_st->selector, val);
         break;
     case 0x10: /* node proximity for _PXM method */
@@ -78,6 +84,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
         val |= mdev->is_enabled   ? 1 : 0;
         val |= mdev->is_inserting ? 2 : 0;
         val |= mdev->is_removing  ? 4 : 0;
+        val |= mdev->is_nvdimm    ? 16 : 0;
         trace_mhp_acpi_read_flags(mem_st->selector, val);
         break;
     default:
@@ -245,6 +252,7 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
 
     mdev->dimm = dev;
     mdev->is_enabled = true;
+    mdev->is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
     if (dev->hotplugged) {
         mdev->is_inserting = true;
         acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index d2c7452..69a05df 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -17,6 +17,7 @@ typedef struct MemStatus {
     bool is_enabled;
     bool is_inserting;
     bool is_removing;
+    bool is_nvdimm;
     uint32_t ost_event;
     uint32_t ost_status;
 } MemStatus;
-- 
1.8.3.1

Re: [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:08 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> nvdimm's memory info can not exported via _CRS, instead, it is reported
> by NFIT/FIT
> 
> This patch let _CRS return zero for both memory address and memory size
> if it is a nvdimm device inserted to the slot
I'm not sure if it's right thing to do.
As it's relatively new, spec isn't clear about NVDIMM hotplug
process and how it's related to PNP0C80 memory devices.

The thing is that notify to PNP0C80 will trigger regular
memory hotplug which would expect a valid _CRS and
I won't even try to predict reaction of different guests
on such behavior.

So far exposing NFIT was sufficient for guest to work with
NVDIMMs at startup. So I'd assume NVDIMM_ROOT._FIT() would
provide sufficient info to hot-plug NVDIMM and allow 
NVDIMM driver to handle it.

The only case of using PNP0C80 with NVDIMM, I'd imagine, is
when one would like to expose NVDIMM as conventional RAM and
make guest OS use it as such, which is probably not what you'd
intended do here.

Question is should/could NVDIMM hotplug work without using
PNP0C80?

One could reuse/share memhotplug GPE._E03 to notify NVDIMM_ROOT
but even that is not necessary as NVDIMM has its own QEMU<->guest
interface and could just take the next free _E04 handler.
So I'd suggest to redo this and 7/8 patches to implement
independent (of memhotplug) NVDIMM hotplug as a starting point.


> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/memory_hotplug.c         | 16 ++++++++++++----
>  include/hw/acpi/memory_hotplug.h |  1 +
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ec4e64b..73fa62d 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -2,6 +2,7 @@
>  #include "hw/acpi/memory_hotplug.h"
>  #include "hw/acpi/pc-hotplug.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/nvdimm.h"
>  #include "hw/boards.h"
>  #include "hw/qdev-core.h"
>  #include "trace.h"
> @@ -41,6 +42,7 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list)
>  static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
>                                           unsigned int size)
>  {
> +    uint64_t maddr, msize;
>      uint32_t val = 0;
>      MemHotplugState *mem_st = opaque;
>      MemStatus *mdev;
> @@ -53,21 +55,25 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
>  
>      mdev = &mem_st->devs[mem_st->selector];
>      o = OBJECT(mdev->dimm);
> +    maddr = o && !mdev->is_nvdimm ? object_property_get_int(o,
> +                                        PC_DIMM_ADDR_PROP, NULL) : 0;
> +    msize = o && !mdev->is_nvdimm ? object_property_get_int(o,
> +                                        PC_DIMM_SIZE_PROP, NULL) : 0;
>      switch (addr) {
>      case 0x0: /* Lo part of phys address where DIMM is mapped */
> -        val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) : 0;
> +        val = maddr;
>          trace_mhp_acpi_read_addr_lo(mem_st->selector, val);
>          break;
>      case 0x4: /* Hi part of phys address where DIMM is mapped */
> -        val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) >> 32 : 0;
> +        val = maddr >> 32;
>          trace_mhp_acpi_read_addr_hi(mem_st->selector, val);
>          break;
>      case 0x8: /* Lo part of DIMM size */
> -        val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) : 0;
> +        val = msize;
>          trace_mhp_acpi_read_size_lo(mem_st->selector, val);
>          break;
>      case 0xc: /* Hi part of DIMM size */
> -        val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) >> 32 : 0;
> +        val = msize >> 32;
>          trace_mhp_acpi_read_size_hi(mem_st->selector, val);
>          break;
>      case 0x10: /* node proximity for _PXM method */
> @@ -78,6 +84,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
>          val |= mdev->is_enabled   ? 1 : 0;
>          val |= mdev->is_inserting ? 2 : 0;
>          val |= mdev->is_removing  ? 4 : 0;
> +        val |= mdev->is_nvdimm    ? 16 : 0;
>          trace_mhp_acpi_read_flags(mem_st->selector, val);
>          break;
>      default:
> @@ -245,6 +252,7 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
>  
>      mdev->dimm = dev;
>      mdev->is_enabled = true;
> +    mdev->is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>      if (dev->hotplugged) {
>          mdev->is_inserting = true;
>          acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index d2c7452..69a05df 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -17,6 +17,7 @@ typedef struct MemStatus {
>      bool is_enabled;
>      bool is_inserting;
>      bool is_removing;
> +    bool is_nvdimm;
>      uint32_t ost_event;
>      uint32_t ost_status;
>  } MemStatus;

Re: [Qemu-devel] [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:08 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> nvdimm's memory info can not exported via _CRS, instead, it is reported
> by NFIT/FIT
> 
> This patch let _CRS return zero for both memory address and memory size
> if it is a nvdimm device inserted to the slot
I'm not sure if it's right thing to do.
As it's relatively new, spec isn't clear about NVDIMM hotplug
process and how it's related to PNP0C80 memory devices.

The thing is that notify to PNP0C80 will trigger regular
memory hotplug which would expect a valid _CRS and
I won't even try to predict reaction of different guests
on such behavior.

So far exposing NFIT was sufficient for guest to work with
NVDIMMs at startup. So I'd assume NVDIMM_ROOT._FIT() would
provide sufficient info to hot-plug NVDIMM and allow 
NVDIMM driver to handle it.

The only case of using PNP0C80 with NVDIMM, I'd imagine, is
when one would like to expose NVDIMM as conventional RAM and
make guest OS use it as such, which is probably not what you'd
intended do here.

Question is should/could NVDIMM hotplug work without using
PNP0C80?

One could reuse/share memhotplug GPE._E03 to notify NVDIMM_ROOT
but even that is not necessary as NVDIMM has its own QEMU<->guest
interface and could just take the next free _E04 handler.
So I'd suggest to redo this and 7/8 patches to implement
independent (of memhotplug) NVDIMM hotplug as a starting point.


> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/memory_hotplug.c         | 16 ++++++++++++----
>  include/hw/acpi/memory_hotplug.h |  1 +
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ec4e64b..73fa62d 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -2,6 +2,7 @@
>  #include "hw/acpi/memory_hotplug.h"
>  #include "hw/acpi/pc-hotplug.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/nvdimm.h"
>  #include "hw/boards.h"
>  #include "hw/qdev-core.h"
>  #include "trace.h"
> @@ -41,6 +42,7 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list)
>  static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
>                                           unsigned int size)
>  {
> +    uint64_t maddr, msize;
>      uint32_t val = 0;
>      MemHotplugState *mem_st = opaque;
>      MemStatus *mdev;
> @@ -53,21 +55,25 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
>  
>      mdev = &mem_st->devs[mem_st->selector];
>      o = OBJECT(mdev->dimm);
> +    maddr = o && !mdev->is_nvdimm ? object_property_get_int(o,
> +                                        PC_DIMM_ADDR_PROP, NULL) : 0;
> +    msize = o && !mdev->is_nvdimm ? object_property_get_int(o,
> +                                        PC_DIMM_SIZE_PROP, NULL) : 0;
>      switch (addr) {
>      case 0x0: /* Lo part of phys address where DIMM is mapped */
> -        val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) : 0;
> +        val = maddr;
>          trace_mhp_acpi_read_addr_lo(mem_st->selector, val);
>          break;
>      case 0x4: /* Hi part of phys address where DIMM is mapped */
> -        val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) >> 32 : 0;
> +        val = maddr >> 32;
>          trace_mhp_acpi_read_addr_hi(mem_st->selector, val);
>          break;
>      case 0x8: /* Lo part of DIMM size */
> -        val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) : 0;
> +        val = msize;
>          trace_mhp_acpi_read_size_lo(mem_st->selector, val);
>          break;
>      case 0xc: /* Hi part of DIMM size */
> -        val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) >> 32 : 0;
> +        val = msize >> 32;
>          trace_mhp_acpi_read_size_hi(mem_st->selector, val);
>          break;
>      case 0x10: /* node proximity for _PXM method */
> @@ -78,6 +84,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
>          val |= mdev->is_enabled   ? 1 : 0;
>          val |= mdev->is_inserting ? 2 : 0;
>          val |= mdev->is_removing  ? 4 : 0;
> +        val |= mdev->is_nvdimm    ? 16 : 0;
>          trace_mhp_acpi_read_flags(mem_st->selector, val);
>          break;
>      default:
> @@ -245,6 +252,7 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
>  
>      mdev->dimm = dev;
>      mdev->is_enabled = true;
> +    mdev->is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>      if (dev->hotplugged) {
>          mdev->is_inserting = true;
>          acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index d2c7452..69a05df 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -17,6 +17,7 @@ typedef struct MemStatus {
>      bool is_enabled;
>      bool is_inserting;
>      bool is_removing;
> +    bool is_nvdimm;
>      uint32_t ost_event;
>      uint32_t ost_status;
>  } MemStatus;

Re: [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Xiao Guangrong]

On 10/03/2016 09:21 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:08 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> nvdimm's memory info can not exported via _CRS, instead, it is reported
>> by NFIT/FIT
>>
>> This patch let _CRS return zero for both memory address and memory size
>> if it is a nvdimm device inserted to the slot
> I'm not sure if it's right thing to do.
> As it's relatively new, spec isn't clear about NVDIMM hotplug
> process and how it's related to PNP0C80 memory devices.
>
> The thing is that notify to PNP0C80 will trigger regular
> memory hotplug which would expect a valid _CRS and
> I won't even try to predict reaction of different guests
> on such behavior.
>
> So far exposing NFIT was sufficient for guest to work with
> NVDIMMs at startup. So I'd assume NVDIMM_ROOT._FIT() would
> provide sufficient info to hot-plug NVDIMM and allow
> NVDIMM driver to handle it.
>
> The only case of using PNP0C80 with NVDIMM, I'd imagine, is
> when one would like to expose NVDIMM as conventional RAM and
> make guest OS use it as such, which is probably not what you'd
> intended do here.
>
> Question is should/could NVDIMM hotplug work without using
> PNP0C80?

Some confusion i got when i was reading ACPI spec. I thing you
are right that notifying memory device (PNP0C80) is needed only
if the nvdimm is trying to export conventional RAM.

>
> One could reuse/share memhotplug GPE._E03 to notify NVDIMM_ROOT
> but even that is not necessary as NVDIMM has its own QEMU<->guest
> interface and could just take the next free _E04 handler.
> So I'd suggest to redo this and 7/8 patches to implement
> independent (of memhotplug) NVDIMM hotplug as a starting point.

Based on ACPI Spec, the memory device (PNP0C80) and nvdimm device
is notified by the same interruption:

Scope (\_GPE)
{
Method (_L00) {
Notify (\_SB.NVDR, 0x80) // Notify to NVDIMM root device
Notify (\_SB.MEM0, 1) // Device Check to Memory Module
}
}

So, it is better to reuse _E03 and only notify NVDIMM device if the
hotplug event is triggered on a slot with nvdimm device attached?

Re: [Qemu-devel] [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Xiao Guangrong]

On 10/03/2016 09:21 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:08 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> nvdimm's memory info can not exported via _CRS, instead, it is reported
>> by NFIT/FIT
>>
>> This patch let _CRS return zero for both memory address and memory size
>> if it is a nvdimm device inserted to the slot
> I'm not sure if it's right thing to do.
> As it's relatively new, spec isn't clear about NVDIMM hotplug
> process and how it's related to PNP0C80 memory devices.
>
> The thing is that notify to PNP0C80 will trigger regular
> memory hotplug which would expect a valid _CRS and
> I won't even try to predict reaction of different guests
> on such behavior.
>
> So far exposing NFIT was sufficient for guest to work with
> NVDIMMs at startup. So I'd assume NVDIMM_ROOT._FIT() would
> provide sufficient info to hot-plug NVDIMM and allow
> NVDIMM driver to handle it.
>
> The only case of using PNP0C80 with NVDIMM, I'd imagine, is
> when one would like to expose NVDIMM as conventional RAM and
> make guest OS use it as such, which is probably not what you'd
> intended do here.
>
> Question is should/could NVDIMM hotplug work without using
> PNP0C80?

Some confusion i got when i was reading ACPI spec. I thing you
are right that notifying memory device (PNP0C80) is needed only
if the nvdimm is trying to export conventional RAM.

>
> One could reuse/share memhotplug GPE._E03 to notify NVDIMM_ROOT
> but even that is not necessary as NVDIMM has its own QEMU<->guest
> interface and could just take the next free _E04 handler.
> So I'd suggest to redo this and 7/8 patches to implement
> independent (of memhotplug) NVDIMM hotplug as a starting point.

Based on ACPI Spec, the memory device (PNP0C80) and nvdimm device
is notified by the same interruption:

Scope (\_GPE)
{
Method (_L00) {
Notify (\_SB.NVDR, 0x80) // Notify to NVDIMM root device
Notify (\_SB.MEM0, 1) // Device Check to Memory Module
}
}

So, it is better to reuse _E03 and only notify NVDIMM device if the
hotplug event is triggered on a slot with nvdimm device attached?

Re: [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Igor Mammedov]

On Sat, 8 Oct 2016 15:42:41 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 10/03/2016 09:21 PM, Igor Mammedov wrote:
> > On Fri, 12 Aug 2016 14:54:08 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> nvdimm's memory info can not exported via _CRS, instead, it is reported
> >> by NFIT/FIT
> >>
> >> This patch let _CRS return zero for both memory address and memory size
> >> if it is a nvdimm device inserted to the slot  
> > I'm not sure if it's right thing to do.
> > As it's relatively new, spec isn't clear about NVDIMM hotplug
> > process and how it's related to PNP0C80 memory devices.
> >
> > The thing is that notify to PNP0C80 will trigger regular
> > memory hotplug which would expect a valid _CRS and
> > I won't even try to predict reaction of different guests
> > on such behavior.
> >
> > So far exposing NFIT was sufficient for guest to work with
> > NVDIMMs at startup. So I'd assume NVDIMM_ROOT._FIT() would
> > provide sufficient info to hot-plug NVDIMM and allow
> > NVDIMM driver to handle it.
> >
> > The only case of using PNP0C80 with NVDIMM, I'd imagine, is
> > when one would like to expose NVDIMM as conventional RAM and
> > make guest OS use it as such, which is probably not what you'd
> > intended do here.
> >
> > Question is should/could NVDIMM hotplug work without using
> > PNP0C80?  
> 
> Some confusion i got when i was reading ACPI spec. I thing you
> are right that notifying memory device (PNP0C80) is needed only
> if the nvdimm is trying to export conventional RAM.
> 
> >
> > One could reuse/share memhotplug GPE._E03 to notify NVDIMM_ROOT
> > but even that is not necessary as NVDIMM has its own QEMU<->guest
> > interface and could just take the next free _E04 handler.
> > So I'd suggest to redo this and 7/8 patches to implement
> > independent (of memhotplug) NVDIMM hotplug as a starting point.  
> 
> Based on ACPI Spec, the memory device (PNP0C80) and nvdimm device
> is notified by the same interruption:
> 
> Scope (\_GPE)
> {
> Method (_L00) {
> Notify (\_SB.NVDR, 0x80) // Notify to NVDIMM root device
> Notify (\_SB.MEM0, 1) // Device Check to Memory Module
> }
> }
> 
> So, it is better to reuse _E03 and only notify NVDIMM device if the
> hotplug event is triggered on a slot with nvdimm device attached?
It's only an example in spec but there isn't requirement to do it that way.
So I'd use a separate handler for now (we can always chain calls in future
is need arises or merge them together)

Re: [Qemu-devel] [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Igor Mammedov]

On Sat, 8 Oct 2016 15:42:41 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 10/03/2016 09:21 PM, Igor Mammedov wrote:
> > On Fri, 12 Aug 2016 14:54:08 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> nvdimm's memory info can not exported via _CRS, instead, it is reported
> >> by NFIT/FIT
> >>
> >> This patch let _CRS return zero for both memory address and memory size
> >> if it is a nvdimm device inserted to the slot  
> > I'm not sure if it's right thing to do.
> > As it's relatively new, spec isn't clear about NVDIMM hotplug
> > process and how it's related to PNP0C80 memory devices.
> >
> > The thing is that notify to PNP0C80 will trigger regular
> > memory hotplug which would expect a valid _CRS and
> > I won't even try to predict reaction of different guests
> > on such behavior.
> >
> > So far exposing NFIT was sufficient for guest to work with
> > NVDIMMs at startup. So I'd assume NVDIMM_ROOT._FIT() would
> > provide sufficient info to hot-plug NVDIMM and allow
> > NVDIMM driver to handle it.
> >
> > The only case of using PNP0C80 with NVDIMM, I'd imagine, is
> > when one would like to expose NVDIMM as conventional RAM and
> > make guest OS use it as such, which is probably not what you'd
> > intended do here.
> >
> > Question is should/could NVDIMM hotplug work without using
> > PNP0C80?  
> 
> Some confusion i got when i was reading ACPI spec. I thing you
> are right that notifying memory device (PNP0C80) is needed only
> if the nvdimm is trying to export conventional RAM.
> 
> >
> > One could reuse/share memhotplug GPE._E03 to notify NVDIMM_ROOT
> > but even that is not necessary as NVDIMM has its own QEMU<->guest
> > interface and could just take the next free _E04 handler.
> > So I'd suggest to redo this and 7/8 patches to implement
> > independent (of memhotplug) NVDIMM hotplug as a starting point.  
> 
> Based on ACPI Spec, the memory device (PNP0C80) and nvdimm device
> is notified by the same interruption:
> 
> Scope (\_GPE)
> {
> Method (_L00) {
> Notify (\_SB.NVDR, 0x80) // Notify to NVDIMM root device
> Notify (\_SB.MEM0, 1) // Device Check to Memory Module
> }
> }
> 
> So, it is better to reuse _E03 and only notify NVDIMM device if the
> hotplug event is triggered on a slot with nvdimm device attached?
It's only an example in spec but there isn't requirement to do it that way.
So I'd use a separate handler for now (we can always chain calls in future
is need arises or merge them together)

Re: [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Xiao Guangrong]

On 10/10/2016 08:47 PM, Igor Mammedov wrote:

>> Based on ACPI Spec, the memory device (PNP0C80) and nvdimm device
>> is notified by the same interruption:
>>
>> Scope (\_GPE)
>> {
>> Method (_L00) {
>> Notify (\_SB.NVDR, 0x80) // Notify to NVDIMM root device
>> Notify (\_SB.MEM0, 1) // Device Check to Memory Module
>> }
>> }
>>
>> So, it is better to reuse _E03 and only notify NVDIMM device if the
>> hotplug event is triggered on a slot with nvdimm device attached?
> It's only an example in spec but there isn't requirement to do it that way.
> So I'd use a separate handler for now (we can always chain calls in future
> is need arises or merge them together)

No objection. Will do it.

Re: [Qemu-devel] [PATCH v2 6/8] pc: memhp: do not export nvdimm's memory via _CRS

[Xiao Guangrong]

On 10/10/2016 08:47 PM, Igor Mammedov wrote:

>> Based on ACPI Spec, the memory device (PNP0C80) and nvdimm device
>> is notified by the same interruption:
>>
>> Scope (\_GPE)
>> {
>> Method (_L00) {
>> Notify (\_SB.NVDR, 0x80) // Notify to NVDIMM root device
>> Notify (\_SB.MEM0, 1) // Device Check to Memory Module
>> }
>> }
>>
>> So, it is better to reuse _E03 and only notify NVDIMM device if the
>> hotplug event is triggered on a slot with nvdimm device attached?
> It's only an example in spec but there isn't requirement to do it that way.
> So I'd use a separate handler for now (we can always chain calls in future
> is need arises or merge them together)

No objection. Will do it.

[PATCH v2 7/8] pc: acpi: memhp: nvdimm hotplug support

[Xiao Guangrong]

It notifies the nvdimm root device if there is a nvdimm device
plugged/unplugged

A new bit is used to indicates it is a nvdimm device

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 docs/specs/acpi_mem_hotplug.txt |  4 +++-
 hw/acpi/memory_hotplug.c        |  5 -----
 hw/i386/acpi-build.c            | 26 ++++++++++++++++++++++----
 hw/mem/nvdimm.c                 |  4 ----
 include/hw/acpi/pc-hotplug.h    |  1 +
 5 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 3df3620..cd9e233 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -21,7 +21,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
                  It's valid only when bit 1 is set.
               2: Device remove event, used to distinguish device for which
                  no device eject request to OSPM was issued.
-              3-7: reserved and should be ignored by OSPM
+              3: reserved and should be ignored by OSPM
+              4: Device is a NVDIMM device.
+              5-7: reserved and should be ignored by OSPM
       [0x15-0x17] reserved
 
   write access:
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 73fa62d..d1c2e92 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -239,11 +239,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
                          DeviceState *dev, Error **errp)
 {
     MemStatus *mdev;
-    DeviceClass *dc = DEVICE_GET_CLASS(dev);
-
-    if (!dc->hotpluggable) {
-        return;
-    }
 
     mdev = acpi_memory_slot_status(mem_st, dev, errp);
     if (!mdev) {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b1d0ced..02cfc4d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1008,9 +1008,10 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
     return crs;
 }
 
-static void build_memory_devices(Aml *sb_scope, int nr_mem,
-                                 uint16_t io_base, uint16_t io_len)
+static void build_memory_devices(Aml *sb_scope, int nr_mem, uint16_t io_base,
+                                 uint16_t io_len, bool enable_nvdimm)
 {
+    #define BASEPATH "\\_SB.PCI0." MEMORY_HOTPLUG_DEVICE "."
     int i;
     Aml *scope;
     Aml *crs;
@@ -1065,6 +1066,12 @@ static void build_memory_devices(Aml *sb_scope, int nr_mem,
     aml_append(field,
         /* initiates device eject, write only */
         aml_named_field(MEMORY_SLOT_EJECT, 1));
+
+    if (enable_nvdimm) {
+        aml_append(field,
+        /* initiates nvdimm device, read only */
+        aml_named_field(MEMORY_SLOT_NVDIMM, 1));
+    }
     aml_append(scope, field);
 
     field = aml_field(MEMORY_HOTPLUG_IO_REGION, AML_DWORD_ACC,
@@ -1079,7 +1086,6 @@ static void build_memory_devices(Aml *sb_scope, int nr_mem,
     aml_append(sb_scope, scope);
 
     for (i = 0; i < nr_mem; i++) {
-        #define BASEPATH "\\_SB.PCI0." MEMORY_HOTPLUG_DEVICE "."
         const char *s;
 
         dev = aml_device("MP%02X", i);
@@ -1124,6 +1130,17 @@ static void build_memory_devices(Aml *sb_scope, int nr_mem,
     method = aml_method(MEMORY_SLOT_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
     for (i = 0; i < nr_mem; i++) {
         ifctx = aml_if(aml_equal(aml_arg(0), aml_int(i)));
+
+        if (enable_nvdimm) {
+            Aml *ifnvdimm;
+
+            ifnvdimm = aml_if(aml_equal(aml_name(BASEPATH MEMORY_SLOT_NVDIMM),
+                                        aml_int(1)));
+            aml_append(ifnvdimm, aml_notify(aml_name("\\_SB.NVDR"),
+                                            aml_int(0x80)));
+            aml_append(ifctx, ifnvdimm);
+        }
+
         aml_append(ifctx,
             aml_notify(aml_name("MP%.02X", i), aml_arg(1))
         );
@@ -2285,7 +2302,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     sb_scope = aml_scope("\\_SB");
     {
         build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
-                             pm->mem_hp_io_len);
+                             pm->mem_hp_io_len,
+                             pcms->acpi_nvdimm_state.is_enabled);
 
         {
             Object *pci_host;
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 5bc81fe..af32508 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -155,13 +155,9 @@ static void nvdimm_prepare_unplug(DeviceState *dev)
 
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(oc);
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
     NVDIMMClass *nvc = NVDIMM_CLASS(oc);
 
-    /* nvdimm hotplug has not been supported yet. */
-    dc->hotpluggable = false;
-
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
     ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
index 6a8d268..a14977d 100644
--- a/include/hw/acpi/pc-hotplug.h
+++ b/include/hw/acpi/pc-hotplug.h
@@ -43,6 +43,7 @@
 #define MEMORY_SLOT_INSERT_EVENT     "MINS"
 #define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
 #define MEMORY_SLOT_EJECT            "MEJ"
+#define MEMORY_SLOT_NVDIMM           "NVDM"
 #define MEMORY_SLOT_SLECTOR          "MSEL"
 #define MEMORY_SLOT_OST_EVENT        "MOEV"
 #define MEMORY_SLOT_OST_STATUS       "MOSC"
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 7/8] pc: acpi: memhp: nvdimm hotplug support

[Xiao Guangrong]

It notifies the nvdimm root device if there is a nvdimm device
plugged/unplugged

A new bit is used to indicates it is a nvdimm device

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 docs/specs/acpi_mem_hotplug.txt |  4 +++-
 hw/acpi/memory_hotplug.c        |  5 -----
 hw/i386/acpi-build.c            | 26 ++++++++++++++++++++++----
 hw/mem/nvdimm.c                 |  4 ----
 include/hw/acpi/pc-hotplug.h    |  1 +
 5 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 3df3620..cd9e233 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -21,7 +21,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
                  It's valid only when bit 1 is set.
               2: Device remove event, used to distinguish device for which
                  no device eject request to OSPM was issued.
-              3-7: reserved and should be ignored by OSPM
+              3: reserved and should be ignored by OSPM
+              4: Device is a NVDIMM device.
+              5-7: reserved and should be ignored by OSPM
       [0x15-0x17] reserved
 
   write access:
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 73fa62d..d1c2e92 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -239,11 +239,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
                          DeviceState *dev, Error **errp)
 {
     MemStatus *mdev;
-    DeviceClass *dc = DEVICE_GET_CLASS(dev);
-
-    if (!dc->hotpluggable) {
-        return;
-    }
 
     mdev = acpi_memory_slot_status(mem_st, dev, errp);
     if (!mdev) {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b1d0ced..02cfc4d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1008,9 +1008,10 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
     return crs;
 }
 
-static void build_memory_devices(Aml *sb_scope, int nr_mem,
-                                 uint16_t io_base, uint16_t io_len)
+static void build_memory_devices(Aml *sb_scope, int nr_mem, uint16_t io_base,
+                                 uint16_t io_len, bool enable_nvdimm)
 {
+    #define BASEPATH "\\_SB.PCI0." MEMORY_HOTPLUG_DEVICE "."
     int i;
     Aml *scope;
     Aml *crs;
@@ -1065,6 +1066,12 @@ static void build_memory_devices(Aml *sb_scope, int nr_mem,
     aml_append(field,
         /* initiates device eject, write only */
         aml_named_field(MEMORY_SLOT_EJECT, 1));
+
+    if (enable_nvdimm) {
+        aml_append(field,
+        /* initiates nvdimm device, read only */
+        aml_named_field(MEMORY_SLOT_NVDIMM, 1));
+    }
     aml_append(scope, field);
 
     field = aml_field(MEMORY_HOTPLUG_IO_REGION, AML_DWORD_ACC,
@@ -1079,7 +1086,6 @@ static void build_memory_devices(Aml *sb_scope, int nr_mem,
     aml_append(sb_scope, scope);
 
     for (i = 0; i < nr_mem; i++) {
-        #define BASEPATH "\\_SB.PCI0." MEMORY_HOTPLUG_DEVICE "."
         const char *s;
 
         dev = aml_device("MP%02X", i);
@@ -1124,6 +1130,17 @@ static void build_memory_devices(Aml *sb_scope, int nr_mem,
     method = aml_method(MEMORY_SLOT_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
     for (i = 0; i < nr_mem; i++) {
         ifctx = aml_if(aml_equal(aml_arg(0), aml_int(i)));
+
+        if (enable_nvdimm) {
+            Aml *ifnvdimm;
+
+            ifnvdimm = aml_if(aml_equal(aml_name(BASEPATH MEMORY_SLOT_NVDIMM),
+                                        aml_int(1)));
+            aml_append(ifnvdimm, aml_notify(aml_name("\\_SB.NVDR"),
+                                            aml_int(0x80)));
+            aml_append(ifctx, ifnvdimm);
+        }
+
         aml_append(ifctx,
             aml_notify(aml_name("MP%.02X", i), aml_arg(1))
         );
@@ -2285,7 +2302,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     sb_scope = aml_scope("\\_SB");
     {
         build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
-                             pm->mem_hp_io_len);
+                             pm->mem_hp_io_len,
+                             pcms->acpi_nvdimm_state.is_enabled);
 
         {
             Object *pci_host;
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 5bc81fe..af32508 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -155,13 +155,9 @@ static void nvdimm_prepare_unplug(DeviceState *dev)
 
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(oc);
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
     NVDIMMClass *nvc = NVDIMM_CLASS(oc);
 
-    /* nvdimm hotplug has not been supported yet. */
-    dc->hotpluggable = false;
-
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
     ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
index 6a8d268..a14977d 100644
--- a/include/hw/acpi/pc-hotplug.h
+++ b/include/hw/acpi/pc-hotplug.h
@@ -43,6 +43,7 @@
 #define MEMORY_SLOT_INSERT_EVENT     "MINS"
 #define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
 #define MEMORY_SLOT_EJECT            "MEJ"
+#define MEMORY_SLOT_NVDIMM           "NVDM"
 #define MEMORY_SLOT_SLECTOR          "MSEL"
 #define MEMORY_SLOT_OST_EVENT        "MOEV"
 #define MEMORY_SLOT_OST_STATUS       "MOSC"
-- 
1.8.3.1

[PATCH v2 8/8] nvdimm docs: add nvdimm Read FIT function

[Xiao Guangrong]

Add the specification of Read FIT function

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 docs/specs/acpi_nvdimm.txt | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 0fdd251..1a31b19 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -127,6 +127,38 @@ _DSM process diagram:
  | result from the page     |      |              |
  +--------------------------+      +--------------+
 
- _FIT implementation
- -------------------
- TODO (will fill it when nvdimm hotplug is introduced)
+ QEMU internal use only _DSM function
+ ------------------------------------
+ There is the function introduced by QEMU and only used by QEMU internal.
+
+ 1) Read FIT
+ As we only reserved one page for NVDIMM ACPI it is impossible to map the
+ whole FIT data to guest's address space. This function is for NVDIMM root
+ device and is used by _FIT method to read a piece of FIT data from QEMU.
+
+ Input parameters:
+ Arg0 – UUID {set to 2f10e7a4-9e91-11e4-89d3-123b93f75cba}
+ Arg1 – Revision ID (set to 1)
+ Arg2 - 0xFFFFFFFF
+ Arg3 - A package containing a buffer whose layout is as follows:
+
+ +----------+-------------+-------------+-----------------------------------+
+ |  Filed   | Byte Length | Byte Offset | Description                       |
+ +----------+-------------+-------------+-----------------------------------+
+ | offset   | 4           | 0           | the offset of FIT buffer          |
+ +----------+-------------+-------------+-----------------------------------+
+
+ Output:
+ +----------+-------------+-------------+-----------------------------------+
+ |  Filed   | Byte Length | Byte Offset | Description                       |
+ +----------+-------------+-------------+-----------------------------------+
+ | status   | 4           | 0           | return status codes following     |
+ |          |             |             | Chapter 3 in DSM Spec Rev1        |
+ +----------+-------------+-------------+-----------------------------------+
+ | fit data | Varies      | 8           | FIT data                          |
+ |          |             |             |                                   |
+ +----------+-------------+-------------+-----------------------------------+
+
+ The FIT offset is maintained by the caller itself, current offset plugs
+ the length returned by the function is the next offset we should read.
+ When all the FIT data has been read out, zero length is returned.
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 8/8] nvdimm docs: add nvdimm Read FIT function

[Xiao Guangrong]

Add the specification of Read FIT function

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 docs/specs/acpi_nvdimm.txt | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 0fdd251..1a31b19 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -127,6 +127,38 @@ _DSM process diagram:
  | result from the page     |      |              |
  +--------------------------+      +--------------+
 
- _FIT implementation
- -------------------
- TODO (will fill it when nvdimm hotplug is introduced)
+ QEMU internal use only _DSM function
+ ------------------------------------
+ There is the function introduced by QEMU and only used by QEMU internal.
+
+ 1) Read FIT
+ As we only reserved one page for NVDIMM ACPI it is impossible to map the
+ whole FIT data to guest's address space. This function is for NVDIMM root
+ device and is used by _FIT method to read a piece of FIT data from QEMU.
+
+ Input parameters:
+ Arg0 – UUID {set to 2f10e7a4-9e91-11e4-89d3-123b93f75cba}
+ Arg1 – Revision ID (set to 1)
+ Arg2 - 0xFFFFFFFF
+ Arg3 - A package containing a buffer whose layout is as follows:
+
+ +----------+-------------+-------------+-----------------------------------+
+ |  Filed   | Byte Length | Byte Offset | Description                       |
+ +----------+-------------+-------------+-----------------------------------+
+ | offset   | 4           | 0           | the offset of FIT buffer          |
+ +----------+-------------+-------------+-----------------------------------+
+
+ Output:
+ +----------+-------------+-------------+-----------------------------------+
+ |  Filed   | Byte Length | Byte Offset | Description                       |
+ +----------+-------------+-------------+-----------------------------------+
+ | status   | 4           | 0           | return status codes following     |
+ |          |             |             | Chapter 3 in DSM Spec Rev1        |
+ +----------+-------------+-------------+-----------------------------------+
+ | fit data | Varies      | 8           | FIT data                          |
+ |          |             |             |                                   |
+ +----------+-------------+-------------+-----------------------------------+
+
+ The FIT offset is maintained by the caller itself, current offset plugs
+ the length returned by the function is the next offset we should read.
+ When all the FIT data has been read out, zero length is returned.
-- 
1.8.3.1

Re: [PATCH v2 8/8] nvdimm docs: add nvdimm Read FIT function

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:10 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Add the specification of Read FIT function
should be squashed into 3/8

> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  docs/specs/acpi_nvdimm.txt | 38 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 0fdd251..1a31b19 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -127,6 +127,38 @@ _DSM process diagram:
>   | result from the page     |      |              |
>   +--------------------------+      +--------------+
>  
> - _FIT implementation
> - -------------------
> - TODO (will fill it when nvdimm hotplug is introduced)
> + QEMU internal use only _DSM function
> + ------------------------------------
> + There is the function introduced by QEMU and only used by QEMU internal.
> +
> + 1) Read FIT
> + As we only reserved one page for NVDIMM ACPI it is impossible to map the
> + whole FIT data to guest's address space. This function is for NVDIMM root
> + device and is used by _FIT method to read a piece of FIT data from QEMU.
> +
> + Input parameters:
> + Arg0 – UUID {set to 2f10e7a4-9e91-11e4-89d3-123b93f75cba}
> + Arg1 – Revision ID (set to 1)
> + Arg2 - 0xFFFFFFFF
> + Arg3 - A package containing a buffer whose layout is as follows:
> +
> + +----------+-------------+-------------+-----------------------------------+
> + |  Filed   | Byte Length | Byte Offset | Description                       |
> + +----------+-------------+-------------+-----------------------------------+
> + | offset   | 4           | 0           | the offset of FIT buffer          |
> + +----------+-------------+-------------+-----------------------------------+
> +
> + Output:
> + +----------+-------------+-------------+-----------------------------------+
> + |  Filed   | Byte Length | Byte Offset | Description                       |
> + +----------+-------------+-------------+-----------------------------------+
> + | status   | 4           | 0           | return status codes following     |
> + |          |             |             | Chapter 3 in DSM Spec Rev1        |
> + +----------+-------------+-------------+-----------------------------------+
> + | fit data | Varies      | 8           | FIT data                          |
> + |          |             |             |                                   |
> + +----------+-------------+-------------+-----------------------------------+
> +
> + The FIT offset is maintained by the caller itself, current offset plugs
> + the length returned by the function is the next offset we should read.
> + When all the FIT data has been read out, zero length is returned.

Re: [Qemu-devel] [PATCH v2 8/8] nvdimm docs: add nvdimm Read FIT function

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:10 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Add the specification of Read FIT function
should be squashed into 3/8

> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  docs/specs/acpi_nvdimm.txt | 38 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 0fdd251..1a31b19 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -127,6 +127,38 @@ _DSM process diagram:
>   | result from the page     |      |              |
>   +--------------------------+      +--------------+
>  
> - _FIT implementation
> - -------------------
> - TODO (will fill it when nvdimm hotplug is introduced)
> + QEMU internal use only _DSM function
> + ------------------------------------
> + There is the function introduced by QEMU and only used by QEMU internal.
> +
> + 1) Read FIT
> + As we only reserved one page for NVDIMM ACPI it is impossible to map the
> + whole FIT data to guest's address space. This function is for NVDIMM root
> + device and is used by _FIT method to read a piece of FIT data from QEMU.
> +
> + Input parameters:
> + Arg0 – UUID {set to 2f10e7a4-9e91-11e4-89d3-123b93f75cba}
> + Arg1 – Revision ID (set to 1)
> + Arg2 - 0xFFFFFFFF
> + Arg3 - A package containing a buffer whose layout is as follows:
> +
> + +----------+-------------+-------------+-----------------------------------+
> + |  Filed   | Byte Length | Byte Offset | Description                       |
> + +----------+-------------+-------------+-----------------------------------+
> + | offset   | 4           | 0           | the offset of FIT buffer          |
> + +----------+-------------+-------------+-----------------------------------+
> +
> + Output:
> + +----------+-------------+-------------+-----------------------------------+
> + |  Filed   | Byte Length | Byte Offset | Description                       |
> + +----------+-------------+-------------+-----------------------------------+
> + | status   | 4           | 0           | return status codes following     |
> + |          |             |             | Chapter 3 in DSM Spec Rev1        |
> + +----------+-------------+-------------+-----------------------------------+
> + | fit data | Varies      | 8           | FIT data                          |
> + |          |             |             |                                   |
> + +----------+-------------+-------------+-----------------------------------+
> +
> + The FIT offset is maintained by the caller itself, current offset plugs
> + the length returned by the function is the next offset we should read.
> + When all the FIT data has been read out, zero length is returned.

Re: [PATCH v2 8/8] nvdimm docs: add nvdimm Read FIT function

[Xiao Guangrong]

On 09/30/2016 10:03 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:10 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> Add the specification of Read FIT function
> should be squashed into 3/8

Okay.

Re: [Qemu-devel] [PATCH v2 8/8] nvdimm docs: add nvdimm Read FIT function

[Xiao Guangrong]

On 09/30/2016 10:03 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:10 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> Add the specification of Read FIT function
> should be squashed into 3/8

Okay.

Re: [PATCH v2 0/8] nvdimm: hotplug support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

On Fri, Aug 12, 2016 at 02:54:02PM +0800, Xiao Guangrong wrote:
> This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
> rx queue siz) on pci branch of Michael's git tree and can be found at:
>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
> 
> Changelog in v2:
>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
> 
> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> for example, a new nvdimm device can be plugged as follows:
> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> device_add nvdimm,id=nvdimm3,memdev=mem3
> 
> and unplug it as follows:
> device_del nvdimm3
> object_del mem3
> 
> Xiao Guangrong (8):
>   acpi nvdimm: fix wrong buffer size returned by DSM method
>   nvdimm acpi: prebuild nvdimm devices for available slots
>   nvdimm acpi: introduce _FIT
>   nvdimm acpi: implement Read FIT function
>   pc-dimm: introduce prepare_unplug() callback
>   pc: memhp: do not export nvdimm's memory via _CRS
>   pc: acpi: memhp: nvdimm hotplug support
>   nvdimm docs: add nvdimm Read FIT function
> 
>  docs/specs/acpi_mem_hotplug.txt  |   4 +-
>  docs/specs/acpi_nvdimm.txt       |  38 +++++++-
>  hw/acpi/ich9.c                   |   3 +
>  hw/acpi/memory_hotplug.c         |  21 +++--
>  hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
>  hw/acpi/piix4.c                  |   3 +
>  hw/i386/acpi-build.c             |  28 +++++-
>  hw/mem/nvdimm.c                  |  12 ++-
>  hw/mem/pc-dimm.c                 |   5 +
>  include/hw/acpi/memory_hotplug.h |   1 +
>  include/hw/acpi/pc-hotplug.h     |   1 +
>  include/hw/mem/nvdimm.h          |   6 +-
>  include/hw/mem/pc-dimm.h         |   1 +
>  13 files changed, 278 insertions(+), 40 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

On Fri, Aug 12, 2016 at 02:54:02PM +0800, Xiao Guangrong wrote:
> This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
> rx queue siz) on pci branch of Michael's git tree and can be found at:
>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
> 
> Changelog in v2:
>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
> 
> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> for example, a new nvdimm device can be plugged as follows:
> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> device_add nvdimm,id=nvdimm3,memdev=mem3
> 
> and unplug it as follows:
> device_del nvdimm3
> object_del mem3
> 
> Xiao Guangrong (8):
>   acpi nvdimm: fix wrong buffer size returned by DSM method
>   nvdimm acpi: prebuild nvdimm devices for available slots
>   nvdimm acpi: introduce _FIT
>   nvdimm acpi: implement Read FIT function
>   pc-dimm: introduce prepare_unplug() callback
>   pc: memhp: do not export nvdimm's memory via _CRS
>   pc: acpi: memhp: nvdimm hotplug support
>   nvdimm docs: add nvdimm Read FIT function
> 
>  docs/specs/acpi_mem_hotplug.txt  |   4 +-
>  docs/specs/acpi_nvdimm.txt       |  38 +++++++-
>  hw/acpi/ich9.c                   |   3 +
>  hw/acpi/memory_hotplug.c         |  21 +++--
>  hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
>  hw/acpi/piix4.c                  |   3 +
>  hw/i386/acpi-build.c             |  28 +++++-
>  hw/mem/nvdimm.c                  |  12 ++-
>  hw/mem/pc-dimm.c                 |   5 +
>  include/hw/acpi/memory_hotplug.h |   1 +
>  include/hw/acpi/pc-hotplug.h     |   1 +
>  include/hw/mem/nvdimm.h          |   6 +-
>  include/hw/mem/pc-dimm.h         |   1 +
>  13 files changed, 278 insertions(+), 40 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 0/8] nvdimm: hotplug support

[Igor Mammedov]

On Fri, 12 Aug 2016 09:35:12 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Fri, Aug 12, 2016 at 02:54:02PM +0800, Xiao Guangrong wrote:
> > This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
> > rx queue siz) on pci branch of Michael's git tree and can be found at:
> >       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
> > 
> > Changelog in v2:
> >    Fixed signed integer overflow pointed out by Stefan Hajnoczi
> > 
> > This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> > for example, a new nvdimm device can be plugged as follows:
> > object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> > device_add nvdimm,id=nvdimm3,memdev=mem3
> > 
> > and unplug it as follows:
> > device_del nvdimm3
> > object_del mem3
> > 
> > Xiao Guangrong (8):
> >   acpi nvdimm: fix wrong buffer size returned by DSM method
> >   nvdimm acpi: prebuild nvdimm devices for available slots
> >   nvdimm acpi: introduce _FIT
> >   nvdimm acpi: implement Read FIT function
> >   pc-dimm: introduce prepare_unplug() callback
> >   pc: memhp: do not export nvdimm's memory via _CRS
> >   pc: acpi: memhp: nvdimm hotplug support
> >   nvdimm docs: add nvdimm Read FIT function
> > 
> >  docs/specs/acpi_mem_hotplug.txt  |   4 +-
> >  docs/specs/acpi_nvdimm.txt       |  38 +++++++-
> >  hw/acpi/ich9.c                   |   3 +
> >  hw/acpi/memory_hotplug.c         |  21 +++--
> >  hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
> >  hw/acpi/piix4.c                  |   3 +
> >  hw/i386/acpi-build.c             |  28 +++++-
> >  hw/mem/nvdimm.c                  |  12 ++-
> >  hw/mem/pc-dimm.c                 |   5 +
> >  include/hw/acpi/memory_hotplug.h |   1 +
> >  include/hw/acpi/pc-hotplug.h     |   1 +
> >  include/hw/mem/nvdimm.h          |   6 +-
> >  include/hw/mem/pc-dimm.h         |   1 +
> >  13 files changed, 278 insertions(+), 40 deletions(-)
> > 
> > -- 
> > 1.8.3.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
I'd like to review it but I need to read NVDIMM/ACPI specs first
to make sensible comments.

However it will take some time and I'm on vacation starting next week
and I'll be back in a month. So please don't apply this series until
I'm back and have a chance to review it.

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Igor Mammedov]

On Fri, 12 Aug 2016 09:35:12 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Fri, Aug 12, 2016 at 02:54:02PM +0800, Xiao Guangrong wrote:
> > This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
> > rx queue siz) on pci branch of Michael's git tree and can be found at:
> >       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
> > 
> > Changelog in v2:
> >    Fixed signed integer overflow pointed out by Stefan Hajnoczi
> > 
> > This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> > for example, a new nvdimm device can be plugged as follows:
> > object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> > device_add nvdimm,id=nvdimm3,memdev=mem3
> > 
> > and unplug it as follows:
> > device_del nvdimm3
> > object_del mem3
> > 
> > Xiao Guangrong (8):
> >   acpi nvdimm: fix wrong buffer size returned by DSM method
> >   nvdimm acpi: prebuild nvdimm devices for available slots
> >   nvdimm acpi: introduce _FIT
> >   nvdimm acpi: implement Read FIT function
> >   pc-dimm: introduce prepare_unplug() callback
> >   pc: memhp: do not export nvdimm's memory via _CRS
> >   pc: acpi: memhp: nvdimm hotplug support
> >   nvdimm docs: add nvdimm Read FIT function
> > 
> >  docs/specs/acpi_mem_hotplug.txt  |   4 +-
> >  docs/specs/acpi_nvdimm.txt       |  38 +++++++-
> >  hw/acpi/ich9.c                   |   3 +
> >  hw/acpi/memory_hotplug.c         |  21 +++--
> >  hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
> >  hw/acpi/piix4.c                  |   3 +
> >  hw/i386/acpi-build.c             |  28 +++++-
> >  hw/mem/nvdimm.c                  |  12 ++-
> >  hw/mem/pc-dimm.c                 |   5 +
> >  include/hw/acpi/memory_hotplug.h |   1 +
> >  include/hw/acpi/pc-hotplug.h     |   1 +
> >  include/hw/mem/nvdimm.h          |   6 +-
> >  include/hw/mem/pc-dimm.h         |   1 +
> >  13 files changed, 278 insertions(+), 40 deletions(-)
> > 
> > -- 
> > 1.8.3.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
I'd like to review it but I need to read NVDIMM/ACPI specs first
to make sensible comments.

However it will take some time and I'm on vacation starting next week
and I'll be back in a month. So please don't apply this series until
I'm back and have a chance to review it.

Re: [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 08/12/2016 11:13 PM, Igor Mammedov wrote:

>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> I'd like to review it but I need to read NVDIMM/ACPI specs first
> to make sensible comments.
>
> However it will take some time and I'm on vacation starting next week
> and I'll be back in a month. So please don't apply this series until
> I'm back and have a chance to review it.
>

Ping... :)

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 08/12/2016 11:13 PM, Igor Mammedov wrote:

>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> I'd like to review it but I need to read NVDIMM/ACPI specs first
> to make sensible comments.
>
> However it will take some time and I'm on vacation starting next week
> and I'll be back in a month. So please don't apply this series until
> I'm back and have a chance to review it.
>

Ping... :)

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Igor Mammedov]

On Mon, 12 Sep 2016 15:33:53 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 08/12/2016 11:13 PM, Igor Mammedov wrote:
> 
> >>
> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>  
> > I'd like to review it but I need to read NVDIMM/ACPI specs first
> > to make sensible comments.
> >
> > However it will take some time and I'm on vacation starting next week
> > and I'll be back in a month. So please don't apply this series until
> > I'm back and have a chance to review it.
> >  
> 
> Ping... :)
I've put it on my TODO list
and I'll look at it after I deal with a month worth mail backlog

Re: [PATCH v2 0/8] nvdimm: hotplug support

[Dan Williams]

[ adding Vishal who implemented the kernel side of nvdimm hotplug support ]

On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
<guangrong.xiao@linux.intel.com> wrote:
> This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
> rx queue siz) on pci branch of Michael's git tree and can be found at:
>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
>
> Changelog in v2:
>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
>
> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> for example, a new nvdimm device can be plugged as follows:
> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> device_add nvdimm,id=nvdimm3,memdev=mem3
>
> and unplug it as follows:
> device_del nvdimm3
> object_del mem3

Did you test this against the Linux NFIT hotplug support?  We just
found that the Linux driver is not properly registering for ACPI0012
event notification.  Is a notification sent on a 'device_add' event?

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Dan Williams]

[ adding Vishal who implemented the kernel side of nvdimm hotplug support ]

On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
<guangrong.xiao@linux.intel.com> wrote:
> This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
> rx queue siz) on pci branch of Michael's git tree and can be found at:
>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
>
> Changelog in v2:
>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
>
> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> for example, a new nvdimm device can be plugged as follows:
> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> device_add nvdimm,id=nvdimm3,memdev=mem3
>
> and unplug it as follows:
> device_del nvdimm3
> object_del mem3

Did you test this against the Linux NFIT hotplug support?  We just
found that the Linux driver is not properly registering for ACPI0012
event notification.  Is a notification sent on a 'device_add' event?

Re: [PATCH v2 0/8] nvdimm: hotplug support

[Vishal Verma]

On 08/18, Dan Williams wrote:
> [ adding Vishal who implemented the kernel side of nvdimm hotplug support ]
> 
> On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
> <guangrong.xiao@linux.intel.com> wrote:
> > This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
> > rx queue siz) on pci branch of Michael's git tree and can be found at:
> >       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
> >
> > Changelog in v2:
> >    Fixed signed integer overflow pointed out by Stefan Hajnoczi
> >
> > This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> > for example, a new nvdimm device can be plugged as follows:
> > object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> > device_add nvdimm,id=nvdimm3,memdev=mem3
> >
> > and unplug it as follows:
> > device_del nvdimm3
> > object_del mem3
> 
> Did you test this against the Linux NFIT hotplug support?  We just
> found that the Linux driver is not properly registering for ACPI0012
> event notification.  Is a notification sent on a 'device_add' event?

I've just sent out a patch that should fix this:
https://lists.01.org/pipermail/linux-nvdimm/2016-August/006637.html

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Vishal Verma]

On 08/18, Dan Williams wrote:
> [ adding Vishal who implemented the kernel side of nvdimm hotplug support ]
> 
> On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
> <guangrong.xiao@linux.intel.com> wrote:
> > This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
> > rx queue siz) on pci branch of Michael's git tree and can be found at:
> >       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
> >
> > Changelog in v2:
> >    Fixed signed integer overflow pointed out by Stefan Hajnoczi
> >
> > This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> > for example, a new nvdimm device can be plugged as follows:
> > object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> > device_add nvdimm,id=nvdimm3,memdev=mem3
> >
> > and unplug it as follows:
> > device_del nvdimm3
> > object_del mem3
> 
> Did you test this against the Linux NFIT hotplug support?  We just
> found that the Linux driver is not properly registering for ACPI0012
> event notification.  Is a notification sent on a 'device_add' event?

I've just sent out a patch that should fix this:
https://lists.01.org/pipermail/linux-nvdimm/2016-August/006637.html

Re: [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 08/19/2016 02:54 AM, Vishal Verma wrote:
> On 08/18, Dan Williams wrote:
>> [ adding Vishal who implemented the kernel side of nvdimm hotplug support ]
>>
>> On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
>> <guangrong.xiao@linux.intel.com> wrote:
>>> This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
>>> rx queue siz) on pci branch of Michael's git tree and can be found at:
>>>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
>>>
>>> Changelog in v2:
>>>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
>>>
>>> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
>>> for example, a new nvdimm device can be plugged as follows:
>>> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
>>> device_add nvdimm,id=nvdimm3,memdev=mem3
>>>
>>> and unplug it as follows:
>>> device_del nvdimm3
>>> object_del mem3
>>
>> Did you test this against the Linux NFIT hotplug support?  We just
>> found that the Linux driver is not properly registering for ACPI0012
>> event notification.  Is a notification sent on a 'device_add' event?
>
> I've just sent out a patch that should fix this:
> https://lists.01.org/pipermail/linux-nvdimm/2016-August/006637.html
>

Interesting. I am using the kvm tree, queue branch, the top commit is
8ff7b956471f:
   Merge tag 'kvm-s390-next-4.8-2' of git://git.kernel.org/pub/scm/linux/kernel

It works.

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 08/19/2016 02:54 AM, Vishal Verma wrote:
> On 08/18, Dan Williams wrote:
>> [ adding Vishal who implemented the kernel side of nvdimm hotplug support ]
>>
>> On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
>> <guangrong.xiao@linux.intel.com> wrote:
>>> This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
>>> rx queue siz) on pci branch of Michael's git tree and can be found at:
>>>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
>>>
>>> Changelog in v2:
>>>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
>>>
>>> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
>>> for example, a new nvdimm device can be plugged as follows:
>>> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
>>> device_add nvdimm,id=nvdimm3,memdev=mem3
>>>
>>> and unplug it as follows:
>>> device_del nvdimm3
>>> object_del mem3
>>
>> Did you test this against the Linux NFIT hotplug support?  We just
>> found that the Linux driver is not properly registering for ACPI0012
>> event notification.  Is a notification sent on a 'device_add' event?
>
> I've just sent out a patch that should fix this:
> https://lists.01.org/pipermail/linux-nvdimm/2016-August/006637.html
>

Interesting. I am using the kvm tree, queue branch, the top commit is
8ff7b956471f:
   Merge tag 'kvm-s390-next-4.8-2' of git://git.kernel.org/pub/scm/linux/kernel

It works.

Re: [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 08/19/2016 11:40 AM, Xiao Guangrong wrote:
>
>
> On 08/19/2016 02:54 AM, Vishal Verma wrote:
>> On 08/18, Dan Williams wrote:
>>> [ adding Vishal who implemented the kernel side of nvdimm hotplug support ]
>>>
>>> On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
>>> <guangrong.xiao@linux.intel.com> wrote:
>>>> This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
>>>> rx queue siz) on pci branch of Michael's git tree and can be found at:
>>>>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
>>>>
>>>> Changelog in v2:
>>>>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
>>>>
>>>> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
>>>> for example, a new nvdimm device can be plugged as follows:
>>>> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
>>>> device_add nvdimm,id=nvdimm3,memdev=mem3
>>>>
>>>> and unplug it as follows:
>>>> device_del nvdimm3
>>>> object_del mem3
>>>
>>> Did you test this against the Linux NFIT hotplug support?  We just
>>> found that the Linux driver is not properly registering for ACPI0012
>>> event notification.  Is a notification sent on a 'device_add' event?
>>
>> I've just sent out a patch that should fix this:
>> https://lists.01.org/pipermail/linux-nvdimm/2016-August/006637.html
>>
>
> Interesting. I am using the kvm tree, queue branch, the top commit is
> 8ff7b956471f:
>   Merge tag 'kvm-s390-next-4.8-2' of git://git.kernel.org/pub/scm/linux/kernel
>
> It works.

It triggers 'notify' event, not 'device add'.

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 08/19/2016 11:40 AM, Xiao Guangrong wrote:
>
>
> On 08/19/2016 02:54 AM, Vishal Verma wrote:
>> On 08/18, Dan Williams wrote:
>>> [ adding Vishal who implemented the kernel side of nvdimm hotplug support ]
>>>
>>> On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
>>> <guangrong.xiao@linux.intel.com> wrote:
>>>> This patchset is against commit c597dc90fbcd6 (virtio-net: allow increasing
>>>> rx queue siz) on pci branch of Michael's git tree and can be found at:
>>>>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
>>>>
>>>> Changelog in v2:
>>>>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
>>>>
>>>> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
>>>> for example, a new nvdimm device can be plugged as follows:
>>>> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
>>>> device_add nvdimm,id=nvdimm3,memdev=mem3
>>>>
>>>> and unplug it as follows:
>>>> device_del nvdimm3
>>>> object_del mem3
>>>
>>> Did you test this against the Linux NFIT hotplug support?  We just
>>> found that the Linux driver is not properly registering for ACPI0012
>>> event notification.  Is a notification sent on a 'device_add' event?
>>
>> I've just sent out a patch that should fix this:
>> https://lists.01.org/pipermail/linux-nvdimm/2016-August/006637.html
>>
>
> Interesting. I am using the kvm tree, queue branch, the top commit is
> 8ff7b956471f:
>   Merge tag 'kvm-s390-next-4.8-2' of git://git.kernel.org/pub/scm/linux/kernel
>
> It works.

It triggers 'notify' event, not 'device add'.

Re: [PATCH v2 0/8] nvdimm: hotplug support

[Dan Williams]

On Thu, Aug 18, 2016 at 8:46 PM, Xiao Guangrong
<guangrong.xiao@linux.intel.com> wrote:
>
>
> On 08/19/2016 11:40 AM, Xiao Guangrong wrote:
>>
>>
>>
>> On 08/19/2016 02:54 AM, Vishal Verma wrote:
>>>
>>> On 08/18, Dan Williams wrote:
>>>>
>>>> [ adding Vishal who implemented the kernel side of nvdimm hotplug
>>>> support ]
>>>>
>>>> On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
>>>> <guangrong.xiao@linux.intel.com> wrote:
>>>>>
>>>>> This patchset is against commit c597dc90fbcd6 (virtio-net: allow
>>>>> increasing
>>>>> rx queue siz) on pci branch of Michael's git tree and can be found at:
>>>>>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
>>>>>
>>>>> Changelog in v2:
>>>>>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
>>>>>
>>>>> This patchset enables nvdimm hotplug support, it is used as pc-dimm
>>>>> hotplug,
>>>>> for example, a new nvdimm device can be plugged as follows:
>>>>> object_add
>>>>> memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
>>>>> device_add nvdimm,id=nvdimm3,memdev=mem3
>>>>>
>>>>> and unplug it as follows:
>>>>> device_del nvdimm3
>>>>> object_del mem3
>>>>
>>>>
>>>> Did you test this against the Linux NFIT hotplug support?  We just
>>>> found that the Linux driver is not properly registering for ACPI0012
>>>> event notification.  Is a notification sent on a 'device_add' event?
>>>
>>>
>>> I've just sent out a patch that should fix this:
>>> https://lists.01.org/pipermail/linux-nvdimm/2016-August/006637.html
>>>
>>
>> Interesting. I am using the kvm tree, queue branch, the top commit is
>> 8ff7b956471f:
>>   Merge tag 'kvm-s390-next-4.8-2' of
>> git://git.kernel.org/pub/scm/linux/kernel
>>
>> It works.
>
>
> It triggers 'notify' event, not 'device add'.

Ah, I missed that the notification handler gets registered via
acpi_device_install_notify_handler() so this
ACPI_DRIVER_ALL_NOTIFY_EVENTS is not needed.

Thank you for confirming.

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Dan Williams]

On Thu, Aug 18, 2016 at 8:46 PM, Xiao Guangrong
<guangrong.xiao@linux.intel.com> wrote:
>
>
> On 08/19/2016 11:40 AM, Xiao Guangrong wrote:
>>
>>
>>
>> On 08/19/2016 02:54 AM, Vishal Verma wrote:
>>>
>>> On 08/18, Dan Williams wrote:
>>>>
>>>> [ adding Vishal who implemented the kernel side of nvdimm hotplug
>>>> support ]
>>>>
>>>> On Thu, Aug 11, 2016 at 11:54 PM, Xiao Guangrong
>>>> <guangrong.xiao@linux.intel.com> wrote:
>>>>>
>>>>> This patchset is against commit c597dc90fbcd6 (virtio-net: allow
>>>>> increasing
>>>>> rx queue siz) on pci branch of Michael's git tree and can be found at:
>>>>>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v2
>>>>>
>>>>> Changelog in v2:
>>>>>    Fixed signed integer overflow pointed out by Stefan Hajnoczi
>>>>>
>>>>> This patchset enables nvdimm hotplug support, it is used as pc-dimm
>>>>> hotplug,
>>>>> for example, a new nvdimm device can be plugged as follows:
>>>>> object_add
>>>>> memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
>>>>> device_add nvdimm,id=nvdimm3,memdev=mem3
>>>>>
>>>>> and unplug it as follows:
>>>>> device_del nvdimm3
>>>>> object_del mem3
>>>>
>>>>
>>>> Did you test this against the Linux NFIT hotplug support?  We just
>>>> found that the Linux driver is not properly registering for ACPI0012
>>>> event notification.  Is a notification sent on a 'device_add' event?
>>>
>>>
>>> I've just sent out a patch that should fix this:
>>> https://lists.01.org/pipermail/linux-nvdimm/2016-August/006637.html
>>>
>>
>> Interesting. I am using the kvm tree, queue branch, the top commit is
>> 8ff7b956471f:
>>   Merge tag 'kvm-s390-next-4.8-2' of
>> git://git.kernel.org/pub/scm/linux/kernel
>>
>> It works.
>
>
> It triggers 'notify' event, not 'device add'.

Ah, I missed that the notification handler gets registered via
acpi_device_install_notify_handler() so this
ACPI_DRIVER_ALL_NOTIFY_EVENTS is not needed.

Thank you for confirming.

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:02 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

General design issue in this series is regenerating
_FIT data every time inside of _FIT read loop.

The issue here is that if FIT data doesn't fit in one page
RFIT would be called several times resulting in calling
nvdimm_dsm_func_read_fit() N-times, and in between 
these N exits generated FIT data could change
(for example if another NVDIMM has been hotpluged in between)
resulting in corrupted concatenated FIT buffer returned by
_FIT method.
So one need either to block follow up hotplug or make sure
that FIT data regenerated/updated only once per _FIT
invocation and stay immutable until _FIT is completed
(maybe RCU protected buffer).

Regenerating/updating inside qemu could also be shared
with NFIT table creation path so that both cold/hotplug
would reuse the same data which are updated only when
a NVDIMM is added/removed.
---
I guess I'm done with v2 review at this point.

PS:
Not related to this series but still existing NVDIMM
codebase issues:

1:
OperationRegion (NRAM, SystemMemory, MEMA, 0x1000)
...
Name (MEMA, 0x7FFFF000)

is not valid ASL, and most certainly would make Windows BSOD.

Check spec for 
 RegionOffset := TermArg => Integer

Named object is not a TermArg.
I'd suggest to make that OperationRegion dynamic i.e
put its definition into sole user NCAL() and use

Store(MEMA, LocalX)
OperationRegion (NRAM, SystemMemory, LocalX, 0x1000)


2:
Field (NRAM, DWordAcc, NoLock, Preserve)
{
   ...
   ARG3,   32672
}
...
NCAL()
...
Store (Local3, ARG3) /* \_SB_.NVDR.ARG3 */

Using ARG3 name is confusing at best and is wrong as ARG3
is reserved name and probably it won't compile back to valid AML.

Suggest s/ARG3/FARG/
with comment at declaration point
/* Package that contains function-specific arguments _DSM(..., Arg3) */

3:
Method (NCAL, 5, Serialized) {
...
Concatenate (Buffer (Zero) {}, OBUF, Arg6)
Return (Arg6)

it's wrong to use Arg6 here as function has only 5 arguments,
use LocalX instead.


4:
if method creates/access named fields it should be serialized.

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 10/03/2016 09:48 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:02 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
> General design issue in this series is regenerating
> _FIT data every time inside of _FIT read loop.
>
> The issue here is that if FIT data doesn't fit in one page
> RFIT would be called several times resulting in calling
> nvdimm_dsm_func_read_fit() N-times, and in between
> these N exits generated FIT data could change
> (for example if another NVDIMM has been hotpluged in between)
> resulting in corrupted concatenated FIT buffer returned by
> _FIT method.
> So one need either to block follow up hotplug or make sure
> that FIT data regenerated/updated only once per _FIT
> invocation and stay immutable until _FIT is completed
> (maybe RCU protected buffer).
>
> Regenerating/updating inside qemu could also be shared
> with NFIT table creation path so that both cold/hotplug
> would reuse the same data which are updated only when
> a NVDIMM is added/removed.

Explicitly sync up QEMU and OSPM is hard as OSPM is running
in a VM which is vulnerable and can not be expected always
triggering correct sequence.

So how about introducing generation-number which is updated
for each hotplug event and a new DSM function reserved to
get this number by OSPM. Then the ACPI code is like this:

restart:
    old-number = DSM(Read_NUM);
    FIT = DSM(FIT)
    new-number = DSM(Read_NUM);

    if (old-number != new-number)
       goto restart;

    return FIT

> ---
> I guess I'm done with v2 review at this point.

It is a hard work to review ACPI changes. Thank you, Igor!

>
> PS:
> Not related to this series but still existing NVDIMM
> codebase issues:
>
> 1:
> OperationRegion (NRAM, SystemMemory, MEMA, 0x1000)
> ...
> Name (MEMA, 0x7FFFF000)
>
> is not valid ASL, and most certainly would make Windows BSOD.

Er, i tried windows guests which do not trigger the issue.

>
> Check spec for
>  RegionOffset := TermArg => Integer
>
> Named object is not a TermArg.
> I'd suggest to make that OperationRegion dynamic i.e
> put its definition into sole user NCAL() and use
>
> Store(MEMA, LocalX)
> OperationRegion (NRAM, SystemMemory, LocalX, 0x1000)
>

You are right, will fix it.

>
> 2:
> Field (NRAM, DWordAcc, NoLock, Preserve)
> {
>    ...
>    ARG3,   32672
> }
> ...
> NCAL()
> ...
> Store (Local3, ARG3) /* \_SB_.NVDR.ARG3 */
>
> Using ARG3 name is confusing at best and is wrong as ARG3
> is reserved name and probably it won't compile back to valid AML.
>
> Suggest s/ARG3/FARG/
> with comment at declaration point
> /* Package that contains function-specific arguments _DSM(..., Arg3) */
>

Good to me, will fix.

> 3:
> Method (NCAL, 5, Serialized) {
> ...
> Concatenate (Buffer (Zero) {}, OBUF, Arg6)
> Return (Arg6)
>
> it's wrong to use Arg6 here as function has only 5 arguments,
> use LocalX instead.
>

Will fix it too.

>
> 4:
> if method creates/access named fields it should be serialized.
>

Yup, will pay more attention on it.

Thanks!

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Igor Mammedov]

On Sat, 8 Oct 2016 16:34:06 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 10/03/2016 09:48 PM, Igor Mammedov wrote:
> > On Fri, 12 Aug 2016 14:54:02 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> > General design issue in this series is regenerating
> > _FIT data every time inside of _FIT read loop.
> >
> > The issue here is that if FIT data doesn't fit in one page
> > RFIT would be called several times resulting in calling
> > nvdimm_dsm_func_read_fit() N-times, and in between
> > these N exits generated FIT data could change
> > (for example if another NVDIMM has been hotpluged in between)
> > resulting in corrupted concatenated FIT buffer returned by
> > _FIT method.
> > So one need either to block follow up hotplug or make sure
> > that FIT data regenerated/updated only once per _FIT
> > invocation and stay immutable until _FIT is completed
> > (maybe RCU protected buffer).
> >


> > Regenerating/updating inside qemu could also be shared
> > with NFIT table creation path so that both cold/hotplug
> > would reuse the same data which are updated only when
> > a NVDIMM is added/removed.  
try to take into account this point so that FIT data
aren't regenerated on every RFIT invocation.


> Explicitly sync up QEMU and OSPM is hard as OSPM is running
> in a VM which is vulnerable and can not be expected always
> triggering correct sequence.
> 
> So how about introducing generation-number which is updated
> for each hotplug event and a new DSM function reserved to
> get this number by OSPM. Then the ACPI code is like this:
> 
> restart:
>     old-number = DSM(Read_NUM);
>     FIT = DSM(FIT)
>     new-number = DSM(Read_NUM);
> 
>     if (old-number != new-number)
>        goto restart;
> 
>     return FIT
It's a bit complicated.

How about returning
 func_ret_status = FAIL_FIT_CHANGED
if current FIT data changed underfoot and restarting.

> 
> > ---
> > I guess I'm done with v2 review at this point.  
> 
> It is a hard work to review ACPI changes. Thank you, Igor!
> 
> >
> > PS:
> > Not related to this series but still existing NVDIMM
> > codebase issues:
> >
> > 1:
> > OperationRegion (NRAM, SystemMemory, MEMA, 0x1000)
> > ...
> > Name (MEMA, 0x7FFFF000)
> >
> > is not valid ASL, and most certainly would make Windows BSOD.  
> 
> Er, i tried windows guests which do not trigger the issue.
> 
> >
> > Check spec for
> >  RegionOffset := TermArg => Integer
> >
> > Named object is not a TermArg.
> > I'd suggest to make that OperationRegion dynamic i.e
> > put its definition into sole user NCAL() and use
> >
> > Store(MEMA, LocalX)
> > OperationRegion (NRAM, SystemMemory, LocalX, 0x1000)
> >  
> 
> You are right, will fix it.
> 
> >
> > 2:
> > Field (NRAM, DWordAcc, NoLock, Preserve)
> > {
> >    ...
> >    ARG3,   32672
> > }
> > ...
> > NCAL()
> > ...
> > Store (Local3, ARG3) /* \_SB_.NVDR.ARG3 */
> >
> > Using ARG3 name is confusing at best and is wrong as ARG3
> > is reserved name and probably it won't compile back to valid AML.
> >
> > Suggest s/ARG3/FARG/
> > with comment at declaration point
> > /* Package that contains function-specific arguments _DSM(..., Arg3) */
> >  
> 
> Good to me, will fix.
> 
> > 3:
> > Method (NCAL, 5, Serialized) {
> > ...
> > Concatenate (Buffer (Zero) {}, OBUF, Arg6)
> > Return (Arg6)
> >
> > it's wrong to use Arg6 here as function has only 5 arguments,
> > use LocalX instead.
> >  
> 
> Will fix it too.
> 
> >
> > 4:
> > if method creates/access named fields it should be serialized.
> >  
> 
> Yup, will pay more attention on it.
> 
> Thanks!
> 

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 10/10/2016 08:59 PM, Igor Mammedov wrote:
> On Sat, 8 Oct 2016 16:34:06 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> On 10/03/2016 09:48 PM, Igor Mammedov wrote:
>>> On Fri, 12 Aug 2016 14:54:02 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>> General design issue in this series is regenerating
>>> _FIT data every time inside of _FIT read loop.
>>>
>>> The issue here is that if FIT data doesn't fit in one page
>>> RFIT would be called several times resulting in calling
>>> nvdimm_dsm_func_read_fit() N-times, and in between
>>> these N exits generated FIT data could change
>>> (for example if another NVDIMM has been hotpluged in between)
>>> resulting in corrupted concatenated FIT buffer returned by
>>> _FIT method.
>>> So one need either to block follow up hotplug or make sure
>>> that FIT data regenerated/updated only once per _FIT
>>> invocation and stay immutable until _FIT is completed
>>> (maybe RCU protected buffer).
>>>
>
>
>>> Regenerating/updating inside qemu could also be shared
>>> with NFIT table creation path so that both cold/hotplug
>>> would reuse the same data which are updated only when
>>> a NVDIMM is added/removed.
> try to take into account this point so that FIT data
> aren't regenerated on every RFIT invocation.

Okay.

The reason that the FIT was always regenerated is avoiding saving
FIT during live-migration. Well, it seems that we are hard to
avoid it now. :)

>
>
>> Explicitly sync up QEMU and OSPM is hard as OSPM is running
>> in a VM which is vulnerable and can not be expected always
>> triggering correct sequence.
>>
>> So how about introducing generation-number which is updated
>> for each hotplug event and a new DSM function reserved to
>> get this number by OSPM. Then the ACPI code is like this:
>>
>> restart:
>>     old-number = DSM(Read_NUM);
>>     FIT = DSM(FIT)
>>     new-number = DSM(Read_NUM);
>>
>>     if (old-number != new-number)
>>        goto restart;
>>
>>     return FIT
> It's a bit complicated.
>

Hmm, how about this algorithm:

struct fit_data {
	u64 generation_number;
	GArray *fit;
};

struct fit_data *fit;
u64 current_number;

The update-side (hotplug happens):
   new_fit = malloc();

   old_fit = rcu_dereference(fit);

   new_fit->generation_number = old_fit->generation_number + 1;
   new_fit->fit = ...generate-fit...;

   rcu_assign(fit, new_fit);
   sync_rcu()
   free(old_fit);


On the read-side (DSM issued by OSPM):
{
    rcu_read_lock()

    fit = rcu_dereference(fit);

    /* if it is the first time issuing RFIT. */
    if (fit-read-position == 0)
      current_number = fit->generation_number;
    else if (current_number != fit->generation_number)
      return FAIL_FIT_CHANGED;

    ... fill returned info with fit->fit...;

    rcu_read_unlock()
}

Of course, @fit and @current_number should be persistent during live migration.

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Igor Mammedov]

On Mon, 10 Oct 2016 21:57:55 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 10/10/2016 08:59 PM, Igor Mammedov wrote:
> > On Sat, 8 Oct 2016 16:34:06 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> On 10/03/2016 09:48 PM, Igor Mammedov wrote:  
> >>> On Fri, 12 Aug 2016 14:54:02 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>
> >>> General design issue in this series is regenerating
> >>> _FIT data every time inside of _FIT read loop.
> >>>
> >>> The issue here is that if FIT data doesn't fit in one page
> >>> RFIT would be called several times resulting in calling
> >>> nvdimm_dsm_func_read_fit() N-times, and in between
> >>> these N exits generated FIT data could change
> >>> (for example if another NVDIMM has been hotpluged in between)
> >>> resulting in corrupted concatenated FIT buffer returned by
> >>> _FIT method.
> >>> So one need either to block follow up hotplug or make sure
> >>> that FIT data regenerated/updated only once per _FIT
> >>> invocation and stay immutable until _FIT is completed
> >>> (maybe RCU protected buffer).
> >>>  
> >
> >  
> >>> Regenerating/updating inside qemu could also be shared
> >>> with NFIT table creation path so that both cold/hotplug
> >>> would reuse the same data which are updated only when
> >>> a NVDIMM is added/removed.  
> > try to take into account this point so that FIT data
> > aren't regenerated on every RFIT invocation.  
> 
> Okay.
> 
> The reason that the FIT was always regenerated is avoiding saving
> FIT during live-migration. Well, it seems that we are hard to
> avoid it now. :)
> 
> >
> >  
> >> Explicitly sync up QEMU and OSPM is hard as OSPM is running
> >> in a VM which is vulnerable and can not be expected always
> >> triggering correct sequence.
> >>
> >> So how about introducing generation-number which is updated
> >> for each hotplug event and a new DSM function reserved to
> >> get this number by OSPM. Then the ACPI code is like this:
> >>
> >> restart:
> >>     old-number = DSM(Read_NUM);
> >>     FIT = DSM(FIT)
> >>     new-number = DSM(Read_NUM);
> >>
> >>     if (old-number != new-number)
> >>        goto restart;
> >>
> >>     return FIT  
> > It's a bit complicated.
> >  
> 
> Hmm, how about this algorithm:
> 
> struct fit_data {
> 	u64 generation_number;
> 	GArray *fit;
> };
> 
> struct fit_data *fit;
> u64 current_number;
> 
> The update-side (hotplug happens):
>    new_fit = malloc();
> 
>    old_fit = rcu_dereference(fit);
> 
>    new_fit->generation_number = old_fit->generation_number + 1;
>    new_fit->fit = ...generate-fit...;
> 
>    rcu_assign(fit, new_fit);
>    sync_rcu()
>    free(old_fit);
> 
> 
> On the read-side (DSM issued by OSPM):
> {
>     rcu_read_lock()
> 
>     fit = rcu_dereference(fit);
> 
>     /* if it is the first time issuing RFIT. */
>     if (fit-read-position == 0)
>       current_number = fit->generation_number;
>     else if (current_number != fit->generation_number)
>       return FAIL_FIT_CHANGED;
> 
>     ... fill returned info with fit->fit...;
> 
>     rcu_read_unlock()
> }
> 
> Of course, @fit and @current_number should be persistent during live migration.
you can drop RCU and @current_number,
and @fit could be exactly recreated on target side from
 -device nvdim ...
set of options, which must include all currently present
(including hotplugged) devices from source side.

It's sufficient to invalidate and restart DMA transfer in flight.

i.e.
pc_dimm_memory_plug ()
  -> regenerate_fit()
      -> set local flag @fit-dirty

  -> start QEMU.fit_read()
      -> if offset == 0 ? clear @fit-dirty
      make sure fit_read() won't conflict with regenerate_fit()
        either plain mutex or RCU would do the job

  ...
  OSPM.rfit()
     ...
     if (@fit-dirty)
         abort/restart DMA from start

In migration case, the target would have @fit-dirty set thanks to
pc_dimm_memory_plug () -> regenerate_fit() and any DMA in flight
would be restarted.
That's a little bit inefficient as source_fit exactly matches
target_fit and DMA could continue, but it would save us from
having fit specific migration state to transfer and maintain.

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong]

On 10/11/2016 08:32 PM, Igor Mammedov wrote:

>>
>> Of course, @fit and @current_number should be persistent during live migration.
> you can drop RCU and @current_number,
> and @fit could be exactly recreated on target side from
>  -device nvdim ...
> set of options, which must include all currently present
> (including hotplugged) devices from source side.
>
> It's sufficient to invalidate and restart DMA transfer in flight.
>
> i.e.
> pc_dimm_memory_plug ()
>   -> regenerate_fit()
>       -> set local flag @fit-dirty
>
>   -> start QEMU.fit_read()
>       -> if offset == 0 ? clear @fit-dirty
>       make sure fit_read() won't conflict with regenerate_fit()
>         either plain mutex or RCU would do the job
>
>   ...
>   OSPM.rfit()
>      ...
>      if (@fit-dirty)
>          abort/restart DMA from start
>
> In migration case, the target would have @fit-dirty set thanks to
> pc_dimm_memory_plug () -> regenerate_fit() and any DMA in flight
> would be restarted.
> That's a little bit inefficient as source_fit exactly matches
> target_fit and DMA could continue, but it would save us from
> having fit specific migration state to transfer and maintain.
>

Nice, that works. :-)

Thanks!