[Qemu-devel] [PATCH v4 0/3] nvdimm: hotplug support

[Qemu-devel] [PATCH v4 0/3] nvdimm: hotplug support

[Xiao Guangrong]

Resend these 3 patches to catch up release window...

Igor,

this is a open that i did not pass a buffer as parameter to RFIT as
tried the way you suggested, but failed. May be i am not very good at
ASL, i need more time to try. So let's keep the way as it is, i will
improve it later.

Thanks!

Changelog in v4:
   1) drop fit lock and post_hotplug_cb
   2) move nvdimm hotplug code to hw/acpi/nvdimm.c
   3) introduce length field to indicate the fit size
   4) nvdimm acpi cleanup
   5) doc typo fixes

Changelog in v3:
   1) use a dedicated interrupt for nvdimm device hotplug
   2) stop nvdimm device hot unplug
   3) reserve UUID and handle for QEMU internally used QEMU
   5) redesign fit buffer to avoid OSPM reading incomplete fit info
   6) bug fixes and cleanups

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

Xiao Guangrong (3):
  nvdimm acpi: introduce fit buffer
  nvdimm acpi: introduce _FIT
  pc: memhp: enable nvdimm device hotplug

 default-configs/mips-softmmu-common.mak |   1 +
 docs/specs/acpi_nvdimm.txt              |  68 +++++++-
 hw/acpi/ich9.c                          |   8 +-
 hw/acpi/nvdimm.c                        | 289 ++++++++++++++++++++++++++++----
 hw/acpi/piix4.c                         |   7 +-
 hw/i386/acpi-build.c                    |   9 +-
 hw/i386/pc.c                            |  16 ++
 hw/mem/nvdimm.c                         |   4 -
 include/hw/acpi/acpi_dev_interface.h    |   1 +
 include/hw/mem/nvdimm.h                 |  22 ++-
 10 files changed, 377 insertions(+), 48 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 1/3] nvdimm acpi: introduce fit buffer

[Xiao Guangrong]

The buffer is used to save the FIT info for all the presented nvdimm
devices which is updated after the nvdimm device is plugged or
unplugged. In the later patch, it will be used to construct NVDIMM
ACPI _FIT method which reflects the presented nvdimm devices after
nvdimm hotplug

As FIT buffer can not completely mapped into guest address space,
OSPM will exit to QEMU multiple times, however, there is the race
condition - FIT may be changed during these multiple exits, so that
we mark @dirty whenever the buffer is updated.

@dirty is cleared for the first time OSPM gets fit buffer, if
dirty is detected in the later access, OSPM will restart the
access

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c        | 57 ++++++++++++++++++++++++++++++-------------------
 hw/i386/acpi-build.c    |  2 +-
 hw/i386/pc.c            |  4 ++++
 include/hw/mem/nvdimm.h | 21 +++++++++++++++++-
 4 files changed, 60 insertions(+), 24 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index b8a2e62..9fee077 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -38,11 +38,7 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque)
     GSList **list = opaque;
 
     if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
-        DeviceState *dev = DEVICE(obj);
-
-        if (dev->realized) { /* only realized NVDIMMs matter */
-            *list = g_slist_append(*list, DEVICE(obj));
-        }
+        *list = g_slist_append(*list, DEVICE(obj));
     }
 
     object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
@@ -348,8 +344,9 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
                                          (DSM) in DSM Spec Rev1.*/);
 }
 
-static GArray *nvdimm_build_device_structure(GSList *device_list)
+static GArray *nvdimm_build_device_structure(void)
 {
+    GSList *device_list = nvdimm_get_plugged_device_list();
     GArray *structures = g_array_new(false, true /* clear */, 1);
 
     for (; device_list; device_list = device_list->next) {
@@ -367,28 +364,50 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
         /* build NVDIMM Control Region Structure. */
         nvdimm_build_structure_dcr(structures, dev);
     }
+    g_slist_free(device_list);
 
     return structures;
 }
 
-static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
+static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
+{
+    fit_buf->fit = g_array_new(false, true /* clear */, 1);
+}
+
+static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
+{
+    g_array_free(fit_buf->fit, true);
+    fit_buf->fit = nvdimm_build_device_structure();
+    fit_buf->dirty = true;
+}
+
+void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
+{
+    nvdimm_build_fit_buffer(&state->fit_buf);
+}
+
+static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
                               GArray *table_data, BIOSLinker *linker)
 {
-    GArray *structures = nvdimm_build_device_structure(device_list);
+    NvdimmFitBuffer *fit_buf = &state->fit_buf;
     unsigned int header;
 
+    /* NVDIMM device is not plugged? */
+    if (!fit_buf->fit->len) {
+        return;
+    }
+
     acpi_add_table(table_offsets, table_data);
 
     /* NFIT header. */
     header = table_data->len;
     acpi_data_push(table_data, sizeof(NvdimmNfitHeader));
     /* NVDIMM device structures. */
-    g_array_append_vals(table_data, structures->data, structures->len);
+    g_array_append_vals(table_data, fit_buf->fit->data, fit_buf->fit->len);
 
     build_header(linker, table_data,
                  (void *)(table_data->data + header), "NFIT",
-                 sizeof(NvdimmNfitHeader) + structures->len, 1, NULL, NULL);
-    g_array_free(structures, true);
+                 sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
 }
 
 struct NvdimmDsmIn {
@@ -771,6 +790,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
     acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
     fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
                     state->dsm_mem->len);
+
+    nvdimm_init_fit_buffer(&state->fit_buf);
 }
 
 #define NVDIMM_COMMON_DSM       "NCAL"
@@ -1045,25 +1066,17 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
 }
 
 void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
-                       BIOSLinker *linker, GArray *dsm_dma_arrea,
+                       BIOSLinker *linker, AcpiNVDIMMState *state,
                        uint32_t ram_slots)
 {
-    GSList *device_list;
-
-    device_list = nvdimm_get_plugged_device_list();
-
-    /* NVDIMM device is plugged. */
-    if (device_list) {
-        nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
-        g_slist_free(device_list);
-    }
+    nvdimm_build_nfit(state, table_offsets, table_data, linker);
 
     /*
      * NVDIMM device is allowed to be plugged only if there is available
      * slot.
      */
     if (ram_slots) {
-        nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
+        nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
                           ram_slots);
     }
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6ae4769..bc49958 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     }
     if (pcms->acpi_nvdimm_state.is_enabled) {
         nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
-                          pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots);
+                          &pcms->acpi_nvdimm_state, machine->ram_slots);
     }
 
     /* Add tables supplied by user (if any) */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 93ff49c..77ca7f4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1700,6 +1700,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
+    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
 out:
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 63a2b20..232437c 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -98,12 +98,30 @@ typedef struct NVDIMMClass NVDIMMClass;
 #define NVDIMM_ACPI_IO_BASE     0x0a18
 #define NVDIMM_ACPI_IO_LEN      4
 
+/*
+ * The buffer, @fit, saves the FIT info for all the presented NVDIMM
+ * devices which is updated after the NVDIMM device is plugged or
+ * unplugged.
+ *
+ * Mark @dirty whenever the buffer is updated so that it preserves NVDIMM
+ * ACPI _FIT method to read incomplete or obsolete fit info if fit update
+ * happens during multiple RFIT calls.
+ */
+struct NvdimmFitBuffer {
+    GArray *fit;
+    bool dirty;
+};
+typedef struct NvdimmFitBuffer NvdimmFitBuffer;
+
 struct AcpiNVDIMMState {
     /* detect if NVDIMM support is enabled. */
     bool is_enabled;
 
     /* the data of the fw_cfg file NVDIMM_DSM_MEM_FILE. */
     GArray *dsm_mem;
+
+    NvdimmFitBuffer fit_buf;
+
     /* the IO region used by OSPM to transfer control to QEMU. */
     MemoryRegion io_mr;
 };
@@ -112,6 +130,7 @@ 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, AcpiNVDIMMState *state,
                        uint32_t ram_slots);
+void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 1/3] nvdimm acpi: introduce fit buffer

[Stefan Hajnoczi]

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

On Thu, Nov 03, 2016 at 11:51:28AM +0800, Xiao Guangrong wrote:
> +static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
> +{
> +    fit_buf->fit = g_array_new(false, true /* clear */, 1);
> +}
> +
> +static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
> +{
> +    g_array_free(fit_buf->fit, true);
> +    fit_buf->fit = nvdimm_build_device_structure();

In the previous revision I pointed out that it's messy to inline
g_array_new(false, true /* clear */, 1) in nvdimm_init_fit_buffer() when
the data structure is normally created by
nvdimm_build_device_structure().  You didn't respond.

Is it possible to call nvdimm_build_device_structure() in
nvdimm_init_fit_buffer() so we don't need to duplicate the details of
how the GArray is created?

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

Re: [Qemu-devel] [PATCH v4 1/3] nvdimm acpi: introduce fit buffer

[Xiao Guangrong]

On 11/03/2016 06:00 PM, Stefan Hajnoczi wrote:
> On Thu, Nov 03, 2016 at 11:51:28AM +0800, Xiao Guangrong wrote:
>> +static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
>> +{
>> +    fit_buf->fit = g_array_new(false, true /* clear */, 1);
>> +}
>> +
>> +static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
>> +{
>> +    g_array_free(fit_buf->fit, true);
>> +    fit_buf->fit = nvdimm_build_device_structure();
>
> In the previous revision I pointed out that it's messy to inline
> g_array_new(false, true /* clear */, 1) in nvdimm_init_fit_buffer() when
> the data structure is normally created by
> nvdimm_build_device_structure().  You didn't respond.
>

Oh, sorry for that.

> Is it possible to call nvdimm_build_device_structure() in
> nvdimm_init_fit_buffer() so we don't need to duplicate the details of
> how the GArray is created?
>

Actually, i tried your suggestion however i noticed it makes the code
little confuse, as we construct the fit by calling
nvdimm_build_device_structure() whose name shows the fit will be
rebuild but do not set .dirty in the init path.

g_array_new(false, true /* clear */, 1) is a more clear way to
show this is just a empty array.

Re: [Qemu-devel] [PATCH v4 1/3] nvdimm acpi: introduce fit buffer

[Igor Mammedov]

On Thu,  3 Nov 2016 11:51:28 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> The buffer is used to save the FIT info for all the presented nvdimm
> devices which is updated after the nvdimm device is plugged or
> unplugged. In the later patch, it will be used to construct NVDIMM
> ACPI _FIT method which reflects the presented nvdimm devices after
> nvdimm hotplug
> 
> As FIT buffer can not completely mapped into guest address space,
> OSPM will exit to QEMU multiple times, however, there is the race
> condition - FIT may be changed during these multiple exits, so that
> we mark @dirty whenever the buffer is updated.
> 
> @dirty is cleared for the first time OSPM gets fit buffer, if
> dirty is detected in the later access, OSPM will restart the
> access
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c        | 57 ++++++++++++++++++++++++++++++-------------------
>  hw/i386/acpi-build.c    |  2 +-
>  hw/i386/pc.c            |  4 ++++
>  include/hw/mem/nvdimm.h | 21 +++++++++++++++++-
>  4 files changed, 60 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index b8a2e62..9fee077 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -38,11 +38,7 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque)
s/nvdimm_plugged_device_list/nvdimm_device_list/

>      GSList **list = opaque;
>  
>      if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
> -        DeviceState *dev = DEVICE(obj);
> -
> -        if (dev->realized) { /* only realized NVDIMMs matter */
> -            *list = g_slist_append(*list, DEVICE(obj));
> -        }
> +        *list = g_slist_append(*list, DEVICE(obj));
>      }
>  
>      object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
> @@ -348,8 +344,9 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
>                                           (DSM) in DSM Spec Rev1.*/);
>  }
>  
> -static GArray *nvdimm_build_device_structure(GSList *device_list)
> +static GArray *nvdimm_build_device_structure(void)
>  {
> +    GSList *device_list = nvdimm_get_plugged_device_list();
>      GArray *structures = g_array_new(false, true /* clear */, 1);
>  
>      for (; device_list; device_list = device_list->next) {
> @@ -367,28 +364,50 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
>          /* build NVDIMM Control Region Structure. */
>          nvdimm_build_structure_dcr(structures, dev);
>      }
> +    g_slist_free(device_list);
>  
>      return structures;
>  }
>  
> -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
> +static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
> +{
> +    fit_buf->fit = g_array_new(false, true /* clear */, 1);
> +}
> +
> +static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
> +{
> +    g_array_free(fit_buf->fit, true);
> +    fit_buf->fit = nvdimm_build_device_structure();
> +    fit_buf->dirty = true;
> +}
> +
> +void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
> +{
> +    nvdimm_build_fit_buffer(&state->fit_buf);
> +}
> +
> +static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
>                                GArray *table_data, BIOSLinker *linker)
>  {
> -    GArray *structures = nvdimm_build_device_structure(device_list);
> +    NvdimmFitBuffer *fit_buf = &state->fit_buf;
>      unsigned int header;
>  
> +    /* NVDIMM device is not plugged? */
> +    if (!fit_buf->fit->len) {
it's not really obvious way to check for present nvdimms,
maybe dropping this hunk and keeping device_list check at call site would be clearer.

> +        return;
> +    }
> +
>      acpi_add_table(table_offsets, table_data);
>  
>      /* NFIT header. */
>      header = table_data->len;
>      acpi_data_push(table_data, sizeof(NvdimmNfitHeader));
>      /* NVDIMM device structures. */
> -    g_array_append_vals(table_data, structures->data, structures->len);
> +    g_array_append_vals(table_data, fit_buf->fit->data, fit_buf->fit->len);
>  
>      build_header(linker, table_data,
>                   (void *)(table_data->data + header), "NFIT",
> -                 sizeof(NvdimmNfitHeader) + structures->len, 1, NULL, NULL);
> -    g_array_free(structures, true);
> +                 sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
>  }
>  
>  struct NvdimmDsmIn {
> @@ -771,6 +790,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>      acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
>      fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
>                      state->dsm_mem->len);
> +
> +    nvdimm_init_fit_buffer(&state->fit_buf);
>  }
>  
>  #define NVDIMM_COMMON_DSM       "NCAL"
> @@ -1045,25 +1066,17 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>  }
>  
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> -                       BIOSLinker *linker, GArray *dsm_dma_arrea,
> +                       BIOSLinker *linker, AcpiNVDIMMState *state,
>                         uint32_t ram_slots)
>  {
> -    GSList *device_list;
> -
> -    device_list = nvdimm_get_plugged_device_list();
> -
> -    /* NVDIMM device is plugged. */
> -    if (device_list) {
> -        nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> -        g_slist_free(device_list);
> -    }
> +    nvdimm_build_nfit(state, table_offsets, table_data, linker);
>  
>      /*
>       * NVDIMM device is allowed to be plugged only if there is available
>       * slot.
>       */
>      if (ram_slots) {
> -        nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
> +        nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
>                            ram_slots);
you've ignored comments on v3 1/4,
fix it up.

>      }
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6ae4769..bc49958 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>      }
>      if (pcms->acpi_nvdimm_state.is_enabled) {
>          nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> -                          pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots);
> +                          &pcms->acpi_nvdimm_state, machine->ram_slots);
>      }
>  
>      /* Add tables supplied by user (if any) */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 93ff49c..77ca7f4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1700,6 +1700,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
s/nvdimm_acpi_hotplug/nvdimm_plug/

> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
>  out:
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 63a2b20..232437c 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -98,12 +98,30 @@ typedef struct NVDIMMClass NVDIMMClass;
>  #define NVDIMM_ACPI_IO_BASE     0x0a18
>  #define NVDIMM_ACPI_IO_LEN      4
>  
> +/*
> + * The buffer, @fit, saves the FIT info for all the presented NVDIMM
> + * devices
FIT structures for present NVDIMMs

> + which is updated after the NVDIMM device is plugged or
> + * unplugged.
s/which is/. It is/
 
> + *
> + * Mark @dirty whenever the buffer is updated so that it preserves NVDIMM
> + * ACPI _FIT method to read incomplete or obsolete fit info if fit update
> + * happens during multiple RFIT calls.
> + */
not valid doc comment, see include/qom/object.h for example

> +struct NvdimmFitBuffer {
> +    GArray *fit;
> +    bool dirty;
> +};
> +typedef struct NvdimmFitBuffer NvdimmFitBuffer;
> +
>  struct AcpiNVDIMMState {
>      /* detect if NVDIMM support is enabled. */
>      bool is_enabled;
>  
>      /* the data of the fw_cfg file NVDIMM_DSM_MEM_FILE. */
>      GArray *dsm_mem;
> +
> +    NvdimmFitBuffer fit_buf;
> +
>      /* the IO region used by OSPM to transfer control to QEMU. */
>      MemoryRegion io_mr;
>  };
> @@ -112,6 +130,7 @@ 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, AcpiNVDIMMState *state,
>                         uint32_t ram_slots);
> +void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
>  #endif

Re: [Qemu-devel] [PATCH v4 1/3] nvdimm acpi: introduce fit buffer

[Xiao Guangrong]

On 11/03/2016 07:02 PM, Igor Mammedov wrote:
> On Thu,  3 Nov 2016 11:51:28 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> The buffer is used to save the FIT info for all the presented nvdimm
>> devices which is updated after the nvdimm device is plugged or
>> unplugged. In the later patch, it will be used to construct NVDIMM
>> ACPI _FIT method which reflects the presented nvdimm devices after
>> nvdimm hotplug
>>
>> As FIT buffer can not completely mapped into guest address space,
>> OSPM will exit to QEMU multiple times, however, there is the race
>> condition - FIT may be changed during these multiple exits, so that
>> we mark @dirty whenever the buffer is updated.
>>
>> @dirty is cleared for the first time OSPM gets fit buffer, if
>> dirty is detected in the later access, OSPM will restart the
>> access
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>  hw/acpi/nvdimm.c        | 57 ++++++++++++++++++++++++++++++-------------------
>>  hw/i386/acpi-build.c    |  2 +-
>>  hw/i386/pc.c            |  4 ++++
>>  include/hw/mem/nvdimm.h | 21 +++++++++++++++++-
>>  4 files changed, 60 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index b8a2e62..9fee077 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -38,11 +38,7 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque)
> s/nvdimm_plugged_device_list/nvdimm_device_list/

Okay.

>
>>      GSList **list = opaque;
>>
>>      if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
>> -        DeviceState *dev = DEVICE(obj);
>> -
>> -        if (dev->realized) { /* only realized NVDIMMs matter */
>> -            *list = g_slist_append(*list, DEVICE(obj));
>> -        }
>> +        *list = g_slist_append(*list, DEVICE(obj));
>>      }
>>
>>      object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
>> @@ -348,8 +344,9 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
>>                                           (DSM) in DSM Spec Rev1.*/);
>>  }
>>
>> -static GArray *nvdimm_build_device_structure(GSList *device_list)
>> +static GArray *nvdimm_build_device_structure(void)
>>  {
>> +    GSList *device_list = nvdimm_get_plugged_device_list();
>>      GArray *structures = g_array_new(false, true /* clear */, 1);
>>
>>      for (; device_list; device_list = device_list->next) {
>> @@ -367,28 +364,50 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
>>          /* build NVDIMM Control Region Structure. */
>>          nvdimm_build_structure_dcr(structures, dev);
>>      }
>> +    g_slist_free(device_list);
>>
>>      return structures;
>>  }
>>
>> -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
>> +static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
>> +{
>> +    fit_buf->fit = g_array_new(false, true /* clear */, 1);
>> +}
>> +
>> +static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
>> +{
>> +    g_array_free(fit_buf->fit, true);
>> +    fit_buf->fit = nvdimm_build_device_structure();
>> +    fit_buf->dirty = true;
>> +}
>> +
>> +void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
>> +{
>> +    nvdimm_build_fit_buffer(&state->fit_buf);
>> +}
>> +
>> +static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
>>                                GArray *table_data, BIOSLinker *linker)
>>  {
>> -    GArray *structures = nvdimm_build_device_structure(device_list);
>> +    NvdimmFitBuffer *fit_buf = &state->fit_buf;
>>      unsigned int header;
>>
>> +    /* NVDIMM device is not plugged? */
>> +    if (!fit_buf->fit->len) {
> it's not really obvious way to check for present nvdimms,
> maybe dropping this hunk and keeping device_list check at call site would be clearer.

Hmm... okay.

>
>> +        return;
>> +    }
>> +
>>      acpi_add_table(table_offsets, table_data);
>>
>>      /* NFIT header. */
>>      header = table_data->len;
>>      acpi_data_push(table_data, sizeof(NvdimmNfitHeader));
>>      /* NVDIMM device structures. */
>> -    g_array_append_vals(table_data, structures->data, structures->len);
>> +    g_array_append_vals(table_data, fit_buf->fit->data, fit_buf->fit->len);
>>
>>      build_header(linker, table_data,
>>                   (void *)(table_data->data + header), "NFIT",
>> -                 sizeof(NvdimmNfitHeader) + structures->len, 1, NULL, NULL);
>> -    g_array_free(structures, true);
>> +                 sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
>>  }
>>
>>  struct NvdimmDsmIn {
>> @@ -771,6 +790,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>>      acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
>>      fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
>>                      state->dsm_mem->len);
>> +
>> +    nvdimm_init_fit_buffer(&state->fit_buf);
>>  }
>>
>>  #define NVDIMM_COMMON_DSM       "NCAL"
>> @@ -1045,25 +1066,17 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>>  }
>>
>>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>> -                       BIOSLinker *linker, GArray *dsm_dma_arrea,
>> +                       BIOSLinker *linker, AcpiNVDIMMState *state,
>>                         uint32_t ram_slots)
>>  {
>> -    GSList *device_list;
>> -
>> -    device_list = nvdimm_get_plugged_device_list();
>> -
>> -    /* NVDIMM device is plugged. */
>> -    if (device_list) {
>> -        nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
>> -        g_slist_free(device_list);
>> -    }
>> +    nvdimm_build_nfit(state, table_offsets, table_data, linker);
>>
>>      /*
>>       * NVDIMM device is allowed to be plugged only if there is available
>>       * slot.
>>       */
>>      if (ram_slots) {
>> -        nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
>> +        nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
>>                            ram_slots);
> you've ignored comments on v3 1/4,
> fix it up.

As you said "I'd prefer it to make a clean revert for patches 2-4/4  first",
i thought the first patch is okay.

But it does not matter, i will quickly post a new version after you
review all the patches.

>
>>      }
>>  }
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 6ae4769..bc49958 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>>      }
>>      if (pcms->acpi_nvdimm_state.is_enabled) {
>>          nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
>> -                          pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots);
>> +                          &pcms->acpi_nvdimm_state, machine->ram_slots);
>>      }
>>
>>      /* Add tables supplied by user (if any) */
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 93ff49c..77ca7f4 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1700,6 +1700,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>>          goto out;
>>      }
>>
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>> +        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
> s/nvdimm_acpi_hotplug/nvdimm_plug/

Okay.

>
>> +    }
>> +
>>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
>>  out:
>> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
>> index 63a2b20..232437c 100644
>> --- a/include/hw/mem/nvdimm.h
>> +++ b/include/hw/mem/nvdimm.h
>> @@ -98,12 +98,30 @@ typedef struct NVDIMMClass NVDIMMClass;
>>  #define NVDIMM_ACPI_IO_BASE     0x0a18
>>  #define NVDIMM_ACPI_IO_LEN      4
>>
>> +/*
>> + * The buffer, @fit, saves the FIT info for all the presented NVDIMM
>> + * devices
> FIT structures for present NVDIMMs

Okay.

>
>> + which is updated after the NVDIMM device is plugged or
>> + * unplugged.
> s/which is/. It is/

Okay.

>
>> + *
>> + * Mark @dirty whenever the buffer is updated so that it preserves NVDIMM
>> + * ACPI _FIT method to read incomplete or obsolete fit info if fit update
>> + * happens during multiple RFIT calls.
>> + */
> not valid doc comment, see include/qom/object.h for example
>

Okay, will change it to:

NvdimmFitBuffer:
@fit: FIT structures for present NVDIMMs. It is updated after the NVDIMM device
       is plugged or unplugged.
@dirty: It is set whenever the buffer is updated so that it preserves NVDIMM
         ACPI _FIT method to read incomplete or obsolete fit info if fit update
         happens during multiple RFIT calls

Re: [Qemu-devel] [PATCH v4 1/3] nvdimm acpi: introduce fit buffer

[Igor Mammedov]

On Thu, 3 Nov 2016 19:09:35 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 11/03/2016 07:02 PM, Igor Mammedov wrote:
> > On Thu,  3 Nov 2016 11:51:28 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> The buffer is used to save the FIT info for all the presented nvdimm
> >> devices which is updated after the nvdimm device is plugged or
> >> unplugged. In the later patch, it will be used to construct NVDIMM
> >> ACPI _FIT method which reflects the presented nvdimm devices after
> >> nvdimm hotplug
> >>
> >> As FIT buffer can not completely mapped into guest address space,
> >> OSPM will exit to QEMU multiple times, however, there is the race
> >> condition - FIT may be changed during these multiple exits, so that
> >> we mark @dirty whenever the buffer is updated.
> >>
> >> @dirty is cleared for the first time OSPM gets fit buffer, if
> >> dirty is detected in the later access, OSPM will restart the
> >> access
> >>
> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> ---
> >>  hw/acpi/nvdimm.c        | 57 ++++++++++++++++++++++++++++++-------------------
> >>  hw/i386/acpi-build.c    |  2 +-
> >>  hw/i386/pc.c            |  4 ++++
> >>  include/hw/mem/nvdimm.h | 21 +++++++++++++++++-
> >>  4 files changed, 60 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> >> index b8a2e62..9fee077 100644
> >> --- a/hw/acpi/nvdimm.c
> >> +++ b/hw/acpi/nvdimm.c
> >> @@ -38,11 +38,7 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque)  
> > s/nvdimm_plugged_device_list/nvdimm_device_list/  
> 
> Okay.
> 
> >  
> >>      GSList **list = opaque;
> >>
> >>      if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
> >> -        DeviceState *dev = DEVICE(obj);
> >> -
> >> -        if (dev->realized) { /* only realized NVDIMMs matter */
> >> -            *list = g_slist_append(*list, DEVICE(obj));
> >> -        }
> >> +        *list = g_slist_append(*list, DEVICE(obj));
> >>      }
> >>
> >>      object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
> >> @@ -348,8 +344,9 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
> >>                                           (DSM) in DSM Spec Rev1.*/);
> >>  }
> >>
> >> -static GArray *nvdimm_build_device_structure(GSList *device_list)
> >> +static GArray *nvdimm_build_device_structure(void)
> >>  {
> >> +    GSList *device_list = nvdimm_get_plugged_device_list();
> >>      GArray *structures = g_array_new(false, true /* clear */, 1);
> >>
> >>      for (; device_list; device_list = device_list->next) {
> >> @@ -367,28 +364,50 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
> >>          /* build NVDIMM Control Region Structure. */
> >>          nvdimm_build_structure_dcr(structures, dev);
> >>      }
> >> +    g_slist_free(device_list);
> >>
> >>      return structures;
> >>  }
> >>
> >> -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
> >> +static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
> >> +{
> >> +    fit_buf->fit = g_array_new(false, true /* clear */, 1);
> >> +}
> >> +
> >> +static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
> >> +{
> >> +    g_array_free(fit_buf->fit, true);
> >> +    fit_buf->fit = nvdimm_build_device_structure();
> >> +    fit_buf->dirty = true;
> >> +}
> >> +
> >> +void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
> >> +{
> >> +    nvdimm_build_fit_buffer(&state->fit_buf);
> >> +}
> >> +
> >> +static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
> >>                                GArray *table_data, BIOSLinker *linker)
> >>  {
> >> -    GArray *structures = nvdimm_build_device_structure(device_list);
> >> +    NvdimmFitBuffer *fit_buf = &state->fit_buf;
> >>      unsigned int header;
> >>
> >> +    /* NVDIMM device is not plugged? */
> >> +    if (!fit_buf->fit->len) {  
> > it's not really obvious way to check for present nvdimms,
> > maybe dropping this hunk and keeping device_list check at call site would be clearer.  
> 
> Hmm... okay.
> 
> >  
> >> +        return;
> >> +    }
> >> +
> >>      acpi_add_table(table_offsets, table_data);
> >>
> >>      /* NFIT header. */
> >>      header = table_data->len;
> >>      acpi_data_push(table_data, sizeof(NvdimmNfitHeader));
> >>      /* NVDIMM device structures. */
> >> -    g_array_append_vals(table_data, structures->data, structures->len);
> >> +    g_array_append_vals(table_data, fit_buf->fit->data, fit_buf->fit->len);
> >>
> >>      build_header(linker, table_data,
> >>                   (void *)(table_data->data + header), "NFIT",
> >> -                 sizeof(NvdimmNfitHeader) + structures->len, 1, NULL, NULL);
> >> -    g_array_free(structures, true);
> >> +                 sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
> >>  }
> >>
> >>  struct NvdimmDsmIn {
> >> @@ -771,6 +790,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
> >>      acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
> >>      fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
> >>                      state->dsm_mem->len);
> >> +
> >> +    nvdimm_init_fit_buffer(&state->fit_buf);
> >>  }
> >>
> >>  #define NVDIMM_COMMON_DSM       "NCAL"
> >> @@ -1045,25 +1066,17 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> >>  }
> >>
> >>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> >> -                       BIOSLinker *linker, GArray *dsm_dma_arrea,
> >> +                       BIOSLinker *linker, AcpiNVDIMMState *state,
> >>                         uint32_t ram_slots)
> >>  {
> >> -    GSList *device_list;
> >> -
> >> -    device_list = nvdimm_get_plugged_device_list();
> >> -
> >> -    /* NVDIMM device is plugged. */
> >> -    if (device_list) {
> >> -        nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> >> -        g_slist_free(device_list);
> >> -    }
> >> +    nvdimm_build_nfit(state, table_offsets, table_data, linker);
> >>
> >>      /*
> >>       * NVDIMM device is allowed to be plugged only if there is available
> >>       * slot.
> >>       */
> >>      if (ram_slots) {
> >> -        nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
> >> +        nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
> >>                            ram_slots);  
> > you've ignored comments on v3 1/4,
> > fix it up.  
> 
> As you said "I'd prefer it to make a clean revert for patches 2-4/4  first",
> i thought the first patch is okay.
> 
> But it does not matter, i will quickly post a new version after you
> review all the patches.
and include in that fixed up v3 1/4

> 
> >  
> >>      }
> >>  }
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 6ae4769..bc49958 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> >>      }
> >>      if (pcms->acpi_nvdimm_state.is_enabled) {
> >>          nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> >> -                          pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots);
> >> +                          &pcms->acpi_nvdimm_state, machine->ram_slots);
> >>      }
> >>
> >>      /* Add tables supplied by user (if any) */
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 93ff49c..77ca7f4 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1700,6 +1700,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >>          goto out;
> >>      }
> >>
> >> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> >> +        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);  
> > s/nvdimm_acpi_hotplug/nvdimm_plug/  
> 
> Okay.
> 
> >  
> >> +    }
> >> +
> >>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> >>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
> >>  out:
> >> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> >> index 63a2b20..232437c 100644
> >> --- a/include/hw/mem/nvdimm.h
> >> +++ b/include/hw/mem/nvdimm.h
> >> @@ -98,12 +98,30 @@ typedef struct NVDIMMClass NVDIMMClass;
> >>  #define NVDIMM_ACPI_IO_BASE     0x0a18
> >>  #define NVDIMM_ACPI_IO_LEN      4
> >>
> >> +/*
> >> + * The buffer, @fit, saves the FIT info for all the presented NVDIMM
> >> + * devices  
> > FIT structures for present NVDIMMs  
> 
> Okay.
> 
> >  
> >> + which is updated after the NVDIMM device is plugged or
> >> + * unplugged.  
> > s/which is/. It is/  
> 
> Okay.
> 
> >  
> >> + *
> >> + * Mark @dirty whenever the buffer is updated so that it preserves NVDIMM
> >> + * ACPI _FIT method to read incomplete or obsolete fit info if fit update
> >> + * happens during multiple RFIT calls.
> >> + */  
> > not valid doc comment, see include/qom/object.h for example
> >  
> 
> Okay, will change it to:
> 
> NvdimmFitBuffer:
> @fit: FIT structures for present NVDIMMs. It is updated after the NVDIMM device
>        is plugged or unplugged.
> @dirty: It is set whenever the buffer is updated so that it preserves NVDIMM
>          ACPI _FIT method to read incomplete or obsolete fit info if fit update
>          happens during multiple RFIT calls
> 

[Qemu-devel] [PATCH v4 2/3] 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 UUID is UUID
648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
is 0x1, 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>
---
 docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
 hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 271 insertions(+), 17 deletions(-)

diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 0fdd251..364e832 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
    The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
    NVDIMM Firmware Interface Table (NFIT).
 
-QEMU NVDIMM Implemention
-========================
+QEMU NVDIMM Implementation
+==========================
 QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
 for NVDIMM ACPI.
 
@@ -82,6 +82,16 @@ Memory:
    ACPI writes _DSM Input Data (based on the offset in the page):
    [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
                 Root device.
+
+                The handle is completely QEMU internal thing, the values in
+                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
+                root device named NVDR), other values are reserved by other
+                purpose.
+
+                Current reserved handle:
+                0x10000 is reserved for QEMU internal DSM function called on
+                the root device.
+
    [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
    [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
    [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
@@ -127,6 +137,49 @@ _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
+   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
+   function (private QEMU function)
+
+   _FIT method uses Read_FIT function to fetch NFIT structures blob from
+   QEMU in 1 page sized increments which are then concatenated and returned
+   as _FIT method result.
+
+   Input parameters:
+   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
+   Arg1 – Revision ID (set to 1)
+   Arg2 - Function Index, 0x1
+   Arg3 - A package containing a buffer whose layout is as follows:
+
+   +----------+--------+--------+-------------------------------------------+
+   |  Field   | Length | Offset |                 Description               |
+   +----------+--------+--------+-------------------------------------------+
+   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
+   |          |        |        | read from                                 |
+   +----------+--------+--------+-------------------------------------------+
+
+   Output:
+   +----------+--------+--------+-------------------------------------------+
+   |  Field   | Length | Offset |                 Description               |
+   +----------+--------+--------+-------------------------------------------+
+   |          |        |        | return status codes                       |
+   |          |        |        | 0x100 - error caused by NFIT update while |
+   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
+   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
+   +----------+--------+--------+-------------------------------------------+
+   | length   |   4    |   4    | The fit size                              |           
+   +----------+-----------------+-------------------------------------------+
+   | fit data | Varies |   8    | FIT data, its size is indicated by length |
+   |          |        |        | filed above                               |
+   +----------+--------+--------+-------------------------------------------+
+
+   The FIT offset is maintained by the OSPM itself, current offset plus
+   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.
+
+   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
+   again).
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 9fee077..593ac0d 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -484,6 +484,23 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
                   offsetof(NvdimmDsmIn, arg3) > 4096);
 
+struct NvdimmFuncReadFITIn {
+    uint32_t offset; /* the offset into 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. */
+    uint32_t length; /* the length of fit. */
+    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)
 {
@@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
     cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
 }
 
+#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
+#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
+#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */
+#define NVDIMM_DSM_RET_STATUS_FIT_CHANGED    0x100 /* FIT Changed */
+
+#define NVDIMM_QEMU_RSVD_HANDLE_ROOT         0x10000
+
+/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
+static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+                                     hwaddr dsm_mem_addr)
+{
+    NvdimmFitBuffer *fit_buf = &state->fit_buf;
+    NvdimmFuncReadFITIn *read_fit;
+    NvdimmFuncReadFITOut *read_fit_out;
+    GArray *fit;
+    uint32_t read_len = 0, func_ret_status, offset;
+    int size;
+
+    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
+    offset = le32_to_cpu(read_fit->offset);
+
+    fit = fit_buf->fit;
+
+    nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
+                 offset, fit->len, fit_buf->dirty ? "Yes" : "No");
+
+    /* It is the first time to read FIT. */
+    if (!offset) {
+        fit_buf->dirty = false;
+    } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */
+        func_ret_status = NVDIMM_DSM_RET_STATUS_FIT_CHANGED;
+        goto exit;
+    }
+
+    if (offset > fit->len) {
+        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
+        goto exit;
+    }
+
+    func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
+    read_len = MIN(fit->len - offset, 4096 - sizeof(NvdimmFuncReadFITOut));
+
+exit:
+    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);
+    read_fit_out->length = cpu_to_le32(read_len);
+    memcpy(read_fit_out->fit, fit->data + offset, read_len);
+
+    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
+
+    g_free(read_fit_out);
+}
+
+static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+                                     hwaddr dsm_mem_addr)
+{
+    switch (in->function) {
+    case 0x0:
+        nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
+        return;
+    case 0x1 /* Read FIT */:
+        nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
+        return;
+    }
+
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
+}
+
 static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
 {
     /*
@@ -563,7 +651,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
 
     nvdimm_debug("label_size %#x, max_xfer %#x.\n", label_size, mxfer);
 
-    label_size_out.func_ret_status = cpu_to_le32(0 /* Success */);
+    label_size_out.func_ret_status = cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
     label_size_out.label_size = cpu_to_le32(label_size);
     label_size_out.max_xfer = cpu_to_le32(mxfer);
 
@@ -574,7 +662,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
 static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
                                            uint32_t offset, uint32_t length)
 {
-    uint32_t ret = 3 /* Invalid Input Parameters */;
+    uint32_t ret = NVDIMM_DSM_RET_STATUS_INVALID;
 
     if (offset + length < offset) {
         nvdimm_debug("offset %#x + length %#x is overflow.\n", offset,
@@ -594,7 +682,7 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
         return ret;
     }
 
-    return 0 /* Success */;
+    return NVDIMM_DSM_RET_STATUS_SUCCESS;
 }
 
 /*
@@ -618,7 +706,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
 
     status = nvdimm_rw_label_data_check(nvdimm, get_label_data->offset,
                                         get_label_data->length);
-    if (status != 0 /* Success */) {
+    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
         nvdimm_dsm_no_payload(status, dsm_mem_addr);
         return;
     }
@@ -628,7 +716,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
     get_label_data_out = g_malloc(size);
 
     get_label_data_out->len = cpu_to_le32(size);
-    get_label_data_out->func_ret_status = cpu_to_le32(0 /* Success */);
+    get_label_data_out->func_ret_status =
+                                cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
     nvc->read_label_data(nvdimm, get_label_data_out->out_buf,
                          get_label_data->length, get_label_data->offset);
 
@@ -656,7 +745,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
 
     status = nvdimm_rw_label_data_check(nvdimm, set_label_data->offset,
                                         set_label_data->length);
-    if (status != 0 /* Success */) {
+    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
         nvdimm_dsm_no_payload(status, dsm_mem_addr);
         return;
     }
@@ -666,7 +755,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
 
     nvc->write_label_data(nvdimm, set_label_data->in_buf,
                           set_label_data->length, set_label_data->offset);
-    nvdimm_dsm_no_payload(0 /* Success */, dsm_mem_addr);
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS, dsm_mem_addr);
 }
 
 static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
@@ -717,7 +806,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
         break;
     }
 
-    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
 }
 
 static uint64_t
@@ -730,6 +819,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
 static void
 nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 {
+    AcpiNVDIMMState *state = opaque;
     NvdimmDsmIn *in;
     hwaddr dsm_mem_addr = val;
 
@@ -753,7 +843,12 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     if (in->revision != 0x1 /* Currently we only support DSM Spec Rev1. */) {
         nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
                      in->revision, 0x1);
-        nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+        nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
+        goto exit;
+    }
+
+    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
+        nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
         goto exit;
     }
 
@@ -809,9 +904,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
 #define NVDIMM_DSM_OUT_BUF      "ODAT"
 
+#define NVDIMM_DSM_RFIT_STATUS  "RSTA"
+
+#define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
+
 static void nvdimm_build_common_dsm(Aml *dev)
 {
-    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
+    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
     Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
     Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
     uint8_t byte_list[1];
@@ -900,9 +999,15 @@ static void nvdimm_build_common_dsm(Aml *dev)
                /* UUID for NVDIMM Root Device */, expected_uuid));
     aml_append(method, ifctx);
     elsectx = aml_else();
-    aml_append(elsectx, aml_store(
+    ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
+    aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
+               /* UUID for QEMU internal use */), expected_uuid));
+    aml_append(elsectx, ifctx);
+    elsectx2 = aml_else();
+    aml_append(elsectx2, aml_store(
                aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
                /* UUID for NVDIMM Devices */, expected_uuid));
+    aml_append(elsectx, elsectx2);
     aml_append(method, elsectx);
 
     uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
@@ -919,7 +1024,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
     aml_append(unsupport, ifctx);
 
     /* No function is supported yet. */
-    byte_list[0] = 1 /* Not Supported */;
+    byte_list[0] = NVDIMM_DSM_RET_STATUS_UNSUPPORT;
     aml_append(unsupport, aml_return(aml_buffer(1, byte_list)));
     aml_append(method, unsupport);
 
@@ -982,6 +1087,101 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
     aml_append(dev, method);
 }
 
+static void nvdimm_build_fit_method(Aml *dev)
+{
+    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
+    Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
+
+    buf = aml_local(0);
+    buf_size = aml_local(1);
+    fit = aml_local(2);
+
+    aml_append(dev, aml_name_decl(NVDIMM_DSM_RFIT_STATUS, aml_int(0)));
+
+    /* build helper function, RFIT. */
+    method = aml_method("RFIT", 1, AML_SERIALIZED);
+    aml_append(method, aml_name_decl("OFST", aml_int(0)));
+
+    /* 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(NVDIMM_QEMU_RSVD_UUID),
+                            aml_int(1) /* Revision 1 */,
+                            aml_int(0x1) /* Read FIT */,
+                            pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
+    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"));
+
+    aml_append(method, aml_store(aml_name("STAU"),
+                                 aml_name(NVDIMM_DSM_RFIT_STATUS)));
+
+     /* 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_create_dword_field(buf,
+               aml_int(4) /* offset at byte 4 */, "LENG"));
+    aml_append(method, aml_store(aml_name("LENG"), 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(8 * 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_SERIALIZED);
+    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));
+
+    /*
+     * if fit buffer was changed during RFIT, read from the beginning
+     * again.
+     */
+    ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
+                             aml_int(NVDIMM_DSM_RET_STATUS_FIT_CHANGED)));
+    aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
+    aml_append(ifctx, aml_store(aml_int(0), offset));
+    aml_append(whilectx, ifctx);
+
+    elsectx = aml_else();
+
+    /* 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(elsectx, ifctx);
+
+    /* update the offset. */
+    aml_append(elsectx, aml_add(offset, buf_size, offset));
+    /* append the data we read out to the fit buffer. */
+    aml_append(elsectx, aml_concatenate(fit, buf, fit));
+    aml_append(whilectx, elsectx);
+    aml_append(method, whilectx);
+
+    aml_append(dev, method);
+}
+
 static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
 {
     uint32_t slot;
@@ -1040,6 +1240,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_method(dev);
 
     nvdimm_build_nvdimm_devices(dev, ram_slots);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Stefan Hajnoczi]

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

On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:
> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>  }
>  
> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */

Not worth changing but please make each logical change a separate patch
in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
constant renaming.  It's easier to review, bisect, and backport when
structured as separate patches.

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

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 11/03/2016 05:53 PM, Stefan Hajnoczi wrote:
> On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:
>> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>>  }
>>
>> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
>> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
>> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */
>
> Not worth changing but please make each logical change a separate patch
> in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
> constant renaming.  It's easier to review, bisect, and backport when
> structured as separate patches.
>

Yes, indeed. Thanks for your suggestion, will pay more attention. :P

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Thu, 3 Nov 2016 18:08:04 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 11/03/2016 05:53 PM, Stefan Hajnoczi wrote:
> > On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:  
> >> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
> >>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
> >>  }
> >>
> >> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
> >> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
> >> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */  
> >
> > Not worth changing but please make each logical change a separate patch
> > in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
> > constant renaming.  It's easier to review, bisect, and backport when
> > structured as separate patches.
> >  
> 
> Yes, indeed. Thanks for your suggestion, will pay more attention. :P
just do renaming first as separate patch
and then hotplug patches on top

> --
> 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

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Thu,  3 Nov 2016 11:51:29 +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
s/_FIT/_FIT method/

the same applies to subj. line

> 
> As FIT buffer is not completely mapped into guest address space, so a
> new function, Read FIT whose UUID is UUID
> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> is concatenated before _FIT return

Commit message hard to read/parse, it might be better if I'd use simple
short sentences.


> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
>  hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 271 insertions(+), 17 deletions(-)
> 
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 0fdd251..364e832 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
>     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
>     NVDIMM Firmware Interface Table (NFIT).
>  
> -QEMU NVDIMM Implemention
> -========================
> +QEMU NVDIMM Implementation
> +==========================
>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
>  for NVDIMM ACPI.
>  
> @@ -82,6 +82,16 @@ Memory:
>     ACPI writes _DSM Input Data (based on the offset in the page):
>     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
>                  Root device.
> +
> +                The handle is completely QEMU internal thing, the values in
> +                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
[1, 0xFFFF] indicate nvdimm device
and move 0 to reserved section

> +                root device named NVDR), other values are reserved by other
s/by/for/

> +                purpose.
s/purpose/purposes/

> +                Current reserved handle:
s/Current reserved handle/Reserved handles/

> +                0x10000 is reserved for QEMU internal DSM function called on
> +                the root device.
description is too obscure, I wonder if it could be more specific


>     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
>     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
>     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
> @@ -127,6 +137,49 @@ _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.
drop it

> +1) Read FIT
> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
> +   function (private QEMU function)
not necessary, drop it. Maybe extend UUID description in
"Input parameters:" section


> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from
s/Read_FIT function/_DSM method/

> +   QEMU in 1 page sized increments which are then concatenated and returned
> +   as _FIT method result.
> +
> +   Input parameters:
> +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> +   Arg1 – Revision ID (set to 1)
> +   Arg2 - Function Index, 0x1
> +   Arg3 - A package containing a buffer whose layout is as follows:
> +
> +   +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset |                 Description               |
> +   +----------+--------+--------+-------------------------------------------+
> +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
> +   |          |        |        | read from                                 |
> +   +----------+--------+--------+-------------------------------------------+
> +
> +   Output:
> +   +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset |                 Description               |
> +   +----------+--------+--------+-------------------------------------------+
> +   |          |        |        | return status codes                       |
> +   |          |        |        | 0x100 - error caused by NFIT update while |
> +   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
> +   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
> +   +----------+--------+--------+-------------------------------------------+
> +   | length   |   4    |   4    | The fit size                              |           
> +   +----------+-----------------+-------------------------------------------+
> +   | fit data | Varies |   8    | FIT data, its size is indicated by length |
> +   |          |        |        | filed above                               |
> +   +----------+--------+--------+-------------------------------------------+
> +
> +   The FIT offset is maintained by the OSPM itself, current offset plus
> +   the length returned by the function is the next offset we should read.
there shouldn't be 'we' or anything personal in spec

> +   When all the FIT data has been read out, zero length is returned.
> +
> +   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
> +   again).

PS:
 fix typos and fix spelling/grammatical errors you are adding in above text.


> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 9fee077..593ac0d 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -484,6 +484,23 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>                    offsetof(NvdimmDsmIn, arg3) > 4096);
>  
> +struct NvdimmFuncReadFITIn {
> +    uint32_t offset; /* the offset into 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. */
> +    uint32_t length; /* the length of fit. */
redundant as len field above already has it all.

just drop this and describe properly 'len' in spec section
i.e. len: length of entire returned data (including the header)

> +    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)
>  {
> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>  }
>  
> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */
> +#define NVDIMM_DSM_RET_STATUS_FIT_CHANGED    0x100 /* FIT Changed */
> +
> +#define NVDIMM_QEMU_RSVD_HANDLE_ROOT         0x10000
> +
> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> +                                     hwaddr dsm_mem_addr)
> +{
> +    NvdimmFitBuffer *fit_buf = &state->fit_buf;
> +    NvdimmFuncReadFITIn *read_fit;
> +    NvdimmFuncReadFITOut *read_fit_out;
> +    GArray *fit;
> +    uint32_t read_len = 0, func_ret_status, offset;
> +    int size;
> +
> +    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> +    offset = le32_to_cpu(read_fit->offset);
> +
> +    fit = fit_buf->fit;
> +
> +    nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
> +                 offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> +
> +    /* It is the first time to read FIT. */
> +    if (!offset) {
> +        fit_buf->dirty = false;
> +    } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */
> +        func_ret_status = NVDIMM_DSM_RET_STATUS_FIT_CHANGED;
> +        goto exit;
> +    }
> +
> +    if (offset > fit->len) {
> +        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> +        goto exit;
> +    }
> +
> +    func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
> +    read_len = MIN(fit->len - offset, 4096 - sizeof(NvdimmFuncReadFITOut));
> +
> +exit:
> +    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);
> +    read_fit_out->length = cpu_to_le32(read_len);
> +    memcpy(read_fit_out->fit, fit->data + offset, read_len);
> +
> +    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
> +
> +    g_free(read_fit_out);
> +}
> +
> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> +                                     hwaddr dsm_mem_addr)
function name doesn't make any sense to me

> +{
> +    switch (in->function) {
> +    case 0x0:
> +        nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
> +        return;
> +    case 0x1 /* Read FIT */:
> +        nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
> +        return;
> +    }
> +
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
> +}
> +
>  static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>  {
>      /*
> @@ -563,7 +651,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
>  
>      nvdimm_debug("label_size %#x, max_xfer %#x.\n", label_size, mxfer);
>  
> -    label_size_out.func_ret_status = cpu_to_le32(0 /* Success */);
> +    label_size_out.func_ret_status = cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
>      label_size_out.label_size = cpu_to_le32(label_size);
>      label_size_out.max_xfer = cpu_to_le32(mxfer);
>  
> @@ -574,7 +662,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
>  static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
>                                             uint32_t offset, uint32_t length)
>  {
> -    uint32_t ret = 3 /* Invalid Input Parameters */;
> +    uint32_t ret = NVDIMM_DSM_RET_STATUS_INVALID;
>  
>      if (offset + length < offset) {
>          nvdimm_debug("offset %#x + length %#x is overflow.\n", offset,
> @@ -594,7 +682,7 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
>          return ret;
>      }
>  
> -    return 0 /* Success */;
> +    return NVDIMM_DSM_RET_STATUS_SUCCESS;
>  }
>  
>  /*
> @@ -618,7 +706,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>  
>      status = nvdimm_rw_label_data_check(nvdimm, get_label_data->offset,
>                                          get_label_data->length);
> -    if (status != 0 /* Success */) {
> +    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
>          nvdimm_dsm_no_payload(status, dsm_mem_addr);
>          return;
>      }
> @@ -628,7 +716,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>      get_label_data_out = g_malloc(size);
>  
>      get_label_data_out->len = cpu_to_le32(size);
> -    get_label_data_out->func_ret_status = cpu_to_le32(0 /* Success */);
> +    get_label_data_out->func_ret_status =
> +                                cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
>      nvc->read_label_data(nvdimm, get_label_data_out->out_buf,
>                           get_label_data->length, get_label_data->offset);
>  
> @@ -656,7 +745,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>  
>      status = nvdimm_rw_label_data_check(nvdimm, set_label_data->offset,
>                                          set_label_data->length);
> -    if (status != 0 /* Success */) {
> +    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
>          nvdimm_dsm_no_payload(status, dsm_mem_addr);
>          return;
>      }
> @@ -666,7 +755,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>  
>      nvc->write_label_data(nvdimm, set_label_data->in_buf,
>                            set_label_data->length, set_label_data->offset);
> -    nvdimm_dsm_no_payload(0 /* Success */, dsm_mem_addr);
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS, dsm_mem_addr);
>  }
>  
>  static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> @@ -717,7 +806,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>          break;
>      }
>  
> -    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
>  }
>  
>  static uint64_t
> @@ -730,6 +819,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
>  static void
>  nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>  {
> +    AcpiNVDIMMState *state = opaque;
>      NvdimmDsmIn *in;
>      hwaddr dsm_mem_addr = val;
>  
> @@ -753,7 +843,12 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      if (in->revision != 0x1 /* Currently we only support DSM Spec Rev1. */) {
>          nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
>                       in->revision, 0x1);
> -        nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +        nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
> +        goto exit;
> +    }
> +
> +    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> +        nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
>          goto exit;
>      }
>  
> @@ -809,9 +904,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>  #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
>  #define NVDIMM_DSM_OUT_BUF      "ODAT"
>  
> +#define NVDIMM_DSM_RFIT_STATUS  "RSTA"
> +
> +#define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
> +
>  static void nvdimm_build_common_dsm(Aml *dev)
>  {
> -    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
> +    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
>      Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
>      Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
>      uint8_t byte_list[1];
> @@ -900,9 +999,15 @@ static void nvdimm_build_common_dsm(Aml *dev)
>                 /* UUID for NVDIMM Root Device */, expected_uuid));
>      aml_append(method, ifctx);
>      elsectx = aml_else();
> -    aml_append(elsectx, aml_store(
> +    ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
> +    aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
> +               /* UUID for QEMU internal use */), expected_uuid));
> +    aml_append(elsectx, ifctx);
> +    elsectx2 = aml_else();
> +    aml_append(elsectx2, aml_store(
>                 aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
>                 /* UUID for NVDIMM Devices */, expected_uuid));
> +    aml_append(elsectx, elsectx2);
>      aml_append(method, elsectx);
>  
>      uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
> @@ -919,7 +1024,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
>      aml_append(unsupport, ifctx);
>  
>      /* No function is supported yet. */
> -    byte_list[0] = 1 /* Not Supported */;
> +    byte_list[0] = NVDIMM_DSM_RET_STATUS_UNSUPPORT;
>      aml_append(unsupport, aml_return(aml_buffer(1, byte_list)));
>      aml_append(method, unsupport);
>  
> @@ -982,6 +1087,101 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>      aml_append(dev, method);
>  }
>  
> +static void nvdimm_build_fit_method(Aml *dev)
> +{
> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
> +    Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
> +
> +    buf = aml_local(0);
> +    buf_size = aml_local(1);
> +    fit = aml_local(2);
> +
> +    aml_append(dev, aml_name_decl(NVDIMM_DSM_RFIT_STATUS, aml_int(0)));
> +
> +    /* build helper function, RFIT. */
> +    method = aml_method("RFIT", 1, AML_SERIALIZED);
> +    aml_append(method, aml_name_decl("OFST", aml_int(0)));
> +
> +    /* 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(NVDIMM_QEMU_RSVD_UUID),
> +                            aml_int(1) /* Revision 1 */,
> +                            aml_int(0x1) /* Read FIT */,
> +                            pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
> +    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"));
> +
> +    aml_append(method, aml_store(aml_name("STAU"),
> +                                 aml_name(NVDIMM_DSM_RFIT_STATUS)));
> +
> +     /* 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_create_dword_field(buf,
> +               aml_int(4) /* offset at byte 4 */, "LENG"));
> +    aml_append(method, aml_store(aml_name("LENG"), 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(8 * 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_SERIALIZED);
> +    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));
> +
> +    /*
> +     * if fit buffer was changed during RFIT, read from the beginning
> +     * again.
> +     */
> +    ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
> +                             aml_int(NVDIMM_DSM_RET_STATUS_FIT_CHANGED)));
> +    aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
> +    aml_append(ifctx, aml_store(aml_int(0), offset));
> +    aml_append(whilectx, ifctx);
> +
> +    elsectx = aml_else();
> +
> +    /* 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(elsectx, ifctx);
> +
> +    /* update the offset. */
> +    aml_append(elsectx, aml_add(offset, buf_size, offset));
> +    /* append the data we read out to the fit buffer. */
> +    aml_append(elsectx, aml_concatenate(fit, buf, fit));
> +    aml_append(whilectx, elsectx);
> +    aml_append(method, whilectx);
> +
> +    aml_append(dev, method);
> +}
> +
>  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  {
>      uint32_t slot;
> @@ -1040,6 +1240,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_method(dev);
>  
>      nvdimm_build_nvdimm_devices(dev, ram_slots);
>  

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 11/03/2016 07:58 PM, Igor Mammedov wrote:
> On Thu,  3 Nov 2016 11:51:29 +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
> s/_FIT/_FIT method/
>
> the same applies to subj. line

Okay.

>
>>
>> As FIT buffer is not completely mapped into guest address space, so a
>> new function, Read FIT whose UUID is UUID
>> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
>> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
>> is concatenated before _FIT return
>
> Commit message hard to read/parse, it might be better if I'd use simple
> short sentences.

Okay, will change it to:

As FIT buffer is not completely mapped into guest address space, Read_FIT
method is introduced to read NFIT structures blob from QEMU, 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>
>> ---
>>  docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
>>  hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 271 insertions(+), 17 deletions(-)
>>
>> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
>> index 0fdd251..364e832 100644
>> --- a/docs/specs/acpi_nvdimm.txt
>> +++ b/docs/specs/acpi_nvdimm.txt
>> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
>>     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
>>     NVDIMM Firmware Interface Table (NFIT).
>>
>> -QEMU NVDIMM Implemention
>> -========================
>> +QEMU NVDIMM Implementation
>> +==========================
>>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
>>  for NVDIMM ACPI.
>>
>> @@ -82,6 +82,16 @@ Memory:
>>     ACPI writes _DSM Input Data (based on the offset in the page):
>>     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
>>                  Root device.
>> +
>> +                The handle is completely QEMU internal thing, the values in
>> +                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
> [1, 0xFFFF] indicate nvdimm device
> and move 0 to reserved section

Okay.

>
>> +                root device named NVDR), other values are reserved by other
> s/by/for/

okay.

>
>> +                purpose.
> s/purpose/purposes/

okay.

>
>> +                Current reserved handle:
> s/Current reserved handle/Reserved handles/
>
>> +                0x10000 is reserved for QEMU internal DSM function called on
>> +                the root device.
> description is too obscure, I wonder if it could be more specific

I would like to make these reserved values more generic in order to
support more QEMU reserved _DSM methods in the further. So, i planed:
UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is dedicated for QEMU reserved
methods. Handle 0x10000 indicates the method is for the root device.
0x10001 ~ 0x1FFFF indicates the method is for the nvdimm devices.

As currently we do not have reserved method on nvdimm device, so i
only document 0x1000 for the root device.

>
>
>>     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
>>     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
>>     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
>> @@ -127,6 +137,49 @@ _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.
> drop it
>
>> +1) Read FIT
>> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
>> +   function (private QEMU function)
> not necessary, drop it. Maybe extend UUID description in
> "Input parameters:" section
>

okay.

>
>> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from
> s/Read_FIT function/_DSM method/

okay.

>
>> +   QEMU in 1 page sized increments which are then concatenated and returned
>> +   as _FIT method result.
>> +
>> +   Input parameters:
>> +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
>> +   Arg1 – Revision ID (set to 1)
>> +   Arg2 - Function Index, 0x1
>> +   Arg3 - A package containing a buffer whose layout is as follows:
>> +
>> +   +----------+--------+--------+-------------------------------------------+
>> +   |  Field   | Length | Offset |                 Description               |
>> +   +----------+--------+--------+-------------------------------------------+
>> +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
>> +   |          |        |        | read from                                 |
>> +   +----------+--------+--------+-------------------------------------------+
>> +
>> +   Output:
>> +   +----------+--------+--------+-------------------------------------------+
>> +   |  Field   | Length | Offset |                 Description               |
>> +   +----------+--------+--------+-------------------------------------------+
>> +   |          |        |        | return status codes                       |
>> +   |          |        |        | 0x100 - error caused by NFIT update while |
>> +   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
>> +   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
>> +   +----------+--------+--------+-------------------------------------------+
>> +   | length   |   4    |   4    | The fit size                              |
>> +   +----------+-----------------+-------------------------------------------+
>> +   | fit data | Varies |   8    | FIT data, its size is indicated by length |
>> +   |          |        |        | filed above                               |
>> +   +----------+--------+--------+-------------------------------------------+
>> +
>> +   The FIT offset is maintained by the OSPM itself, current offset plus
>> +   the length returned by the function is the next offset we should read.
> there shouldn't be 'we' or anything personal in spec

Okay, change it to OSPM. :)

>
>> +   When all the FIT data has been read out, zero length is returned.
>> +
>> +   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
>> +   again).
>
> PS:
>  fix typos and fix spelling/grammatical errors you are adding in above text.

Sorry for the poor English, will check it carefully.

>
>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 9fee077..593ac0d 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -484,6 +484,23 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>>                    offsetof(NvdimmDsmIn, arg3) > 4096);
>>
>> +struct NvdimmFuncReadFITIn {
>> +    uint32_t offset; /* the offset into 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. */
>> +    uint32_t length; /* the length of fit. */
> redundant as len field above already has it all.
>
> just drop this and describe properly 'len' in spec section
> i.e. len: length of entire returned data (including the header)

Okay, i will change the spec like this:

    QEMU Writes Output Data (based on the offset in the page):
    [0x0 - 0x3]: 4 bytes, length of entire returned data (including the header)

And drop the length field in Read_Fit return buffer, doc
the fit buffer like this:

    +----------+--------+--------+-------------------------------------------+
    |  Field   | Length | Offset |                 Description               |
    +----------+--------+--------+-------------------------------------------+
    |          |        |        | return status codes                       |
    |          |        |        | 0x100 - error caused by NFIT update while |
    | status   |   4    |   0    | read by _FIT wasn't completed, other      |
    |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
    +----------+--------+--------+-------------------------------------------+
    | fit data | Varies |   8    | FIT data, The remaining size in the       |
    |          |        |        | returned buffer is used by FIT            |
    +----------+--------+--------+-------------------------------------------+



>> +}
>> +
>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>> +                                     hwaddr dsm_mem_addr)
> function name doesn't make any sense to me

As i explained above, handle 0x10000 indicates the reserved _DSM method is
called on the root device...

It makes sense now? :)

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Thu, 3 Nov 2016 20:21:05 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 11/03/2016 07:58 PM, Igor Mammedov wrote:
> > On Thu,  3 Nov 2016 11:51:29 +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  
> > s/_FIT/_FIT method/
> >
> > the same applies to subj. line  
> 
> Okay.
> 
> >  
> >>
> >> As FIT buffer is not completely mapped into guest address space, so a
> >> new function, Read FIT whose UUID is UUID
> >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> >> is concatenated before _FIT return  
> >
> > Commit message hard to read/parse, it might be better if I'd use simple
> > short sentences.  
> 
> Okay, will change it to:
> 
> As FIT buffer is not completely mapped into guest address space, Read_FIT
> method is introduced to read NFIT structures blob from QEMU, 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>
> >> ---
> >>  docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
> >>  hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
> >>  2 files changed, 271 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> >> index 0fdd251..364e832 100644
> >> --- a/docs/specs/acpi_nvdimm.txt
> >> +++ b/docs/specs/acpi_nvdimm.txt
> >> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
> >>     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
> >>     NVDIMM Firmware Interface Table (NFIT).
> >>
> >> -QEMU NVDIMM Implemention
> >> -========================
> >> +QEMU NVDIMM Implementation
> >> +==========================
> >>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
> >>  for NVDIMM ACPI.
> >>
> >> @@ -82,6 +82,16 @@ Memory:
> >>     ACPI writes _DSM Input Data (based on the offset in the page):
> >>     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
> >>                  Root device.
> >> +
> >> +                The handle is completely QEMU internal thing, the values in
> >> +                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm  
> > [1, 0xFFFF] indicate nvdimm device
> > and move 0 to reserved section  
> 
> Okay.
> 
> >  
> >> +                root device named NVDR), other values are reserved by other  
> > s/by/for/  
> 
> okay.
> 
> >  
> >> +                purpose.  
> > s/purpose/purposes/  
> 
> okay.
> 
> >  
> >> +                Current reserved handle:  
> > s/Current reserved handle/Reserved handles/
> >  
> >> +                0x10000 is reserved for QEMU internal DSM function called on
> >> +                the root device.  
> > description is too obscure, I wonder if it could be more specific  
> 
> I would like to make these reserved values more generic in order to
> support more QEMU reserved _DSM methods in the further. So, i planed:
> UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is dedicated for QEMU reserved
> methods. Handle 0x10000 indicates the method is for the root device.
> 0x10001 ~ 0x1FFFF indicates the method is for the nvdimm devices.
> 
> As currently we do not have reserved method on nvdimm device, so i
> only document 0x1000 for the root device.
> 
> >
> >  
> >>     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
> >>     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
> >>     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
> >> @@ -127,6 +137,49 @@ _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.  
> > drop it
> >  
> >> +1) Read FIT
> >> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
> >> +   function (private QEMU function)  
> > not necessary, drop it. Maybe extend UUID description in
> > "Input parameters:" section
> >  
> 
> okay.
> 
> >  
> >> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from  
> > s/Read_FIT function/_DSM method/  
> 
> okay.
> 
> >  
> >> +   QEMU in 1 page sized increments which are then concatenated and returned
> >> +   as _FIT method result.
> >> +
> >> +   Input parameters:
> >> +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> >> +   Arg1 – Revision ID (set to 1)
> >> +   Arg2 - Function Index, 0x1
> >> +   Arg3 - A package containing a buffer whose layout is as follows:
> >> +
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   |  Field   | Length | Offset |                 Description               |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
> >> +   |          |        |        | read from                                 |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +
> >> +   Output:
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   |  Field   | Length | Offset |                 Description               |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   |          |        |        | return status codes                       |
> >> +   |          |        |        | 0x100 - error caused by NFIT update while |
> >> +   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
> >> +   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   | length   |   4    |   4    | The fit size                              |
> >> +   +----------+-----------------+-------------------------------------------+
> >> +   | fit data | Varies |   8    | FIT data, its size is indicated by length |
> >> +   |          |        |        | filed above                               |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +
> >> +   The FIT offset is maintained by the OSPM itself, current offset plus
> >> +   the length returned by the function is the next offset we should read.  
> > there shouldn't be 'we' or anything personal in spec  
> 
> Okay, change it to OSPM. :)
> 
> >  
> >> +   When all the FIT data has been read out, zero length is returned.
> >> +
> >> +   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
> >> +   again).  
> >
> > PS:
> >  fix typos and fix spelling/grammatical errors you are adding in above text.  
> 
> Sorry for the poor English, will check it carefully.
> 
> >
> >  
> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> >> index 9fee077..593ac0d 100644
> >> --- a/hw/acpi/nvdimm.c
> >> +++ b/hw/acpi/nvdimm.c
> >> @@ -484,6 +484,23 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
> >>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> >>                    offsetof(NvdimmDsmIn, arg3) > 4096);
> >>
> >> +struct NvdimmFuncReadFITIn {
> >> +    uint32_t offset; /* the offset into 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. */
> >> +    uint32_t length; /* the length of fit. */  
> > redundant as len field above already has it all.
> >
> > just drop this and describe properly 'len' in spec section
> > i.e. len: length of entire returned data (including the header)  
> 
> Okay, i will change the spec like this:
> 
>     QEMU Writes Output Data (based on the offset in the page):
>     [0x0 - 0x3]: 4 bytes, length of entire returned data (including the header)
> 
> And drop the length field in Read_Fit return buffer, doc
> the fit buffer like this:
> 
>     +----------+--------+--------+-------------------------------------------+
>     |  Field   | Length | Offset |                 Description               |
>     +----------+--------+--------+-------------------------------------------+
you need to add length here, otherwise this table is not correct


>     |          |        |        | return status codes                       |
>     |          |        |        | 0x100 - error caused by NFIT update while |
>     | status   |   4    |   0    | read by _FIT wasn't completed, other      |
>     |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
>     +----------+--------+--------+-------------------------------------------+
>     | fit data | Varies |   8    | FIT data, The remaining size in the       |
>     |          |        |        | returned buffer is used by FIT            |
>     +----------+--------+--------+-------------------------------------------+
> 
> 
> 
> >> +}
> >> +
> >> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> >> +                                     hwaddr dsm_mem_addr)  
> > function name doesn't make any sense to me  
> 
> As i explained above, handle 0x10000 indicates the reserved _DSM method is
> called on the root device...
> 
> It makes sense now? :)
function name should reflect what it does,
i.e use verb and I see only nouns here.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 11/03/2016 09:00 PM, Igor Mammedov wrote:




>>> just drop this and describe properly 'len' in spec section
>>> i.e. len: length of entire returned data (including the header)
>>
>> Okay, i will change the spec like this:
>>
>>     QEMU Writes Output Data (based on the offset in the page):
>>     [0x0 - 0x3]: 4 bytes, length of entire returned data (including the header)
>>
>> And drop the length field in Read_Fit return buffer, doc
>> the fit buffer like this:
>>
>>     +----------+--------+--------+-------------------------------------------+
>>     |  Field   | Length | Offset |                 Description               |
>>     +----------+--------+--------+-------------------------------------------+
> you need to add length here, otherwise this table is not correct

Ah, so i am confused.

struct NvdimmFuncReadFITOut definition is based on the layout of
Read_FI output. You suggested to drop the length filed in NvdimmFuncReadFITOut
but keep it in the layout, it is not consistent.

I missed something?

>
>
>>     |          |        |        | return status codes                       |
>>     |          |        |        | 0x100 - error caused by NFIT update while |
>>     | status   |   4    |   0    | read by _FIT wasn't completed, other      |
>>     |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
>>     +----------+--------+--------+-------------------------------------------+
>>     | fit data | Varies |   8    | FIT data, The remaining size in the       |
>>     |          |        |        | returned buffer is used by FIT            |
>>     +----------+--------+--------+-------------------------------------------+
>>
>>
>>
>>>> +}
>>>> +
>>>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>>>> +                                     hwaddr dsm_mem_addr)
>>> function name doesn't make any sense to me
>>
>> As i explained above, handle 0x10000 indicates the reserved _DSM method is
>> called on the root device...
>>
>> It makes sense now? :)
> function name should reflect what it does,
> i.e use verb and I see only nouns here.

Got it, will change it to: nvdimm_handle_reserved_root_method().

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Thu, 3 Nov 2016 21:02:22 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> 
> 
> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> 
> 
> 
> 
> >>> just drop this and describe properly 'len' in spec section
> >>> i.e. len: length of entire returned data (including the header)
> >>
> >> Okay, i will change the spec like this:
> >>
> >>     QEMU Writes Output Data (based on the offset in the page):
> >>     [0x0 - 0x3]: 4 bytes, length of entire returned data
> >> (including the header)
> >>
> >> And drop the length field in Read_Fit return buffer, doc
> >> the fit buffer like this:
> >>
> >>     +----------+--------+--------+-------------------------------------------+
> >>     |  Field   | Length | Offset |
> >> Description               |
> >> +----------+--------+--------+-------------------------------------------+
> > you need to add length here, otherwise this table is not correct
> 
> Ah, so i am confused.
> 
> struct NvdimmFuncReadFITOut definition is based on the layout of
> Read_FI output. You suggested to drop the length filed in
> NvdimmFuncReadFITOut but keep it in the layout, it is not consistent.
> 
> I missed something?

+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;

--------------------------------
| field       | len | off | desc...
--------------------------------
| length      |  4  |  0  | ....
--------------------------------
| status      |  4  |  4  | ....
--------------------------------
| fit data    | ................

i.e. you were forgetting to add length in spec so offsets were wrong
even for described fields.

> 
> >
> >
> >>     |          |        |        | return status
> >> codes                       | |          |        |        | 0x100
> >> - error caused by NFIT update while | | status   |   4    |   0
> >> | read by _FIT wasn't completed, other      | |          |
> >> |        | codes follow Chapter 3 in DSM Spec Rev1   |
> >> +----------+--------+--------+-------------------------------------------+
> >> | fit data | Varies |   8    | FIT data, The remaining size in
> >> the       | |          |        |        | returned buffer is used
> >> by FIT            |
> >> +----------+--------+--------+-------------------------------------------+
> >>
> >>
> >>
> >>>> +}
> >>>> +
> >>>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state,
> >>>> NvdimmDsmIn *in,
> >>>> +                                     hwaddr dsm_mem_addr)
> >>> function name doesn't make any sense to me
> >>
> >> As i explained above, handle 0x10000 indicates the reserved _DSM
> >> method is called on the root device...
> >>
> >> It makes sense now? :)
> > function name should reflect what it does,
> > i.e use verb and I see only nouns here.
> 
> Got it, will change it to: nvdimm_handle_reserved_root_method().
> 

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> On Thu, 3 Nov 2016 21:02:22 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>>
>>
>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
>>
>>
>>
>>
>>>>> just drop this and describe properly 'len' in spec section
>>>>> i.e. len: length of entire returned data (including the header)
>>>>
>>>> Okay, i will change the spec like this:
>>>>
>>>>     QEMU Writes Output Data (based on the offset in the page):
>>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
>>>> (including the header)
>>>>
>>>> And drop the length field in Read_Fit return buffer, doc
>>>> the fit buffer like this:
>>>>
>>>>     +----------+--------+--------+-------------------------------------------+
>>>>     |  Field   | Length | Offset |
>>>> Description               |
>>>> +----------+--------+--------+-------------------------------------------+
>>> you need to add length here, otherwise this table is not correct
>>
>> Ah, so i am confused.
>>
>> struct NvdimmFuncReadFITOut definition is based on the layout of
>> Read_FI output. You suggested to drop the length filed in
>> NvdimmFuncReadFITOut but keep it in the layout, it is not consistent.
>>
>> I missed something?
>
> +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;
>
> --------------------------------
> | field       | len | off | desc...
> --------------------------------
> | length      |  4  |  0  | ....
> --------------------------------
> | status      |  4  |  4  | ....
> --------------------------------
> | fit data    | ................
>
> i.e. you were forgetting to add length in spec so offsets were wrong
> even for described fields.


We can not do this.

@len is used by QEMU emulation to count the size of the buffer that
_DSM should return. It's only used in NVDIMM_COMMON_DSM method which
is shared by the DSM method from VM and Read_Fit.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Thu, 3 Nov 2016 22:53:43 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> 
> 
> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 21:02:22 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> >>
> >>
> >> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>
> >>
> >>
> >>
> >>>>> just drop this and describe properly 'len' in spec section
> >>>>> i.e. len: length of entire returned data (including the header)
> >>>>
> >>>> Okay, i will change the spec like this:
> >>>>
> >>>>     QEMU Writes Output Data (based on the offset in the page):
> >>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>> (including the header)
> >>>>
> >>>> And drop the length field in Read_Fit return buffer, doc
> >>>> the fit buffer like this:
> >>>>
> >>>>     +----------+--------+--------+-------------------------------------------+
> >>>>     |  Field   | Length | Offset |
> >>>> Description               |
> >>>> +----------+--------+--------+-------------------------------------------+
> >>> you need to add length here, otherwise this table is not correct
> >>
> >> Ah, so i am confused.
> >>
> >> struct NvdimmFuncReadFITOut definition is based on the layout of
> >> Read_FI output. You suggested to drop the length filed in
> >> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >> consistent.
> >>
> >> I missed something?
> >
> > +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;
> >
> > --------------------------------
> > | field       | len | off | desc...
> > --------------------------------
> > | length      |  4  |  0  | ....
> > --------------------------------
> > | status      |  4  |  4  | ....
> > --------------------------------
> > | fit data    | ................
> >
> > i.e. you were forgetting to add length in spec so offsets were wrong
> > even for described fields.
> 
> 
> We can not do this.
> 
> @len is used by QEMU emulation to count the size of the buffer that
> _DSM should return. It's only used in NVDIMM_COMMON_DSM method which
> is shared by the DSM method from VM and Read_Fit.
spec describes buffer layout independently from AML that uses it,
so it should describe whole data structure.

Then it's upto guest how to read this data, it could be QEMU generated
AML (as it's here) or some other driver or even BIOS.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> On Thu, 3 Nov 2016 22:53:43 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>>
>>
>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
>>> On Thu, 3 Nov 2016 21:02:22 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>>>
>>>>
>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
>>>>
>>>>
>>>>
>>>>
>>>>>>> just drop this and describe properly 'len' in spec section
>>>>>>> i.e. len: length of entire returned data (including the header)
>>>>>>
>>>>>> Okay, i will change the spec like this:
>>>>>>
>>>>>>     QEMU Writes Output Data (based on the offset in the page):
>>>>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
>>>>>> (including the header)
>>>>>>
>>>>>> And drop the length field in Read_Fit return buffer, doc
>>>>>> the fit buffer like this:
>>>>>>
>>>>>>     +----------+--------+--------+-------------------------------------------+
>>>>>>     |  Field   | Length | Offset |
>>>>>> Description               |
>>>>>> +----------+--------+--------+-------------------------------------------+
>>>>> you need to add length here, otherwise this table is not correct
>>>>
>>>> Ah, so i am confused.
>>>>
>>>> struct NvdimmFuncReadFITOut definition is based on the layout of
>>>> Read_FI output. You suggested to drop the length filed in
>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
>>>> consistent.
>>>>
>>>> I missed something?
>>>
>>> +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;
>>>
>>> --------------------------------
>>> | field       | len | off | desc...
>>> --------------------------------
>>> | length      |  4  |  0  | ....
>>> --------------------------------
>>> | status      |  4  |  4  | ....
>>> --------------------------------
>>> | fit data    | ................
>>>
>>> i.e. you were forgetting to add length in spec so offsets were wrong
>>> even for described fields.
>>
>>
>> We can not do this.
>>
>> @len is used by QEMU emulation to count the size of the buffer that
>> _DSM should return. It's only used in NVDIMM_COMMON_DSM method which
>> is shared by the DSM method from VM and Read_Fit.
> spec describes buffer layout independently from AML that uses it,
> so it should describe whole data structure.
>
> Then it's upto guest how to read this data, it could be QEMU generated
> AML (as it's here) or some other driver or even BIOS.

However, what we are talking about is Read_FIT method, so this is
the layout of Read_FIT output rather than all memory in the dsm page.

Actually, in the spec we already have documented the common len field:

    QEMU Writes Output Data (based on the offset in the page):
    [0x0 - 0x3]: 4 bytes, the length of result
    [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU

Also, i really do not hope to use this field to count the buffer size
returned by Read_FIT, we'd try the best to reuse existing DSM method
(NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Fri, 4 Nov 2016 00:17:00 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> 
> 
> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 22:53:43 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> >>
> >>
> >> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>>>> just drop this and describe properly 'len' in spec section
> >>>>>>> i.e. len: length of entire returned data (including the
> >>>>>>> header)
> >>>>>>
> >>>>>> Okay, i will change the spec like this:
> >>>>>>
> >>>>>>     QEMU Writes Output Data (based on the offset in the page):
> >>>>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>>>> (including the header)
> >>>>>>
> >>>>>> And drop the length field in Read_Fit return buffer, doc
> >>>>>> the fit buffer like this:
> >>>>>>
> >>>>>>     +----------+--------+--------+-------------------------------------------+
> >>>>>>     |  Field   | Length | Offset |
> >>>>>> Description               |
> >>>>>> +----------+--------+--------+-------------------------------------------+
> >>>>> you need to add length here, otherwise this table is not correct
> >>>>
> >>>> Ah, so i am confused.
> >>>>
> >>>> struct NvdimmFuncReadFITOut definition is based on the layout of
> >>>> Read_FI output. You suggested to drop the length filed in
> >>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >>>> consistent.
> >>>>
> >>>> I missed something?
> >>>
> >>> +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;
> >>>
> >>> --------------------------------
> >>> | field       | len | off | desc...
> >>> --------------------------------
> >>> | length      |  4  |  0  | ....
> >>> --------------------------------
> >>> | status      |  4  |  4  | ....
> >>> --------------------------------
> >>> | fit data    | ................
> >>>
> >>> i.e. you were forgetting to add length in spec so offsets were
> >>> wrong even for described fields.
> >>
> >>
> >> We can not do this.
> >>
> >> @len is used by QEMU emulation to count the size of the buffer that
> >> _DSM should return. It's only used in NVDIMM_COMMON_DSM method
> >> which is shared by the DSM method from VM and Read_Fit.
> > spec describes buffer layout independently from AML that uses it,
> > so it should describe whole data structure.
> >
> > Then it's upto guest how to read this data, it could be QEMU
> > generated AML (as it's here) or some other driver or even BIOS.
> 
> However, what we are talking about is Read_FIT method, so this is
> the layout of Read_FIT output rather than all memory in the dsm page.
> 
> Actually, in the spec we already have documented the common len field:
> 
>     QEMU Writes Output Data (based on the offset in the page):
>     [0x0 - 0x3]: 4 bytes, the length of result
>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> 
> Also, i really do not hope to use this field to count the buffer size
> returned by Read_FIT, we'd try the best to reuse existing DSM method
> (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.
buffer layout describes interface between QEMU and firmware (AML)
and it should describe it completely every time to avoid confusion.

See for example ACPI spec, NFIT table, SRAT table, ...
all table descriptions start with complete header.

If you skip length it rises question how much fit data are there,
meaning interface description isn't complete.

if you want to describe AML there you can do it after interface
description saying that common NCAL method extracts status and fit
data form dsm page and returns that as buffer object, but it's AML
impl. specific. I could write an alternative AML code that would deal
with dms page in its own way as long as I would know what write/read at
what offset.


> 
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> On Fri, 4 Nov 2016 00:17:00 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>>
>>
>> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
>>> On Thu, 3 Nov 2016 22:53:43 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>>>
>>>>
>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
>>>>> On Thu, 3 Nov 2016 21:02:22 +0800
>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>> just drop this and describe properly 'len' in spec section
>>>>>>>>> i.e. len: length of entire returned data (including the
>>>>>>>>> header)
>>>>>>>>
>>>>>>>> Okay, i will change the spec like this:
>>>>>>>>
>>>>>>>>     QEMU Writes Output Data (based on the offset in the page):
>>>>>>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
>>>>>>>> (including the header)
>>>>>>>>
>>>>>>>> And drop the length field in Read_Fit return buffer, doc
>>>>>>>> the fit buffer like this:
>>>>>>>>
>>>>>>>>     +----------+--------+--------+-------------------------------------------+
>>>>>>>>     |  Field   | Length | Offset |
>>>>>>>> Description               |
>>>>>>>> +----------+--------+--------+-------------------------------------------+
>>>>>>> you need to add length here, otherwise this table is not correct
>>>>>>
>>>>>> Ah, so i am confused.
>>>>>>
>>>>>> struct NvdimmFuncReadFITOut definition is based on the layout of
>>>>>> Read_FI output. You suggested to drop the length filed in
>>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
>>>>>> consistent.
>>>>>>
>>>>>> I missed something?
>>>>>
>>>>> +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;
>>>>>
>>>>> --------------------------------
>>>>> | field       | len | off | desc...
>>>>> --------------------------------
>>>>> | length      |  4  |  0  | ....
>>>>> --------------------------------
>>>>> | status      |  4  |  4  | ....
>>>>> --------------------------------
>>>>> | fit data    | ................
>>>>>
>>>>> i.e. you were forgetting to add length in spec so offsets were
>>>>> wrong even for described fields.
>>>>
>>>>
>>>> We can not do this.
>>>>
>>>> @len is used by QEMU emulation to count the size of the buffer that
>>>> _DSM should return. It's only used in NVDIMM_COMMON_DSM method
>>>> which is shared by the DSM method from VM and Read_Fit.
>>> spec describes buffer layout independently from AML that uses it,
>>> so it should describe whole data structure.
>>>
>>> Then it's upto guest how to read this data, it could be QEMU
>>> generated AML (as it's here) or some other driver or even BIOS.
>>
>> However, what we are talking about is Read_FIT method, so this is
>> the layout of Read_FIT output rather than all memory in the dsm page.
>>
>> Actually, in the spec we already have documented the common len field:
>>
>>     QEMU Writes Output Data (based on the offset in the page):
>>     [0x0 - 0x3]: 4 bytes, the length of result
>>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
>>
>> Also, i really do not hope to use this field to count the buffer size
>> returned by Read_FIT, we'd try the best to reuse existing DSM method
>> (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.
> buffer layout describes interface between QEMU and firmware (AML)
> and it should describe it completely every time to avoid confusion.
>
> See for example ACPI spec, NFIT table, SRAT table, ...
> all table descriptions start with complete header.

Okay. Understood. :)

>
> If you skip length it rises question how much fit data are there,
> meaning interface description isn't complete.

So how about introduce a length field in the output buffer just
as this patch did? I understand we are able to count the size
from dsm len, however, it can simplify the code a lot...

>
> if you want to describe AML there you can do it after interface
> description saying that common NCAL method extracts status and fit
> data form dsm page and returns that as buffer object, but it's AML
> impl. specific. I could write an alternative AML code that would deal
> with dms page in its own way as long as I would know what write/read at
> what offset.

Yes, i agree with you.

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Fri, 4 Nov 2016 00:53:06 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> 
> 
> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:17:00 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> >>> On Thu, 3 Nov 2016 22:53:43 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>>>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>>>> just drop this and describe properly 'len' in spec section
> >>>>>>>>> i.e. len: length of entire returned data (including the
> >>>>>>>>> header)
> >>>>>>>>
> >>>>>>>> Okay, i will change the spec like this:
> >>>>>>>>
> >>>>>>>>     QEMU Writes Output Data (based on the offset in the
> >>>>>>>> page): [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>>>>>> (including the header)
> >>>>>>>>
> >>>>>>>> And drop the length field in Read_Fit return buffer, doc
> >>>>>>>> the fit buffer like this:
> >>>>>>>>
> >>>>>>>>     +----------+--------+--------+-------------------------------------------+
> >>>>>>>>     |  Field   | Length | Offset |
> >>>>>>>> Description               |
> >>>>>>>> +----------+--------+--------+-------------------------------------------+
> >>>>>>> you need to add length here, otherwise this table is not
> >>>>>>> correct
> >>>>>>
> >>>>>> Ah, so i am confused.
> >>>>>>
> >>>>>> struct NvdimmFuncReadFITOut definition is based on the layout
> >>>>>> of Read_FI output. You suggested to drop the length filed in
> >>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >>>>>> consistent.
> >>>>>>
> >>>>>> I missed something?
> >>>>>
> >>>>> +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;
> >>>>>
> >>>>> --------------------------------
> >>>>> | field       | len | off | desc...
> >>>>> --------------------------------
> >>>>> | length      |  4  |  0  | ....
> >>>>> --------------------------------
> >>>>> | status      |  4  |  4  | ....
> >>>>> --------------------------------
> >>>>> | fit data    | ................
> >>>>>
> >>>>> i.e. you were forgetting to add length in spec so offsets were
> >>>>> wrong even for described fields.
> >>>>
> >>>>
> >>>> We can not do this.
> >>>>
> >>>> @len is used by QEMU emulation to count the size of the buffer
> >>>> that _DSM should return. It's only used in NVDIMM_COMMON_DSM
> >>>> method which is shared by the DSM method from VM and Read_Fit.
> >>> spec describes buffer layout independently from AML that uses it,
> >>> so it should describe whole data structure.
> >>>
> >>> Then it's upto guest how to read this data, it could be QEMU
> >>> generated AML (as it's here) or some other driver or even BIOS.
> >>
> >> However, what we are talking about is Read_FIT method, so this is
> >> the layout of Read_FIT output rather than all memory in the dsm
> >> page.
> >>
> >> Actually, in the spec we already have documented the common len
> >> field:
> >>
> >>     QEMU Writes Output Data (based on the offset in the page):
> >>     [0x0 - 0x3]: 4 bytes, the length of result
> >>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> >>
> >> Also, i really do not hope to use this field to count the buffer
> >> size returned by Read_FIT, we'd try the best to reuse existing DSM
> >> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
> >> method.
> > buffer layout describes interface between QEMU and firmware (AML)
> > and it should describe it completely every time to avoid confusion.
> >
> > See for example ACPI spec, NFIT table, SRAT table, ...
> > all table descriptions start with complete header.
> 
> Okay. Understood. :)
> 
> >
> > If you skip length it rises question how much fit data are there,
> > meaning interface description isn't complete.
> 
> So how about introduce a length field in the output buffer just
> as this patch did? I understand we are able to count the size
> from dsm len, however, it can simplify the code a lot...
it's redundant as there already is length for whole buffer,
interface should be kept as simple as possible and not include
redundant data for convenience.

> 
> >
> > if you want to describe AML there you can do it after interface
> > description saying that common NCAL method extracts status and fit
> > data form dsm page and returns that as buffer object, but it's AML
> > impl. specific. I could write an alternative AML code that would
> > deal with dms page in its own way as long as I would know what
> > write/read at what offset.
> 
> Yes, i agree with you.
> 

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 11/04/2016 01:29 AM, Igor Mammedov wrote:
> On Fri, 4 Nov 2016 00:53:06 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>>
>>
>> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
>>> On Fri, 4 Nov 2016 00:17:00 +0800
>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>
>>>>
>>>>
>>>> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
>>>>> On Thu, 3 Nov 2016 22:53:43 +0800
>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
>>>>>>> On Thu, 3 Nov 2016 21:02:22 +0800
>>>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> just drop this and describe properly 'len' in spec section
>>>>>>>>>>> i.e. len: length of entire returned data (including the
>>>>>>>>>>> header)
>>>>>>>>>>
>>>>>>>>>> Okay, i will change the spec like this:
>>>>>>>>>>
>>>>>>>>>>     QEMU Writes Output Data (based on the offset in the
>>>>>>>>>> page): [0x0 - 0x3]: 4 bytes, length of entire returned data
>>>>>>>>>> (including the header)
>>>>>>>>>>
>>>>>>>>>> And drop the length field in Read_Fit return buffer, doc
>>>>>>>>>> the fit buffer like this:
>>>>>>>>>>
>>>>>>>>>>     +----------+--------+--------+-------------------------------------------+
>>>>>>>>>>     |  Field   | Length | Offset |
>>>>>>>>>> Description               |
>>>>>>>>>> +----------+--------+--------+-------------------------------------------+
>>>>>>>>> you need to add length here, otherwise this table is not
>>>>>>>>> correct
>>>>>>>>
>>>>>>>> Ah, so i am confused.
>>>>>>>>
>>>>>>>> struct NvdimmFuncReadFITOut definition is based on the layout
>>>>>>>> of Read_FI output. You suggested to drop the length filed in
>>>>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
>>>>>>>> consistent.
>>>>>>>>
>>>>>>>> I missed something?
>>>>>>>
>>>>>>> +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;
>>>>>>>
>>>>>>> --------------------------------
>>>>>>> | field       | len | off | desc...
>>>>>>> --------------------------------
>>>>>>> | length      |  4  |  0  | ....
>>>>>>> --------------------------------
>>>>>>> | status      |  4  |  4  | ....
>>>>>>> --------------------------------
>>>>>>> | fit data    | ................
>>>>>>>
>>>>>>> i.e. you were forgetting to add length in spec so offsets were
>>>>>>> wrong even for described fields.
>>>>>>
>>>>>>
>>>>>> We can not do this.
>>>>>>
>>>>>> @len is used by QEMU emulation to count the size of the buffer
>>>>>> that _DSM should return. It's only used in NVDIMM_COMMON_DSM
>>>>>> method which is shared by the DSM method from VM and Read_Fit.
>>>>> spec describes buffer layout independently from AML that uses it,
>>>>> so it should describe whole data structure.
>>>>>
>>>>> Then it's upto guest how to read this data, it could be QEMU
>>>>> generated AML (as it's here) or some other driver or even BIOS.
>>>>
>>>> However, what we are talking about is Read_FIT method, so this is
>>>> the layout of Read_FIT output rather than all memory in the dsm
>>>> page.
>>>>
>>>> Actually, in the spec we already have documented the common len
>>>> field:
>>>>
>>>>     QEMU Writes Output Data (based on the offset in the page):
>>>>     [0x0 - 0x3]: 4 bytes, the length of result
>>>>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
>>>>
>>>> Also, i really do not hope to use this field to count the buffer
>>>> size returned by Read_FIT, we'd try the best to reuse existing DSM
>>>> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
>>>> method.
>>> buffer layout describes interface between QEMU and firmware (AML)
>>> and it should describe it completely every time to avoid confusion.
>>>
>>> See for example ACPI spec, NFIT table, SRAT table, ...
>>> all table descriptions start with complete header.
>>
>> Okay. Understood. :)
>>
>>>
>>> If you skip length it rises question how much fit data are there,
>>> meaning interface description isn't complete.
>>
>> So how about introduce a length field in the output buffer just
>> as this patch did? I understand we are able to count the size
>> from dsm len, however, it can simplify the code a lot...
> it's redundant as there already is length for whole buffer,
> interface should be kept as simple as possible and not include
> redundant data for convenience.

Okay.

So the doc should be changed to:

    Output layout in the dsm memory page:

    +----------+--------+--------+-------------------------------------------+
    |  Field   | Length | Offset |                 Description               |
    +----------+--------+--------+-------------------------------------------+
    | length   |   4    |   4    | length of entire returned data            |
    |          |        |        | (including the header)                    |
    +----------+-----------------+-------------------------------------------+
    |          |        |        | return status codes                       |
    |          |        |        | 0x100 - error caused by NFIT update while |
    | status   |   4    |   4    | read by _FIT wasn't completed, other      |
    |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
    +----------+--------+--------+-------------------------------------------+
    | fit data | Varies |   8    | FIT data, the remaining size is used by   |
    |          |        |        | fit data if status is success;            |
    |          |        |        | otherwise it is not valid                 |
    +----------+--------+--------+-------------------------------------------+

As you know, i am not good at doc, any suggestion is welcome. :)

Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Fri, 4 Nov 2016 01:39:31 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> 
> 
> On 11/04/2016 01:29 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:53:06 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> >>> On Fri, 4 Nov 2016 00:17:00 +0800
> >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> >>>>> On Thu, 3 Nov 2016 22:53:43 +0800
> >>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>>>>>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>>>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>>> just drop this and describe properly 'len' in spec section
> >>>>>>>>>>> i.e. len: length of entire returned data (including the
> >>>>>>>>>>> header)
> >>>>>>>>>>
> >>>>>>>>>> Okay, i will change the spec like this:
> >>>>>>>>>>
> >>>>>>>>>>     QEMU Writes Output Data (based on the offset in the
> >>>>>>>>>> page): [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>>>>>>>> (including the header)
> >>>>>>>>>>
> >>>>>>>>>> And drop the length field in Read_Fit return buffer, doc
> >>>>>>>>>> the fit buffer like this:
> >>>>>>>>>>
> >>>>>>>>>>     +----------+--------+--------+-------------------------------------------+
> >>>>>>>>>>     |  Field   | Length | Offset |
> >>>>>>>>>> Description               |
> >>>>>>>>>> +----------+--------+--------+-------------------------------------------+
> >>>>>>>>> you need to add length here, otherwise this table is not
> >>>>>>>>> correct
> >>>>>>>>
> >>>>>>>> Ah, so i am confused.
> >>>>>>>>
> >>>>>>>> struct NvdimmFuncReadFITOut definition is based on the layout
> >>>>>>>> of Read_FI output. You suggested to drop the length filed in
> >>>>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >>>>>>>> consistent.
> >>>>>>>>
> >>>>>>>> I missed something?
> >>>>>>>
> >>>>>>> +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;
> >>>>>>>
> >>>>>>> --------------------------------
> >>>>>>> | field       | len | off | desc...
> >>>>>>> --------------------------------
> >>>>>>> | length      |  4  |  0  | ....
> >>>>>>> --------------------------------
> >>>>>>> | status      |  4  |  4  | ....
> >>>>>>> --------------------------------
> >>>>>>> | fit data    | ................
> >>>>>>>
> >>>>>>> i.e. you were forgetting to add length in spec so offsets were
> >>>>>>> wrong even for described fields.
> >>>>>>
> >>>>>>
> >>>>>> We can not do this.
> >>>>>>
> >>>>>> @len is used by QEMU emulation to count the size of the buffer
> >>>>>> that _DSM should return. It's only used in NVDIMM_COMMON_DSM
> >>>>>> method which is shared by the DSM method from VM and Read_Fit.
> >>>>> spec describes buffer layout independently from AML that uses it,
> >>>>> so it should describe whole data structure.
> >>>>>
> >>>>> Then it's upto guest how to read this data, it could be QEMU
> >>>>> generated AML (as it's here) or some other driver or even BIOS.
> >>>>
> >>>> However, what we are talking about is Read_FIT method, so this is
> >>>> the layout of Read_FIT output rather than all memory in the dsm
> >>>> page.
> >>>>
> >>>> Actually, in the spec we already have documented the common len
> >>>> field:
> >>>>
> >>>>     QEMU Writes Output Data (based on the offset in the page):
> >>>>     [0x0 - 0x3]: 4 bytes, the length of result
> >>>>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> >>>>
> >>>> Also, i really do not hope to use this field to count the buffer
> >>>> size returned by Read_FIT, we'd try the best to reuse existing DSM
> >>>> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
> >>>> method.
> >>> buffer layout describes interface between QEMU and firmware (AML)
> >>> and it should describe it completely every time to avoid confusion.
> >>>
> >>> See for example ACPI spec, NFIT table, SRAT table, ...
> >>> all table descriptions start with complete header.
> >>
> >> Okay. Understood. :)
> >>
> >>>
> >>> If you skip length it rises question how much fit data are there,
> >>> meaning interface description isn't complete.
> >>
> >> So how about introduce a length field in the output buffer just
> >> as this patch did? I understand we are able to count the size
> >> from dsm len, however, it can simplify the code a lot...
> > it's redundant as there already is length for whole buffer,
> > interface should be kept as simple as possible and not include
> > redundant data for convenience.
> 
> Okay.
> 
> So the doc should be changed to:
> 
>     Output layout in the dsm memory page:
> 
>     +----------+--------+--------+-------------------------------------------+
>     |  Field   | Length | Offset |                 Description               |
>     +----------+--------+--------+-------------------------------------------+
>     | length   |   4    |   4    | length of entire returned data            |
>     |          |        |        | (including the header)                    |
wrong offset

>     +----------+-----------------+-------------------------------------------+
>     |          |        |        | return status codes                       |
>     |          |        |        | 0x100 - error caused by NFIT update while |
>     | status   |   4    |   4    | read by _FIT wasn't completed, other      |
>     |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
it wouldn't be bad to specify success status code here too.

>     +----------+--------+--------+-------------------------------------------+
>     | fit data | Varies |   8    | FIT data, the remaining size is used by   |
>     |          |        |        | fit data if status is success;            |
>     |          |        |        | otherwise it is not valid                 |
>     +----------+--------+--------+-------------------------------------------+
contains FIT data, this field is present if status field is [concrete number here]

> 
> As you know, i am not good at doc, any suggestion is welcome. :)
> 
> 

[Qemu-devel] [PATCH v4 3/3] pc: memhp: enable nvdimm device hotplug

[Xiao Guangrong]

_GPE.E04 is dedicated for nvdimm device hotplug

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 default-configs/mips-softmmu-common.mak |  1 +
 docs/specs/acpi_nvdimm.txt              |  5 +++++
 hw/acpi/ich9.c                          |  8 ++++++--
 hw/acpi/nvdimm.c                        |  7 +++++++
 hw/acpi/piix4.c                         |  7 ++++++-
 hw/i386/acpi-build.c                    |  7 +++++++
 hw/i386/pc.c                            | 12 ++++++++++++
 hw/mem/nvdimm.c                         |  4 ----
 include/hw/acpi/acpi_dev_interface.h    |  1 +
 include/hw/mem/nvdimm.h                 |  1 +
 10 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/default-configs/mips-softmmu-common.mak b/default-configs/mips-softmmu-common.mak
index 0394514..f0676f5 100644
--- a/default-configs/mips-softmmu-common.mak
+++ b/default-configs/mips-softmmu-common.mak
@@ -17,6 +17,7 @@ CONFIG_FDC=y
 CONFIG_ACPI=y
 CONFIG_ACPI_X86=y
 CONFIG_ACPI_MEMORY_HOTPLUG=y
+CONFIG_ACPI_NVDIMM=y
 CONFIG_ACPI_CPU_HOTPLUG=y
 CONFIG_APM=y
 CONFIG_I8257=y
diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 364e832..7887e57 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -137,6 +137,11 @@ _DSM process diagram:
  | result from the page     |      |              |
  +--------------------------+      +--------------+
 
+NVDIMM hotplug
+--------------
+ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
+hot-add and hot-remove events.
+
 QEMU internal use only _DSM function
 ------------------------------------
 There is the function introduced by QEMU and only used by QEMU internal.
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index e5a3c18..830c475 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -490,8 +490,12 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     if (lpc->pm.acpi_memory_hotplug.is_enabled &&
         object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
-                            dev, errp);
+        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+            nvdimm_acpi_plug_cb(hotplug_dev, dev);
+        } else {
+            acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
+                                dev, errp);
+        }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         if (lpc->pm.cpu_hotplug_legacy) {
             legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp);
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 593ac0d..b8548cc 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -874,6 +874,13 @@ static const MemoryRegionOps nvdimm_dsm_ops = {
     },
 };
 
+void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    if (dev->hotplugged) {
+        acpi_send_event(DEVICE(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS);
+    }
+}
+
 void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
                             FWCfgState *fw_cfg, Object *owner)
 {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2adc246..17d36bd 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -378,7 +378,12 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
 
     if (s->acpi_memory_hotplug.is_enabled &&
         object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, dev, errp);
+        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+            nvdimm_acpi_plug_cb(hotplug_dev, dev);
+        } else {
+            acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug,
+                                dev, errp);
+        }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
         acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index bc49958..32270c3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2039,6 +2039,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         method = aml_method("_E03", 0, AML_NOTSERIALIZED);
         aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
         aml_append(scope, method);
+
+        if (pcms->acpi_nvdimm_state.is_enabled) {
+            method = aml_method("_E04", 0, AML_NOTSERIALIZED);
+            aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
+                                          aml_int(0x80)));
+            aml_append(scope, method);
+        }
     }
     aml_append(dsdt, scope);
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 77ca7f4..0403452 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1723,6 +1723,12 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
         goto out;
     }
 
+    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+        error_setg(&local_err,
+                   "nvdimm device hot unplug is not supported yet.");
+        goto out;
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 
@@ -1740,6 +1746,12 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
 
+    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+        error_setg(&local_err,
+                   "nvdimm device hot unplug is not supported yet.");
+        goto out;
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7895805..db896b0 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -148,13 +148,9 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
 
 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/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index da4ef7f..901a4ae 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -10,6 +10,7 @@ typedef enum {
     ACPI_PCI_HOTPLUG_STATUS = 2,
     ACPI_CPU_HOTPLUG_STATUS = 4,
     ACPI_MEMORY_HOTPLUG_STATUS = 8,
+    ACPI_NVDIMM_HOTPLUG_STATUS = 16,
 } AcpiEventStatusBits;
 
 #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 232437c..4b90584 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -133,4 +133,5 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                        BIOSLinker *linker, AcpiNVDIMMState *state,
                        uint32_t ram_slots);
 void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
+void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 3/3] pc: memhp: enable nvdimm device hotplug

[Igor Mammedov]

On Thu,  3 Nov 2016 11:51:30 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> _GPE.E04 is dedicated for nvdimm device hotplug
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  default-configs/mips-softmmu-common.mak |  1 +
>  docs/specs/acpi_nvdimm.txt              |  5 +++++
>  hw/acpi/ich9.c                          |  8 ++++++--
>  hw/acpi/nvdimm.c                        |  7 +++++++
>  hw/acpi/piix4.c                         |  7 ++++++-
>  hw/i386/acpi-build.c                    |  7 +++++++
>  hw/i386/pc.c                            | 12 ++++++++++++
>  hw/mem/nvdimm.c                         |  4 ----
>  include/hw/acpi/acpi_dev_interface.h    |  1 +
>  include/hw/mem/nvdimm.h                 |  1 +
>  10 files changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/default-configs/mips-softmmu-common.mak b/default-configs/mips-softmmu-common.mak
> index 0394514..f0676f5 100644
> --- a/default-configs/mips-softmmu-common.mak
> +++ b/default-configs/mips-softmmu-common.mak
> @@ -17,6 +17,7 @@ CONFIG_FDC=y
>  CONFIG_ACPI=y
>  CONFIG_ACPI_X86=y
>  CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_NVDIMM=y
>  CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 364e832..7887e57 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -137,6 +137,11 @@ _DSM process diagram:
>   | result from the page     |      |              |
>   +--------------------------+      +--------------+
>  
> +NVDIMM hotplug
> +--------------
> +ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
> +hot-add and hot-remove events.
s/and hot-remove //


>  QEMU internal use only _DSM function
>  ------------------------------------
>  There is the function introduced by QEMU and only used by QEMU internal.
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e5a3c18..830c475 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -490,8 +490,12 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      if (lpc->pm.acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
> -                            dev, errp);
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
> +        } else {
> +            acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
> +                                dev, errp);
> +        }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          if (lpc->pm.cpu_hotplug_legacy) {
>              legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp);
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 593ac0d..b8548cc 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -874,6 +874,13 @@ static const MemoryRegionOps nvdimm_dsm_ops = {
>      },
>  };
>  
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> +    if (dev->hotplugged) {
> +        acpi_send_event(DEVICE(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS);
> +    }
> +}
> +
>  void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>                              FWCfgState *fw_cfg, Object *owner)
>  {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 2adc246..17d36bd 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -378,7 +378,12 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>  
>      if (s->acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, dev, errp);
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
> +        } else {
> +            acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug,
> +                                dev, errp);
> +        }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>          acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index bc49958..32270c3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2039,6 +2039,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          method = aml_method("_E03", 0, AML_NOTSERIALIZED);
>          aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
>          aml_append(scope, method);
> +
> +        if (pcms->acpi_nvdimm_state.is_enabled) {
> +            method = aml_method("_E04", 0, AML_NOTSERIALIZED);
> +            aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
> +                                          aml_int(0x80)));
> +            aml_append(scope, method);
> +        }
>      }
>      aml_append(dsdt, scope);
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 77ca7f4..0403452 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1723,6 +1723,12 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +        error_setg(&local_err,
> +                   "nvdimm device hot unplug is not supported yet.");
> +        goto out;
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>  
> @@ -1740,6 +1746,12 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>  
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +        error_setg(&local_err,
> +                   "nvdimm device hot unplug is not supported yet.");
> +        goto out;
> +    }
drop this as it's not reachable without unplug support for
NVDIMM in piix4/ich9, call chain for it would be:

  guest(MMIO on unplug)
      -> piix4/ich9 IO port handler
          ->hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
              -> pc_machine_device_unplug_cb
                  -> pc_dimm_unplug

>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>  
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7895805..db896b0 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -148,13 +148,9 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>  
>  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/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
> index da4ef7f..901a4ae 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -10,6 +10,7 @@ typedef enum {
>      ACPI_PCI_HOTPLUG_STATUS = 2,
>      ACPI_CPU_HOTPLUG_STATUS = 4,
>      ACPI_MEMORY_HOTPLUG_STATUS = 8,
> +    ACPI_NVDIMM_HOTPLUG_STATUS = 16,
>  } AcpiEventStatusBits;
>  
>  #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 232437c..4b90584 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -133,4 +133,5 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         BIOSLinker *linker, AcpiNVDIMMState *state,
>                         uint32_t ram_slots);
>  void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
>  #endif

Re: [Qemu-devel] [PATCH v4 3/3] pc: memhp: enable nvdimm device hotplug

[Xiao Guangrong]

On 11/03/2016 08:51 PM, Igor Mammedov wrote:
> On Thu,  3 Nov 2016 11:51:30 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _GPE.E04 is dedicated for nvdimm device hotplug
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>  default-configs/mips-softmmu-common.mak |  1 +
>>  docs/specs/acpi_nvdimm.txt              |  5 +++++
>>  hw/acpi/ich9.c                          |  8 ++++++--
>>  hw/acpi/nvdimm.c                        |  7 +++++++
>>  hw/acpi/piix4.c                         |  7 ++++++-
>>  hw/i386/acpi-build.c                    |  7 +++++++
>>  hw/i386/pc.c                            | 12 ++++++++++++
>>  hw/mem/nvdimm.c                         |  4 ----
>>  include/hw/acpi/acpi_dev_interface.h    |  1 +
>>  include/hw/mem/nvdimm.h                 |  1 +
>>  10 files changed, 46 insertions(+), 7 deletions(-)
>>
>> diff --git a/default-configs/mips-softmmu-common.mak b/default-configs/mips-softmmu-common.mak
>> index 0394514..f0676f5 100644
>> --- a/default-configs/mips-softmmu-common.mak
>> +++ b/default-configs/mips-softmmu-common.mak
>> @@ -17,6 +17,7 @@ CONFIG_FDC=y
>>  CONFIG_ACPI=y
>>  CONFIG_ACPI_X86=y
>>  CONFIG_ACPI_MEMORY_HOTPLUG=y
>> +CONFIG_ACPI_NVDIMM=y
>>  CONFIG_ACPI_CPU_HOTPLUG=y
>>  CONFIG_APM=y
>>  CONFIG_I8257=y
>> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
>> index 364e832..7887e57 100644
>> --- a/docs/specs/acpi_nvdimm.txt
>> +++ b/docs/specs/acpi_nvdimm.txt
>> @@ -137,6 +137,11 @@ _DSM process diagram:
>>   | result from the page     |      |              |
>>   +--------------------------+      +--------------+
>>
>> +NVDIMM hotplug
>> +--------------
>> +ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
>> +hot-add and hot-remove events.
> s/and hot-remove //

Okay.

>
>
>>  QEMU internal use only _DSM function
>>  ------------------------------------
>>  There is the function introduced by QEMU and only used by QEMU internal.
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index e5a3c18..830c475 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -490,8 +490,12 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>
>>      if (lpc->pm.acpi_memory_hotplug.is_enabled &&
>>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -        acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
>> -                            dev, errp);
>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
>> +        } else {
>> +            acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
>> +                                dev, errp);
>> +        }
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>          if (lpc->pm.cpu_hotplug_legacy) {
>>              legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp);
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 593ac0d..b8548cc 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -874,6 +874,13 @@ static const MemoryRegionOps nvdimm_dsm_ops = {
>>      },
>>  };
>>
>> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
>> +{
>> +    if (dev->hotplugged) {
>> +        acpi_send_event(DEVICE(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS);
>> +    }
>> +}
>> +
>>  void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>>                              FWCfgState *fw_cfg, Object *owner)
>>  {
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 2adc246..17d36bd 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -378,7 +378,12 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>>
>>      if (s->acpi_memory_hotplug.is_enabled &&
>>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -        acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, dev, errp);
>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
>> +        } else {
>> +            acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug,
>> +                                dev, errp);
>> +        }
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>          acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index bc49958..32270c3 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2039,6 +2039,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>          method = aml_method("_E03", 0, AML_NOTSERIALIZED);
>>          aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
>>          aml_append(scope, method);
>> +
>> +        if (pcms->acpi_nvdimm_state.is_enabled) {
>> +            method = aml_method("_E04", 0, AML_NOTSERIALIZED);
>> +            aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
>> +                                          aml_int(0x80)));
>> +            aml_append(scope, method);
>> +        }
>>      }
>>      aml_append(dsdt, scope);
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 77ca7f4..0403452 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1723,6 +1723,12 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
>>          goto out;
>>      }
>>
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>> +        error_setg(&local_err,
>> +                   "nvdimm device hot unplug is not supported yet.");
>> +        goto out;
>> +    }
>> +
>>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>>      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>>
>> @@ -1740,6 +1746,12 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
>>      HotplugHandlerClass *hhc;
>>      Error *local_err = NULL;
>>
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>> +        error_setg(&local_err,
>> +                   "nvdimm device hot unplug is not supported yet.");
>> +        goto out;
>> +    }
> drop this as it's not reachable without unplug support for
> NVDIMM in piix4/ich9, call chain for it would be:

Thanks for you pointing it out, i will drop the change in pc_dimm_unplug().

Re: [Qemu-devel] [PATCH v4 0/3] nvdimm: hotplug support

[Michael S. Tsirkin]

On Thu, Nov 03, 2016 at 11:51:27AM +0800, Xiao Guangrong wrote:
> Resend these 3 patches to catch up release window...
> 
> Igor,
> 
> this is a open that i did not pass a buffer as parameter to RFIT as
> tried the way you suggested, but failed. May be i am not very good at
> ASL, i need more time to try. So let's keep the way as it is, i will
> improve it later.
> 
> Thanks!

And just for comparison, could you pls generate the
incremental diff as well?


> Changelog in v4:
>    1) drop fit lock and post_hotplug_cb
>    2) move nvdimm hotplug code to hw/acpi/nvdimm.c
>    3) introduce length field to indicate the fit size
>    4) nvdimm acpi cleanup
>    5) doc typo fixes
> 
> Changelog in v3:
>    1) use a dedicated interrupt for nvdimm device hotplug
>    2) stop nvdimm device hot unplug
>    3) reserve UUID and handle for QEMU internally used QEMU
>    5) redesign fit buffer to avoid OSPM reading incomplete fit info
>    6) bug fixes and cleanups
> 
> 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
> 
> Xiao Guangrong (3):
>   nvdimm acpi: introduce fit buffer
>   nvdimm acpi: introduce _FIT
>   pc: memhp: enable nvdimm device hotplug
> 
>  default-configs/mips-softmmu-common.mak |   1 +
>  docs/specs/acpi_nvdimm.txt              |  68 +++++++-
>  hw/acpi/ich9.c                          |   8 +-
>  hw/acpi/nvdimm.c                        | 289 ++++++++++++++++++++++++++++----
>  hw/acpi/piix4.c                         |   7 +-
>  hw/i386/acpi-build.c                    |   9 +-
>  hw/i386/pc.c                            |  16 ++
>  hw/mem/nvdimm.c                         |   4 -
>  include/hw/acpi/acpi_dev_interface.h    |   1 +
>  include/hw/mem/nvdimm.h                 |  22 ++-
>  10 files changed, 377 insertions(+), 48 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v4 0/3] nvdimm: hotplug support

[Xiao Guangrong]

On 11/03/2016 12:14 PM, Michael S. Tsirkin wrote:
> On Thu, Nov 03, 2016 at 11:51:27AM +0800, Xiao Guangrong wrote:
>> Resend these 3 patches to catch up release window...
>>
>> Igor,
>>
>> this is a open that i did not pass a buffer as parameter to RFIT as
>> tried the way you suggested, but failed. May be i am not very good at
>> ASL, i need more time to try. So let's keep the way as it is, i will
>> improve it later.
>>
>> Thanks!
>
> And just for comparison, could you pls generate the
> incremental diff as well?

Hi Michael,

Okay. This is the diff comparing with v3 and v4:

diff --git a/default-configs/mips-softmmu-common.mak b/default-configs/mips-softmmu-common.mak
index 0394514..f0676f5 100644
--- a/default-configs/mips-softmmu-common.mak
+++ b/default-configs/mips-softmmu-common.mak
@@ -17,6 +17,7 @@ CONFIG_FDC=y
  CONFIG_ACPI=y
  CONFIG_ACPI_X86=y
  CONFIG_ACPI_MEMORY_HOTPLUG=y
+CONFIG_ACPI_NVDIMM=y
  CONFIG_ACPI_CPU_HOTPLUG=y
  CONFIG_APM=y
  CONFIG_I8257=y
diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index cb26dd2..3df3620 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -4,9 +4,6 @@ QEMU<->ACPI BIOS memory hotplug interface
  ACPI BIOS GPE.3 handler is dedicated for notifying OS about memory hot-add
  and hot-remove events.

-ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
-hot-add and hot-remove events.
-
  Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
  ---------------------------------------------------------------
  0xa00:
diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 4aa5e3d..7887e57 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
     NVDIMM Firmware Interface Table (NFIT).

-QEMU NVDIMM Implemention
-========================
+QEMU NVDIMM Implementation
+==========================
  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
  for NVDIMM ACPI.

@@ -82,6 +82,16 @@ Memory:
     ACPI writes _DSM Input Data (based on the offset in the page):
     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
                  Root device.
+
+                The handle is completely QEMU internal thing, the values in
+                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
+                root device named NVDR), other values are reserved by other
+                purpose.
+
+                Current reserved handle:
+                0x10000 is reserved for QEMU internal DSM function called on
+                the root device.
+
     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
@@ -127,28 +137,22 @@ _DSM process diagram:
   | result from the page     |      |              |
   +--------------------------+      +--------------+

-Device Handle Reservation
--------------------------
-As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device
-handle. The handle is completely QEMU internal thing, the values in range
-[0, 0xFFFF] indicate nvdimm device (O means nvdimm root device named NVDR),
-other values are reserved by other purpose.
-
-Current reserved handle:
-0x10000 is reserved for QEMU internal DSM function called on the root
-device.
+NVDIMM hotplug
+--------------
+ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
+hot-add and hot-remove events.

  QEMU internal use only _DSM function
  ------------------------------------
-UUID, 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, is reserved for QEMU internal
-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 used by _FIT
-   method to read a piece of FIT data from QEMU.
+   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
+   function (private QEMU function)
+
+   _FIT method uses Read_FIT function to fetch NFIT structures blob from
+   QEMU in 1 page sized increments which are then concatenated and returned
+   as _FIT method result.

     Input parameters:
     Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
@@ -156,27 +160,29 @@ There is the function introduced by QEMU and only used by QEMU internal.
     Arg2 - Function Index, 0x1
     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          |
-   +----------+-------------+-------------+-----------------------------------+
+   +----------+--------+--------+-------------------------------------------+
+   |  Field   | Length | Offset |                 Description               |
+   +----------+--------+--------+-------------------------------------------+
+   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
+   |          |        |        | read from                                 |
+   +----------+--------+--------+-------------------------------------------+

     Output:
-   +----------+-------------+-------------+-----------------------------------+
-   |  Filed   | Byte Length | Byte Offset | Description                       |
-   +----------+-------------+-------------+-----------------------------------+
-   |          |             |             | return status codes               |
-   |          |             |             |   0x100 indicates fit has been    |
-   | status   |     4       |    0        |   updated                         |
-   |          |             |             | other follows Chapter 3 in DSM    |
-   |          |             |             | Spec Rev1                         |
-   +----------+-------------+-------------+-----------------------------------+
-   | fit data |  Varies     |    4        | FIT data                          |
-   |          |             |             |                                   |
-   +----------+-------------+-------------+-----------------------------------+
-
-   The FIT offset is maintained by the caller itself, current offset plugs
+   +----------+--------+--------+-------------------------------------------+
+   |  Field   | Length | Offset |                 Description               |
+   +----------+--------+--------+-------------------------------------------+
+   |          |        |        | return status codes                       |
+   |          |        |        | 0x100 - error caused by NFIT update while |
+   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
+   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
+   +----------+--------+--------+-------------------------------------------+
+   | length   |   4    |   4    | The fit size                              |
+   +----------+-----------------+-------------------------------------------+
+   | fit data | Varies |   8    | FIT data, its size is indicated by length |
+   |          |        |        | filed above                               |
+   +----------+--------+--------+-------------------------------------------+
+
+   The FIT offset is maintained by the OSPM itself, current offset plus
     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.

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index e5a3c18..830c475 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -490,8 +490,12 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,

      if (lpc->pm.acpi_memory_hotplug.is_enabled &&
          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
-                            dev, errp);
+        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+            nvdimm_acpi_plug_cb(hotplug_dev, dev);
+        } else {
+            acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
+                                dev, errp);
+        }
      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
          if (lpc->pm.cpu_hotplug_legacy) {
              legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp);
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 70f6451..ec4e64b 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -2,7 +2,6 @@
  #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"
@@ -233,8 +232,11 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
                           DeviceState *dev, Error **errp)
  {
      MemStatus *mdev;
-    AcpiEventStatusBits event;
-    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+
+    if (!dc->hotpluggable) {
+        return;
+    }

      mdev = acpi_memory_slot_status(mem_st, dev, errp);
      if (!mdev) {
@@ -242,23 +244,10 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
      }

      mdev->dimm = dev;
-
-    /*
-     * do not set is_enabled and is_inserting if the slot is plugged with
-     * a nvdimm device to stop OSPM inquires memory region from the slot.
-     */
-    if (is_nvdimm) {
-        event = ACPI_NVDIMM_HOTPLUG_STATUS;
-    } else {
-        mdev->is_enabled = true;
-        event = ACPI_MEMORY_HOTPLUG_STATUS;
-    }
-
+    mdev->is_enabled = true;
      if (dev->hotplugged) {
-        if (!is_nvdimm) {
-            mdev->is_inserting = true;
-        }
-        acpi_send_event(DEVICE(hotplug_dev), event);
+        mdev->is_inserting = true;
+        acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
      }
  }

@@ -273,8 +262,6 @@ void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev,
          return;
      }

-    /* nvdimm device hot unplug is not supported yet. */
-    assert(!object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM));
      mdev->is_removing = true;
      acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
  }
@@ -289,8 +276,6 @@ void acpi_memory_unplug_cb(MemHotplugState *mem_st,
          return;
      }

-    /* nvdimm device hot unplug is not supported yet. */
-    assert(!object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM));
      mdev->is_enabled = false;
      mdev->dimm = NULL;
  }
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index fc1a012..b8548cc 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -38,11 +38,7 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque)
      GSList **list = opaque;

      if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
-        DeviceState *dev = DEVICE(obj);
-
-        if (dev->realized) { /* only realized NVDIMMs matter */
-            *list = g_slist_append(*list, DEVICE(obj));
-        }
+        *list = g_slist_append(*list, DEVICE(obj));
      }

      object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
@@ -375,17 +371,14 @@ static GArray *nvdimm_build_device_structure(void)

  static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
  {
-    qemu_mutex_init(&fit_buf->lock);
      fit_buf->fit = g_array_new(false, true /* clear */, 1);
  }

  static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
  {
-    qemu_mutex_lock(&fit_buf->lock);
      g_array_free(fit_buf->fit, true);
      fit_buf->fit = nvdimm_build_device_structure();
      fit_buf->dirty = true;
-    qemu_mutex_unlock(&fit_buf->lock);
  }

  void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
@@ -399,11 +392,9 @@ static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
      NvdimmFitBuffer *fit_buf = &state->fit_buf;
      unsigned int header;

-    qemu_mutex_lock(&fit_buf->lock);
-
      /* NVDIMM device is not plugged? */
      if (!fit_buf->fit->len) {
-        goto exit;
+        return;
      }

      acpi_add_table(table_offsets, table_data);
@@ -417,9 +408,6 @@ static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
      build_header(linker, table_data,
                   (void *)(table_data->data + header), "NFIT",
                   sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
-
-exit:
-    qemu_mutex_unlock(&fit_buf->lock);
  }

  struct NvdimmDsmIn {
@@ -497,7 +485,7 @@ QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
                    offsetof(NvdimmDsmIn, arg3) > 4096);

  struct NvdimmFuncReadFITIn {
-    uint32_t offset; /* the offset of FIT buffer. */
+    uint32_t offset; /* the offset into FIT buffer. */
  } QEMU_PACKED;
  typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
@@ -507,6 +495,7 @@ struct NvdimmFuncReadFITOut {
      /* the size of buffer filled by QEMU. */
      uint32_t len;
      uint32_t func_ret_status; /* return status code. */
+    uint32_t length; /* the length of fit. */
      uint8_t fit[0]; /* the FIT data. */
  } QEMU_PACKED;
  typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
@@ -532,7 +521,12 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
  }

-#define NVDIMM_QEMU_RSVD_HANDLE_ROOT 0x10000
+#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
+#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
+#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */
+#define NVDIMM_DSM_RET_STATUS_FIT_CHANGED    0x100 /* FIT Changed */
+
+#define NVDIMM_QEMU_RSVD_HANDLE_ROOT         0x10000

  /* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
  static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
@@ -542,34 +536,32 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
      NvdimmFuncReadFITIn *read_fit;
      NvdimmFuncReadFITOut *read_fit_out;
      GArray *fit;
-    uint32_t read_len = 0, func_ret_status;
+    uint32_t read_len = 0, func_ret_status, offset;
      int size;

      read_fit = (NvdimmFuncReadFITIn *)in->arg3;
-    le32_to_cpus(&read_fit->offset);
+    offset = le32_to_cpu(read_fit->offset);

-    qemu_mutex_lock(&fit_buf->lock);
      fit = fit_buf->fit;

      nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
-                 read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
-
-    if (read_fit->offset > fit->len) {
-        func_ret_status = 3 /* Invalid Input Parameters */;
-        goto exit;
-    }
+                 offset, fit->len, fit_buf->dirty ? "Yes" : "No");

      /* It is the first time to read FIT. */
-    if (!read_fit->offset) {
+    if (!offset) {
          fit_buf->dirty = false;
      } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */
-        func_ret_status = 0x100 /* fit changed */;
+        func_ret_status = NVDIMM_DSM_RET_STATUS_FIT_CHANGED;
          goto exit;
      }

-    func_ret_status = 0 /* Success */;
-    read_len = MIN(fit->len - read_fit->offset,
-                   4096 - sizeof(NvdimmFuncReadFITOut));
+    if (offset > fit->len) {
+        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
+        goto exit;
+    }
+
+    func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
+    read_len = MIN(fit->len - offset, 4096 - sizeof(NvdimmFuncReadFITOut));

  exit:
      size = sizeof(NvdimmFuncReadFITOut) + read_len;
@@ -577,12 +569,12 @@ exit:

      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);
+    read_fit_out->length = cpu_to_le32(read_len);
+    memcpy(read_fit_out->fit, fit->data + offset, read_len);

      cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);

      g_free(read_fit_out);
-    qemu_mutex_unlock(&fit_buf->lock);
  }

  static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
@@ -592,12 +584,12 @@ static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
      case 0x0:
          nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
          return;
-    case 0x1 /*Read FIT */:
+    case 0x1 /* Read FIT */:
          nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
          return;
      }

-    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
  }

  static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
@@ -659,7 +651,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)

      nvdimm_debug("label_size %#x, max_xfer %#x.\n", label_size, mxfer);

-    label_size_out.func_ret_status = cpu_to_le32(0 /* Success */);
+    label_size_out.func_ret_status = cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
      label_size_out.label_size = cpu_to_le32(label_size);
      label_size_out.max_xfer = cpu_to_le32(mxfer);

@@ -670,7 +662,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
  static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
                                             uint32_t offset, uint32_t length)
  {
-    uint32_t ret = 3 /* Invalid Input Parameters */;
+    uint32_t ret = NVDIMM_DSM_RET_STATUS_INVALID;

      if (offset + length < offset) {
          nvdimm_debug("offset %#x + length %#x is overflow.\n", offset,
@@ -690,7 +682,7 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
          return ret;
      }

-    return 0 /* Success */;
+    return NVDIMM_DSM_RET_STATUS_SUCCESS;
  }

  /*
@@ -714,7 +706,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,

      status = nvdimm_rw_label_data_check(nvdimm, get_label_data->offset,
                                          get_label_data->length);
-    if (status != 0 /* Success */) {
+    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
          nvdimm_dsm_no_payload(status, dsm_mem_addr);
          return;
      }
@@ -724,7 +716,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
      get_label_data_out = g_malloc(size);

      get_label_data_out->len = cpu_to_le32(size);
-    get_label_data_out->func_ret_status = cpu_to_le32(0 /* Success */);
+    get_label_data_out->func_ret_status =
+                                cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
      nvc->read_label_data(nvdimm, get_label_data_out->out_buf,
                           get_label_data->length, get_label_data->offset);

@@ -752,7 +745,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,

      status = nvdimm_rw_label_data_check(nvdimm, set_label_data->offset,
                                          set_label_data->length);
-    if (status != 0 /* Success */) {
+    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
          nvdimm_dsm_no_payload(status, dsm_mem_addr);
          return;
      }
@@ -762,7 +755,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,

      nvc->write_label_data(nvdimm, set_label_data->in_buf,
                            set_label_data->length, set_label_data->offset);
-    nvdimm_dsm_no_payload(0 /* Success */, dsm_mem_addr);
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS, dsm_mem_addr);
  }

  static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
@@ -813,7 +806,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
          break;
      }

-    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
  }

  static uint64_t
@@ -850,7 +843,7 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
      if (in->revision != 0x1 /* Currently we only support DSM Spec Rev1. */) {
          nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
                       in->revision, 0x1);
-        nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+        nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
          goto exit;
      }

@@ -881,6 +874,13 @@ static const MemoryRegionOps nvdimm_dsm_ops = {
      },
  };

+void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    if (dev->hotplugged) {
+        acpi_send_event(DEVICE(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS);
+    }
+}
+
  void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
                              FWCfgState *fw_cfg, Object *owner)
  {
@@ -1031,7 +1031,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
      aml_append(unsupport, ifctx);

      /* No function is supported yet. */
-    byte_list[0] = 1 /* Not Supported */;
+    byte_list[0] = NVDIMM_DSM_RET_STATUS_UNSUPPORT;
      aml_append(unsupport, aml_return(aml_buffer(1, byte_list)));
      aml_append(method, unsupport);

@@ -1094,7 +1094,7 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
      aml_append(dev, method);
  }

-static void nvdimm_build_fit(Aml *dev)
+static void nvdimm_build_fit_method(Aml *dev)
  {
      Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
      Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
@@ -1103,13 +1103,11 @@ static void nvdimm_build_fit(Aml *dev)
      buf_size = aml_local(1);
      fit = aml_local(2);

-    aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL),
-               aml_int(0), NVDIMM_DSM_RFIT_STATUS));
+    aml_append(dev, aml_name_decl(NVDIMM_DSM_RFIT_STATUS, aml_int(0)));

      /* build helper function, RFIT. */
      method = aml_method("RFIT", 1, AML_SERIALIZED);
-    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
-                                              aml_int(0), "OFST"));
+    aml_append(method, aml_name_decl("OFST", aml_int(0)));

      /* prepare input package. */
      pkg = aml_package(1);
@@ -1137,11 +1135,9 @@ static void nvdimm_build_fit(Aml *dev)
      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));
-
+    aml_append(method, aml_create_dword_field(buf,
+               aml_int(4) /* offset at byte 4 */, "LENG"));
+    aml_append(method, aml_store(aml_name("LENG"), 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)));
@@ -1150,7 +1146,7 @@ static void nvdimm_build_fit(Aml *dev)
      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.*/
+                            aml_int(8 * BITS_PER_BYTE), /* offset at byte 4.*/
                              buf_size, "BUFF"));
      aml_append(method, aml_return(aml_name("BUFF")));
      aml_append(dev, method);
@@ -1171,7 +1167,7 @@ static void nvdimm_build_fit(Aml *dev)
       * again.
       */
      ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
-                             aml_int(0x100 /* fit changed */)));
+                             aml_int(NVDIMM_DSM_RET_STATUS_FIT_CHANGED)));
      aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
      aml_append(ifctx, aml_store(aml_int(0), offset));
      aml_append(whilectx, ifctx);
@@ -1251,7 +1247,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_fit_method(dev);

      nvdimm_build_nvdimm_devices(dev, ram_slots);

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2adc246..17d36bd 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -378,7 +378,12 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,

      if (s->acpi_memory_hotplug.is_enabled &&
          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, dev, errp);
+        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+            nvdimm_acpi_plug_cb(hotplug_dev, dev);
+        } else {
+            acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug,
+                                dev, errp);
+        }
      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
          acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index ab34c19..17ac986 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -35,17 +35,6 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
      }
  }

-void hotplug_handler_post_plug(HotplugHandler *plug_handler,
-                               DeviceState *plugged_dev,
-                               Error **errp)
-{
-    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
-
-    if (hdc->post_plug) {
-        hdc->post_plug(plug_handler, plugged_dev, errp);
-    }
-}
-
  void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
                                      DeviceState *plugged_dev,
                                      Error **errp)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d835e62..5783442 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -945,21 +945,10 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
                  goto child_realize_fail;
              }
          }
-
          if (dev->hotplugged) {
              device_reset(dev);
          }
          dev->pending_deleted_event = false;
-        dev->realized = value;
-
-        if (hotplug_ctrl) {
-            hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err);
-        }
-
-        if (local_err != NULL) {
-            dev->realized = value;
-            goto post_realize_fail;
-        }
      } else if (!value && dev->realized) {
          Error **local_errp = NULL;
          QLIST_FOREACH(bus, &dev->child_bus, sibling) {
@@ -976,14 +965,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
          }
          dev->pending_deleted_event = true;
          DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
+    }

-        if (local_err != NULL) {
-            goto fail;
-        }
-
-        dev->realized = value;
+    if (local_err != NULL) {
+        goto fail;
      }

+    dev->realized = value;
      return;

  child_realize_fail:
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 200963f..0403452 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1700,22 +1700,16 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
          goto out;
      }

+    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
+    }
+
      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
  out:
      error_propagate(errp, local_err);
  }

-static void pc_dimm_post_plug(HotplugHandler *hotplug_dev,
-                              DeviceState *dev, Error **errp)
-{
-    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-
-    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
-        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
-    }
-}
-
  static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
                                     DeviceState *dev, Error **errp)
  {
@@ -1988,14 +1982,6 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
      }
  }

-static void pc_machine_device_post_plug_cb(HotplugHandler *hotplug_dev,
-                                           DeviceState *dev, Error **errp)
-{
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_post_plug(hotplug_dev, dev, errp);
-    }
-}
-
  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                  DeviceState *dev, Error **errp)
  {
@@ -2330,7 +2316,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
      mc->reset = pc_machine_reset;
      hc->pre_plug = pc_machine_device_pre_plug_cb;
      hc->plug = pc_machine_device_plug_cb;
-    hc->post_plug = pc_machine_device_post_plug_cb;
      hc->unplug_request = pc_machine_device_unplug_request_cb;
      hc->unplug = pc_machine_device_unplug_cb;
      nc->nmi_monitor_handler = x86_nmi;
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index 10ca5b6..c0db869 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -47,7 +47,6 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
   * @parent: Opaque parent interface.
   * @pre_plug: pre plug callback called at start of device.realize(true)
   * @plug: plug callback called at end of device.realize(true).
- * @post_pug: post plug callback called after device is successfully plugged.
   * @unplug_request: unplug request callback.
   *                  Used as a means to initiate device unplug for devices that
   *                  require asynchronous unplug handling.
@@ -62,7 +61,6 @@ typedef struct HotplugHandlerClass {
      /* <public> */
      hotplug_fn pre_plug;
      hotplug_fn plug;
-    hotplug_fn post_plug;
      hotplug_fn unplug_request;
      hotplug_fn unplug;
  } HotplugHandlerClass;
@@ -85,14 +83,6 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
                                DeviceState *plugged_dev,
                                Error **errp);

-/**
- * hotplug_handler_post_plug:
- *
- * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
- */
-void hotplug_handler_post_plug(HotplugHandler *plug_handler,
-                               DeviceState *plugged_dev,
-                               Error **errp);

  /**
   * hotplug_handler_unplug_request:
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 33cd421..4b90584 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -103,16 +103,11 @@ typedef struct NVDIMMClass NVDIMMClass;
   * devices which is updated after the NVDIMM device is plugged or
   * unplugged.
   *
- * Rules to use the buffer:
- *    1) the user should hold the @lock to access the buffer.
- *    2) mark @dirty whenever the buffer is updated.
- *
- * These rules preserve NVDIMM ACPI _FIT method to read incomplete
- * or obsolete fit info if fit update happens during multiple RFIT
- * calls.
+ * Mark @dirty whenever the buffer is updated so that it preserves NVDIMM
+ * ACPI _FIT method to read incomplete or obsolete fit info if fit update
+ * happens during multiple RFIT calls.
   */
  struct NvdimmFitBuffer {
-    QemuMutex lock;
      GArray *fit;
      bool dirty;
  };
@@ -138,4 +133,5 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                         BIOSLinker *linker, AcpiNVDIMMState *state,
                         uint32_t ram_slots);
  void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
+void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
  #endif

Re: [Qemu-devel] [PATCH v4 0/3] nvdimm: hotplug support

[Michael S. Tsirkin]

On Thu, Nov 03, 2016 at 12:25:01PM +0800, Xiao Guangrong wrote:
> 
> 
> On 11/03/2016 12:14 PM, Michael S. Tsirkin wrote:
> > On Thu, Nov 03, 2016 at 11:51:27AM +0800, Xiao Guangrong wrote:
> > > Resend these 3 patches to catch up release window...
> > > 
> > > Igor,
> > > 
> > > this is a open that i did not pass a buffer as parameter to RFIT as
> > > tried the way you suggested, but failed. May be i am not very good at
> > > ASL, i need more time to try. So let's keep the way as it is, i will
> > > improve it later.
> > > 
> > > Thanks!
> > 
> > And just for comparison, could you pls generate the
> > incremental diff as well?
> 
> Hi Michael,
> 
> Okay. This is the diff comparing with v3 and v4:

So I don't see how all this requires revert and reapply
and IMHO review of incremental patches would be easier
but if it makes Igor happier, so be it.


I'll wait until we have acks though.


> diff --git a/default-configs/mips-softmmu-common.mak b/default-configs/mips-softmmu-common.mak
> index 0394514..f0676f5 100644
> --- a/default-configs/mips-softmmu-common.mak
> +++ b/default-configs/mips-softmmu-common.mak
> @@ -17,6 +17,7 @@ CONFIG_FDC=y
>  CONFIG_ACPI=y
>  CONFIG_ACPI_X86=y
>  CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_NVDIMM=y
>  CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index cb26dd2..3df3620 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -4,9 +4,6 @@ QEMU<->ACPI BIOS memory hotplug interface
>  ACPI BIOS GPE.3 handler is dedicated for notifying OS about memory hot-add
>  and hot-remove events.
> 
> -ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
> -hot-add and hot-remove events.
> -
>  Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>  ---------------------------------------------------------------
>  0xa00:
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 4aa5e3d..7887e57 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
>     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
>     NVDIMM Firmware Interface Table (NFIT).
> 
> -QEMU NVDIMM Implemention
> -========================
> +QEMU NVDIMM Implementation
> +==========================
>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
>  for NVDIMM ACPI.
> 
> @@ -82,6 +82,16 @@ Memory:
>     ACPI writes _DSM Input Data (based on the offset in the page):
>     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
>                  Root device.
> +
> +                The handle is completely QEMU internal thing, the values in
> +                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
> +                root device named NVDR), other values are reserved by other
> +                purpose.
> +
> +                Current reserved handle:
> +                0x10000 is reserved for QEMU internal DSM function called on
> +                the root device.
> +
>     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
>     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
>     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
> @@ -127,28 +137,22 @@ _DSM process diagram:
>   | result from the page     |      |              |
>   +--------------------------+      +--------------+
> 
> -Device Handle Reservation
> --------------------------
> -As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device
> -handle. The handle is completely QEMU internal thing, the values in range
> -[0, 0xFFFF] indicate nvdimm device (O means nvdimm root device named NVDR),
> -other values are reserved by other purpose.
> -
> -Current reserved handle:
> -0x10000 is reserved for QEMU internal DSM function called on the root
> -device.
> +NVDIMM hotplug
> +--------------
> +ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
> +hot-add and hot-remove events.
> 
>  QEMU internal use only _DSM function
>  ------------------------------------
> -UUID, 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, is reserved for QEMU internal
> -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 used by _FIT
> -   method to read a piece of FIT data from QEMU.
> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
> +   function (private QEMU function)
> +
> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from
> +   QEMU in 1 page sized increments which are then concatenated and returned
> +   as _FIT method result.
> 
>     Input parameters:
>     Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> @@ -156,27 +160,29 @@ There is the function introduced by QEMU and only used by QEMU internal.
>     Arg2 - Function Index, 0x1
>     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          |
> -   +----------+-------------+-------------+-----------------------------------+
> +   +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset |                 Description               |
> +   +----------+--------+--------+-------------------------------------------+
> +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
> +   |          |        |        | read from                                 |
> +   +----------+--------+--------+-------------------------------------------+
> 
>     Output:
> -   +----------+-------------+-------------+-----------------------------------+
> -   |  Filed   | Byte Length | Byte Offset | Description                       |
> -   +----------+-------------+-------------+-----------------------------------+
> -   |          |             |             | return status codes               |
> -   |          |             |             |   0x100 indicates fit has been    |
> -   | status   |     4       |    0        |   updated                         |
> -   |          |             |             | other follows Chapter 3 in DSM    |
> -   |          |             |             | Spec Rev1                         |
> -   +----------+-------------+-------------+-----------------------------------+
> -   | fit data |  Varies     |    4        | FIT data                          |
> -   |          |             |             |                                   |
> -   +----------+-------------+-------------+-----------------------------------+
> -
> -   The FIT offset is maintained by the caller itself, current offset plugs
> +   +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset |                 Description               |
> +   +----------+--------+--------+-------------------------------------------+
> +   |          |        |        | return status codes                       |
> +   |          |        |        | 0x100 - error caused by NFIT update while |
> +   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
> +   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
> +   +----------+--------+--------+-------------------------------------------+
> +   | length   |   4    |   4    | The fit size                              |
> +   +----------+-----------------+-------------------------------------------+
> +   | fit data | Varies |   8    | FIT data, its size is indicated by length |
> +   |          |        |        | filed above                               |
> +   +----------+--------+--------+-------------------------------------------+
> +
> +   The FIT offset is maintained by the OSPM itself, current offset plus
>     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.
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e5a3c18..830c475 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -490,8 +490,12 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> 
>      if (lpc->pm.acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
> -                            dev, errp);
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
> +        } else {
> +            acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
> +                                dev, errp);
> +        }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          if (lpc->pm.cpu_hotplug_legacy) {
>              legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp);
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 70f6451..ec4e64b 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -2,7 +2,6 @@
>  #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"
> @@ -233,8 +232,11 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
>                           DeviceState *dev, Error **errp)
>  {
>      MemStatus *mdev;
> -    AcpiEventStatusBits event;
> -    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +
> +    if (!dc->hotpluggable) {
> +        return;
> +    }
> 
>      mdev = acpi_memory_slot_status(mem_st, dev, errp);
>      if (!mdev) {
> @@ -242,23 +244,10 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
>      }
> 
>      mdev->dimm = dev;
> -
> -    /*
> -     * do not set is_enabled and is_inserting if the slot is plugged with
> -     * a nvdimm device to stop OSPM inquires memory region from the slot.
> -     */
> -    if (is_nvdimm) {
> -        event = ACPI_NVDIMM_HOTPLUG_STATUS;
> -    } else {
> -        mdev->is_enabled = true;
> -        event = ACPI_MEMORY_HOTPLUG_STATUS;
> -    }
> -
> +    mdev->is_enabled = true;
>      if (dev->hotplugged) {
> -        if (!is_nvdimm) {
> -            mdev->is_inserting = true;
> -        }
> -        acpi_send_event(DEVICE(hotplug_dev), event);
> +        mdev->is_inserting = true;
> +        acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
>      }
>  }
> 
> @@ -273,8 +262,6 @@ void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev,
>          return;
>      }
> 
> -    /* nvdimm device hot unplug is not supported yet. */
> -    assert(!object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM));
>      mdev->is_removing = true;
>      acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
>  }
> @@ -289,8 +276,6 @@ void acpi_memory_unplug_cb(MemHotplugState *mem_st,
>          return;
>      }
> 
> -    /* nvdimm device hot unplug is not supported yet. */
> -    assert(!object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM));
>      mdev->is_enabled = false;
>      mdev->dimm = NULL;
>  }
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index fc1a012..b8548cc 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -38,11 +38,7 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque)
>      GSList **list = opaque;
> 
>      if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
> -        DeviceState *dev = DEVICE(obj);
> -
> -        if (dev->realized) { /* only realized NVDIMMs matter */
> -            *list = g_slist_append(*list, DEVICE(obj));
> -        }
> +        *list = g_slist_append(*list, DEVICE(obj));
>      }
> 
>      object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
> @@ -375,17 +371,14 @@ static GArray *nvdimm_build_device_structure(void)
> 
>  static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
>  {
> -    qemu_mutex_init(&fit_buf->lock);
>      fit_buf->fit = g_array_new(false, true /* clear */, 1);
>  }
> 
>  static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
>  {
> -    qemu_mutex_lock(&fit_buf->lock);
>      g_array_free(fit_buf->fit, true);
>      fit_buf->fit = nvdimm_build_device_structure();
>      fit_buf->dirty = true;
> -    qemu_mutex_unlock(&fit_buf->lock);
>  }
> 
>  void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
> @@ -399,11 +392,9 @@ static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
>      NvdimmFitBuffer *fit_buf = &state->fit_buf;
>      unsigned int header;
> 
> -    qemu_mutex_lock(&fit_buf->lock);
> -
>      /* NVDIMM device is not plugged? */
>      if (!fit_buf->fit->len) {
> -        goto exit;
> +        return;
>      }
> 
>      acpi_add_table(table_offsets, table_data);
> @@ -417,9 +408,6 @@ static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
>      build_header(linker, table_data,
>                   (void *)(table_data->data + header), "NFIT",
>                   sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
> -
> -exit:
> -    qemu_mutex_unlock(&fit_buf->lock);
>  }
> 
>  struct NvdimmDsmIn {
> @@ -497,7 +485,7 @@ QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>                    offsetof(NvdimmDsmIn, arg3) > 4096);
> 
>  struct NvdimmFuncReadFITIn {
> -    uint32_t offset; /* the offset of FIT buffer. */
> +    uint32_t offset; /* the offset into FIT buffer. */
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> @@ -507,6 +495,7 @@ struct NvdimmFuncReadFITOut {
>      /* the size of buffer filled by QEMU. */
>      uint32_t len;
>      uint32_t func_ret_status; /* return status code. */
> +    uint32_t length; /* the length of fit. */
>      uint8_t fit[0]; /* the FIT data. */
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
> @@ -532,7 +521,12 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>  }
> 
> -#define NVDIMM_QEMU_RSVD_HANDLE_ROOT 0x10000
> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */
> +#define NVDIMM_DSM_RET_STATUS_FIT_CHANGED    0x100 /* FIT Changed */
> +
> +#define NVDIMM_QEMU_RSVD_HANDLE_ROOT         0x10000
> 
>  /* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
>  static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> @@ -542,34 +536,32 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>      NvdimmFuncReadFITIn *read_fit;
>      NvdimmFuncReadFITOut *read_fit_out;
>      GArray *fit;
> -    uint32_t read_len = 0, func_ret_status;
> +    uint32_t read_len = 0, func_ret_status, offset;
>      int size;
> 
>      read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> -    le32_to_cpus(&read_fit->offset);
> +    offset = le32_to_cpu(read_fit->offset);
> 
> -    qemu_mutex_lock(&fit_buf->lock);
>      fit = fit_buf->fit;
> 
>      nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
> -                 read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> -
> -    if (read_fit->offset > fit->len) {
> -        func_ret_status = 3 /* Invalid Input Parameters */;
> -        goto exit;
> -    }
> +                 offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> 
>      /* It is the first time to read FIT. */
> -    if (!read_fit->offset) {
> +    if (!offset) {
>          fit_buf->dirty = false;
>      } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */
> -        func_ret_status = 0x100 /* fit changed */;
> +        func_ret_status = NVDIMM_DSM_RET_STATUS_FIT_CHANGED;
>          goto exit;
>      }
> 
> -    func_ret_status = 0 /* Success */;
> -    read_len = MIN(fit->len - read_fit->offset,
> -                   4096 - sizeof(NvdimmFuncReadFITOut));
> +    if (offset > fit->len) {
> +        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> +        goto exit;
> +    }
> +
> +    func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
> +    read_len = MIN(fit->len - offset, 4096 - sizeof(NvdimmFuncReadFITOut));
> 
>  exit:
>      size = sizeof(NvdimmFuncReadFITOut) + read_len;
> @@ -577,12 +569,12 @@ exit:
> 
>      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);
> +    read_fit_out->length = cpu_to_le32(read_len);
> +    memcpy(read_fit_out->fit, fit->data + offset, read_len);
> 
>      cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
> 
>      g_free(read_fit_out);
> -    qemu_mutex_unlock(&fit_buf->lock);
>  }
> 
>  static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> @@ -592,12 +584,12 @@ static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>      case 0x0:
>          nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
>          return;
> -    case 0x1 /*Read FIT */:
> +    case 0x1 /* Read FIT */:
>          nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
>          return;
>      }
> 
> -    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
>  }
> 
>  static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> @@ -659,7 +651,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
> 
>      nvdimm_debug("label_size %#x, max_xfer %#x.\n", label_size, mxfer);
> 
> -    label_size_out.func_ret_status = cpu_to_le32(0 /* Success */);
> +    label_size_out.func_ret_status = cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
>      label_size_out.label_size = cpu_to_le32(label_size);
>      label_size_out.max_xfer = cpu_to_le32(mxfer);
> 
> @@ -670,7 +662,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
>  static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
>                                             uint32_t offset, uint32_t length)
>  {
> -    uint32_t ret = 3 /* Invalid Input Parameters */;
> +    uint32_t ret = NVDIMM_DSM_RET_STATUS_INVALID;
> 
>      if (offset + length < offset) {
>          nvdimm_debug("offset %#x + length %#x is overflow.\n", offset,
> @@ -690,7 +682,7 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
>          return ret;
>      }
> 
> -    return 0 /* Success */;
> +    return NVDIMM_DSM_RET_STATUS_SUCCESS;
>  }
> 
>  /*
> @@ -714,7 +706,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> 
>      status = nvdimm_rw_label_data_check(nvdimm, get_label_data->offset,
>                                          get_label_data->length);
> -    if (status != 0 /* Success */) {
> +    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
>          nvdimm_dsm_no_payload(status, dsm_mem_addr);
>          return;
>      }
> @@ -724,7 +716,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>      get_label_data_out = g_malloc(size);
> 
>      get_label_data_out->len = cpu_to_le32(size);
> -    get_label_data_out->func_ret_status = cpu_to_le32(0 /* Success */);
> +    get_label_data_out->func_ret_status =
> +                                cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
>      nvc->read_label_data(nvdimm, get_label_data_out->out_buf,
>                           get_label_data->length, get_label_data->offset);
> 
> @@ -752,7 +745,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> 
>      status = nvdimm_rw_label_data_check(nvdimm, set_label_data->offset,
>                                          set_label_data->length);
> -    if (status != 0 /* Success */) {
> +    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
>          nvdimm_dsm_no_payload(status, dsm_mem_addr);
>          return;
>      }
> @@ -762,7 +755,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> 
>      nvc->write_label_data(nvdimm, set_label_data->in_buf,
>                            set_label_data->length, set_label_data->offset);
> -    nvdimm_dsm_no_payload(0 /* Success */, dsm_mem_addr);
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS, dsm_mem_addr);
>  }
> 
>  static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> @@ -813,7 +806,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>          break;
>      }
> 
> -    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
>  }
> 
>  static uint64_t
> @@ -850,7 +843,7 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      if (in->revision != 0x1 /* Currently we only support DSM Spec Rev1. */) {
>          nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
>                       in->revision, 0x1);
> -        nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +        nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
>          goto exit;
>      }
> 
> @@ -881,6 +874,13 @@ static const MemoryRegionOps nvdimm_dsm_ops = {
>      },
>  };
> 
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> +    if (dev->hotplugged) {
> +        acpi_send_event(DEVICE(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS);
> +    }
> +}
> +
>  void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>                              FWCfgState *fw_cfg, Object *owner)
>  {
> @@ -1031,7 +1031,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
>      aml_append(unsupport, ifctx);
> 
>      /* No function is supported yet. */
> -    byte_list[0] = 1 /* Not Supported */;
> +    byte_list[0] = NVDIMM_DSM_RET_STATUS_UNSUPPORT;
>      aml_append(unsupport, aml_return(aml_buffer(1, byte_list)));
>      aml_append(method, unsupport);
> 
> @@ -1094,7 +1094,7 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>      aml_append(dev, method);
>  }
> 
> -static void nvdimm_build_fit(Aml *dev)
> +static void nvdimm_build_fit_method(Aml *dev)
>  {
>      Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
>      Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
> @@ -1103,13 +1103,11 @@ static void nvdimm_build_fit(Aml *dev)
>      buf_size = aml_local(1);
>      fit = aml_local(2);
> 
> -    aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL),
> -               aml_int(0), NVDIMM_DSM_RFIT_STATUS));
> +    aml_append(dev, aml_name_decl(NVDIMM_DSM_RFIT_STATUS, aml_int(0)));
> 
>      /* build helper function, RFIT. */
>      method = aml_method("RFIT", 1, AML_SERIALIZED);
> -    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
> -                                              aml_int(0), "OFST"));
> +    aml_append(method, aml_name_decl("OFST", aml_int(0)));
> 
>      /* prepare input package. */
>      pkg = aml_package(1);
> @@ -1137,11 +1135,9 @@ static void nvdimm_build_fit(Aml *dev)
>      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));
> -
> +    aml_append(method, aml_create_dword_field(buf,
> +               aml_int(4) /* offset at byte 4 */, "LENG"));
> +    aml_append(method, aml_store(aml_name("LENG"), 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)));
> @@ -1150,7 +1146,7 @@ static void nvdimm_build_fit(Aml *dev)
>      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.*/
> +                            aml_int(8 * BITS_PER_BYTE), /* offset at byte 4.*/
>                              buf_size, "BUFF"));
>      aml_append(method, aml_return(aml_name("BUFF")));
>      aml_append(dev, method);
> @@ -1171,7 +1167,7 @@ static void nvdimm_build_fit(Aml *dev)
>       * again.
>       */
>      ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
> -                             aml_int(0x100 /* fit changed */)));
> +                             aml_int(NVDIMM_DSM_RET_STATUS_FIT_CHANGED)));
>      aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
>      aml_append(ifctx, aml_store(aml_int(0), offset));
>      aml_append(whilectx, ifctx);
> @@ -1251,7 +1247,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_fit_method(dev);
> 
>      nvdimm_build_nvdimm_devices(dev, ram_slots);
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 2adc246..17d36bd 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -378,7 +378,12 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> 
>      if (s->acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, dev, errp);
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
> +        } else {
> +            acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug,
> +                                dev, errp);
> +        }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>          acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index ab34c19..17ac986 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -35,17 +35,6 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>      }
>  }
> 
> -void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> -                               DeviceState *plugged_dev,
> -                               Error **errp)
> -{
> -    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> -
> -    if (hdc->post_plug) {
> -        hdc->post_plug(plug_handler, plugged_dev, errp);
> -    }
> -}
> -
>  void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>                                      DeviceState *plugged_dev,
>                                      Error **errp)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index d835e62..5783442 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -945,21 +945,10 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>                  goto child_realize_fail;
>              }
>          }
> -
>          if (dev->hotplugged) {
>              device_reset(dev);
>          }
>          dev->pending_deleted_event = false;
> -        dev->realized = value;
> -
> -        if (hotplug_ctrl) {
> -            hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err);
> -        }
> -
> -        if (local_err != NULL) {
> -            dev->realized = value;
> -            goto post_realize_fail;
> -        }
>      } else if (!value && dev->realized) {
>          Error **local_errp = NULL;
>          QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> @@ -976,14 +965,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          }
>          dev->pending_deleted_event = true;
>          DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
> +    }
> 
> -        if (local_err != NULL) {
> -            goto fail;
> -        }
> -
> -        dev->realized = value;
> +    if (local_err != NULL) {
> +        goto fail;
>      }
> 
> +    dev->realized = value;
>      return;
> 
>  child_realize_fail:
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 200963f..0403452 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1700,22 +1700,16 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
> 
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
>  out:
>      error_propagate(errp, local_err);
>  }
> 
> -static void pc_dimm_post_plug(HotplugHandler *hotplug_dev,
> -                              DeviceState *dev, Error **errp)
> -{
> -    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> -        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
> -    }
> -}
> -
>  static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
>                                     DeviceState *dev, Error **errp)
>  {
> @@ -1988,14 +1982,6 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>      }
>  }
> 
> -static void pc_machine_device_post_plug_cb(HotplugHandler *hotplug_dev,
> -                                           DeviceState *dev, Error **errp)
> -{
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_post_plug(hotplug_dev, dev, errp);
> -    }
> -}
> -
>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                                  DeviceState *dev, Error **errp)
>  {
> @@ -2330,7 +2316,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->reset = pc_machine_reset;
>      hc->pre_plug = pc_machine_device_pre_plug_cb;
>      hc->plug = pc_machine_device_plug_cb;
> -    hc->post_plug = pc_machine_device_post_plug_cb;
>      hc->unplug_request = pc_machine_device_unplug_request_cb;
>      hc->unplug = pc_machine_device_unplug_cb;
>      nc->nmi_monitor_handler = x86_nmi;
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index 10ca5b6..c0db869 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -47,7 +47,6 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   * @parent: Opaque parent interface.
>   * @pre_plug: pre plug callback called at start of device.realize(true)
>   * @plug: plug callback called at end of device.realize(true).
> - * @post_pug: post plug callback called after device is successfully plugged.
>   * @unplug_request: unplug request callback.
>   *                  Used as a means to initiate device unplug for devices that
>   *                  require asynchronous unplug handling.
> @@ -62,7 +61,6 @@ typedef struct HotplugHandlerClass {
>      /* <public> */
>      hotplug_fn pre_plug;
>      hotplug_fn plug;
> -    hotplug_fn post_plug;
>      hotplug_fn unplug_request;
>      hotplug_fn unplug;
>  } HotplugHandlerClass;
> @@ -85,14 +83,6 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
>                                DeviceState *plugged_dev,
>                                Error **errp);
> 
> -/**
> - * hotplug_handler_post_plug:
> - *
> - * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
> - */
> -void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> -                               DeviceState *plugged_dev,
> -                               Error **errp);
> 
>  /**
>   * hotplug_handler_unplug_request:
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 33cd421..4b90584 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -103,16 +103,11 @@ typedef struct NVDIMMClass NVDIMMClass;
>   * devices which is updated after the NVDIMM device is plugged or
>   * unplugged.
>   *
> - * Rules to use the buffer:
> - *    1) the user should hold the @lock to access the buffer.
> - *    2) mark @dirty whenever the buffer is updated.
> - *
> - * These rules preserve NVDIMM ACPI _FIT method to read incomplete
> - * or obsolete fit info if fit update happens during multiple RFIT
> - * calls.
> + * Mark @dirty whenever the buffer is updated so that it preserves NVDIMM
> + * ACPI _FIT method to read incomplete or obsolete fit info if fit update
> + * happens during multiple RFIT calls.
>   */
>  struct NvdimmFitBuffer {
> -    QemuMutex lock;
>      GArray *fit;
>      bool dirty;
>  };
> @@ -138,4 +133,5 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         BIOSLinker *linker, AcpiNVDIMMState *state,
>                         uint32_t ram_slots);
>  void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
>  #endif
>