[Qemu-devel] [PATCH 00/14] improve nvdimm hotplug

[Qemu-devel] [PATCH 00/14] improve nvdimm hotplug

[Xiao Guangrong]

This patchset is against commit 72213633 (acpi: fix DMAR device scope
for IOAPIC) on pci branch of Michael's git tree.

It improves nvdimm hotplug based on the comments from Igor and Stefan
during this release window

Thanks!

Xiao Guangrong (14):
  qdev: hotplug: drop HotplugHandler.post_plug callback
  nvdimm acpi: drop the lock of fit buffer
  pc: memhp: move nvdimm hotplug out of memory hotplug
  pc: memhp: stop handling nvdimm hotplug in pc_dimm_unplug
  nvdimm acpi: clean up nvdimm_build_acpi
  docs: improve the doc of Read FIT method
  nvdimm acpi: rename nvdimm_plugged_device_list
  nvdimm acpi: cleanup nvdimm_build_fit_method
  nvdimm acpi: rename nvdimm_acpi_hotplug
  nvdimm acpi: define DSM return codes
  nvdimm acpi: fix two comments
  nvdimm acpi: rename nvdimm_dsm_reserved_root
  nvdimm acpi: use aml_name_decl to define named object
  nvdimm acpi: introduce NVDIMM_DSM_MEMORY_SIZE

 default-configs/mips-softmmu-common.mak |   1 +
 docs/specs/acpi_mem_hotplug.txt         |   3 -
 docs/specs/acpi_nvdimm.txt              |  99 ++++++++++---------
 hw/acpi/ich9.c                          |   8 +-
 hw/acpi/memory_hotplug.c                |  31 ++----
 hw/acpi/nvdimm.c                        | 166 ++++++++++++++++----------------
 hw/acpi/piix4.c                         |   7 +-
 hw/core/hotplug.c                       |  11 ---
 hw/core/qdev.c                          |  20 +---
 hw/i386/pc.c                            |  29 +-----
 include/hw/hotplug.h                    |  11 ---
 include/hw/mem/nvdimm.h                 |  20 ++--
 12 files changed, 172 insertions(+), 234 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/14] qdev: hotplug: drop HotplugHandler.post_plug callback

[Xiao Guangrong]

as nvdimm acpi is okay to build fit when the nvdimm device
has not been 'realized'

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c     |  6 +-----
 hw/core/hotplug.c    | 11 -----------
 hw/core/qdev.c       | 20 ++++----------------
 hw/i386/pc.c         | 23 ++++-------------------
 include/hw/hotplug.h | 11 -----------
 5 files changed, 9 insertions(+), 62 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 602ec54..623bb36 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);
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 c011552..8ef3920 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1715,22 +1715,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)
 {
@@ -2008,14 +2002,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)
 {
@@ -2320,7 +2306,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..1a0516a 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;
@@ -86,15 +84,6 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
                               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:
  *
  * Calls #HotplugHandlerClass.unplug_request callback of @plug_handler.
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 01/14] qdev: hotplug: drop HotplugHandler.post_plug callback

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:36PM +0800, Xiao Guangrong wrote:
> as nvdimm acpi is okay to build fit when the nvdimm device
> has not been 'realized'
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c     |  6 +-----
>  hw/core/hotplug.c    | 11 -----------
>  hw/core/qdev.c       | 20 ++++----------------
>  hw/i386/pc.c         | 23 ++++-------------------
>  include/hw/hotplug.h | 11 -----------
>  5 files changed, 9 insertions(+), 62 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 01/14] qdev: hotplug: drop HotplugHandler.post_plug callback

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:36 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> as nvdimm acpi is okay to build fit when the nvdimm device
> has not been 'realized'
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c     |  6 +-----
>  hw/core/hotplug.c    | 11 -----------
>  hw/core/qdev.c       | 20 ++++----------------
>  hw/i386/pc.c         | 23 ++++-------------------
>  include/hw/hotplug.h | 11 -----------
>  5 files changed, 9 insertions(+), 62 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 602ec54..623bb36 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);
> 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 c011552..8ef3920 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1715,22 +1715,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)
>  {
> @@ -2008,14 +2002,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)
>  {
> @@ -2320,7 +2306,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..1a0516a 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;
> @@ -86,15 +84,6 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
>                                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:
>   *
>   * Calls #HotplugHandlerClass.unplug_request callback of @plug_handler.

[Qemu-devel] [PATCH 02/14] nvdimm acpi: drop the lock of fit buffer

[Xiao Guangrong]

as there is a global lock to protect vm-exit handlers and
QMP/monitor, this lock can be dropped

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c        | 11 +----------
 include/hw/mem/nvdimm.h | 17 +++++------------
 2 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 623bb36..0fe3547 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -371,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)
@@ -395,11 +392,10 @@ 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);
@@ -413,9 +409,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 {
@@ -544,7 +537,6 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
     read_fit = (NvdimmFuncReadFITIn *)in->arg3;
     le32_to_cpus(&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",
@@ -578,7 +570,6 @@ exit:
     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,
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 33cd421..d3ffb25 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -99,20 +99,13 @@ typedef struct NVDIMMClass NVDIMMClass;
 #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.
- *
- * 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.
+ * NvdimmFitBuffer:
+ * @fit: FIT structures for present NVDIMMs. It is updated when
+ *   the NVDIMM device is plugged or unplugged.
+ * @dirty: It allows OSPM to detect change and restart read in
+ *   progress if there is any.
  */
 struct NvdimmFitBuffer {
-    QemuMutex lock;
     GArray *fit;
     bool dirty;
 };
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 02/14] nvdimm acpi: drop the lock of fit buffer

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:37PM +0800, Xiao Guangrong wrote:
> as there is a global lock to protect vm-exit handlers and
> QMP/monitor, this lock can be dropped
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c        | 11 +----------
>  include/hw/mem/nvdimm.h | 17 +++++------------
>  2 files changed, 6 insertions(+), 22 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 02/14] nvdimm acpi: drop the lock of fit buffer

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:37 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> as there is a global lock to protect vm-exit handlers and
> QMP/monitor, this lock can be dropped
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c        | 11 +----------
>  include/hw/mem/nvdimm.h | 17 +++++------------
>  2 files changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 623bb36..0fe3547 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -371,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)
> @@ -395,11 +392,10 @@ 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);
> @@ -413,9 +409,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 {
> @@ -544,7 +537,6 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>      read_fit = (NvdimmFuncReadFITIn *)in->arg3;
>      le32_to_cpus(&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",
> @@ -578,7 +570,6 @@ exit:
>      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,
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 33cd421..d3ffb25 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -99,20 +99,13 @@ typedef struct NVDIMMClass NVDIMMClass;
>  #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.
> - *
> - * 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.
> + * NvdimmFitBuffer:
> + * @fit: FIT structures for present NVDIMMs. It is updated when
> + *   the NVDIMM device is plugged or unplugged.
> + * @dirty: It allows OSPM to detect change and restart read in
> + *   progress if there is any.
>   */
>  struct NvdimmFitBuffer {
> -    QemuMutex lock;
>      GArray *fit;
>      bool dirty;
>  };

[Qemu-devel] [PATCH 03/14] pc: memhp: move nvdimm hotplug out of memory hotplug

[Xiao Guangrong]

as they use completely different way to handle hotplug event

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 default-configs/mips-softmmu-common.mak |  1 +
 docs/specs/acpi_mem_hotplug.txt         |  3 ---
 docs/specs/acpi_nvdimm.txt              |  5 +++++
 hw/acpi/ich9.c                          |  8 ++++++--
 hw/acpi/memory_hotplug.c                | 31 ++++++++-----------------------
 hw/acpi/nvdimm.c                        |  7 +++++++
 hw/acpi/piix4.c                         |  7 ++++++-
 include/hw/mem/nvdimm.h                 |  1 +
 8 files changed, 34 insertions(+), 29 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_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..d244147 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -127,6 +127,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 event.
+
 Device Handle Reservation
 -------------------------
 As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device
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 0fe3547..5156565 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -868,6 +868,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/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index d3ffb25..60585c3 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -131,4 +131,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 03/14] pc: memhp: move nvdimm hotplug out of memory hotplug

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:38PM +0800, Xiao Guangrong wrote:
> as they use completely different way to handle hotplug event
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  default-configs/mips-softmmu-common.mak |  1 +
>  docs/specs/acpi_mem_hotplug.txt         |  3 ---
>  docs/specs/acpi_nvdimm.txt              |  5 +++++
>  hw/acpi/ich9.c                          |  8 ++++++--
>  hw/acpi/memory_hotplug.c                | 31 ++++++++-----------------------
>  hw/acpi/nvdimm.c                        |  7 +++++++
>  hw/acpi/piix4.c                         |  7 ++++++-
>  include/hw/mem/nvdimm.h                 |  1 +
>  8 files changed, 34 insertions(+), 29 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 03/14] pc: memhp: move nvdimm hotplug out of memory hotplug

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:38 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> as they use completely different way to handle hotplug event
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  default-configs/mips-softmmu-common.mak |  1 +
>  docs/specs/acpi_mem_hotplug.txt         |  3 ---
>  docs/specs/acpi_nvdimm.txt              |  5 +++++
>  hw/acpi/ich9.c                          |  8 ++++++--
>  hw/acpi/memory_hotplug.c                | 31 ++++++++-----------------------
>  hw/acpi/nvdimm.c                        |  7 +++++++
>  hw/acpi/piix4.c                         |  7 ++++++-
>  include/hw/mem/nvdimm.h                 |  1 +
>  8 files changed, 34 insertions(+), 29 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_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..d244147 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -127,6 +127,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 event.
> +
>  Device Handle Reservation
>  -------------------------
>  As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device
> 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 0fe3547..5156565 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -868,6 +868,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/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index d3ffb25..60585c3 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -131,4 +131,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

[Qemu-devel] [PATCH 04/14] pc: memhp: stop handling nvdimm hotplug in pc_dimm_unplug

[Xiao Guangrong]

as it is never called when nvdimm hotplug happens

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/i386/pc.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8ef3920..97e23c4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1761,12 +1761,6 @@ 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);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/14] pc: memhp: stop handling nvdimm hotplug in pc_dimm_unplug

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:39PM +0800, Xiao Guangrong wrote:
> as it is never called when nvdimm hotplug happens
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/i386/pc.c | 6 ------
>  1 file changed, 6 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 04/14] pc: memhp: stop handling nvdimm hotplug in pc_dimm_unplug

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:39 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> as it is never called when nvdimm hotplug happens
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8ef3920..97e23c4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1761,12 +1761,6 @@ 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);
>  

[Qemu-devel] [PATCH 05/14] nvdimm acpi: clean up nvdimm_build_acpi

[Xiao Guangrong]

To make the code more clearer, we
1) check ram_slots first, and build ssdt & nfit only when it is available
2) use nvdimm_get_plugged_device_list() to check if there is nvdimm device
   plugged

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5156565..65eb6c9 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -392,12 +392,6 @@ static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
     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. */
@@ -1275,14 +1269,22 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                        BIOSLinker *linker, AcpiNVDIMMState *state,
                        uint32_t ram_slots)
 {
-    nvdimm_build_nfit(state, table_offsets, table_data, linker);
+    GSList *device_list;
 
-    /*
-     * NVDIMM device is allowed to be plugged only if there is available
-     * slot.
-     */
-    if (ram_slots) {
-        nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
-                          ram_slots);
+    /* no nvdimm device can be plugged. */
+    if (!ram_slots) {
+        return;
     }
+
+    nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
+                      ram_slots);
+
+    device_list = nvdimm_get_plugged_device_list();
+    /* no NVDIMM device is plugged. */
+    if (!device_list) {
+        return;
+    }
+
+    nvdimm_build_nfit(state, table_offsets, table_data, linker);
+    g_slist_free(device_list);
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 05/14] nvdimm acpi: clean up nvdimm_build_acpi

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:40PM +0800, Xiao Guangrong wrote:
> To make the code more clearer, we
> 1) check ram_slots first, and build ssdt & nfit only when it is available
> 2) use nvdimm_get_plugged_device_list() to check if there is nvdimm device
>    plugged
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 05/14] nvdimm acpi: clean up nvdimm_build_acpi

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:40 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> To make the code more clearer, we
> 1) check ram_slots first, and build ssdt & nfit only when it is available
> 2) use nvdimm_get_plugged_device_list() to check if there is nvdimm device
>    plugged
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

>  hw/acpi/nvdimm.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 5156565..65eb6c9 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -392,12 +392,6 @@ static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
>      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. */
> @@ -1275,14 +1269,22 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         BIOSLinker *linker, AcpiNVDIMMState *state,
>                         uint32_t ram_slots)
>  {
> -    nvdimm_build_nfit(state, table_offsets, table_data, linker);
> +    GSList *device_list;
>  
> -    /*
> -     * NVDIMM device is allowed to be plugged only if there is available
> -     * slot.
> -     */
> -    if (ram_slots) {
> -        nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
> -                          ram_slots);
> +    /* no nvdimm device can be plugged. */
> +    if (!ram_slots) {
> +        return;
>      }
> +
> +    nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
> +                      ram_slots);
> +
> +    device_list = nvdimm_get_plugged_device_list();
> +    /* no NVDIMM device is plugged. */
> +    if (!device_list) {
> +        return;
> +    }
> +
> +    nvdimm_build_nfit(state, table_offsets, table_data, linker);
> +    g_slist_free(device_list);
>  }

[Qemu-devel] [PATCH 06/14] docs: improve the doc of Read FIT method

[Xiao Guangrong]

Improve the description and clearly document the length field

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 docs/specs/acpi_nvdimm.txt | 96 +++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 49 deletions(-)

diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index d244147..3f322e6 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.
 
@@ -80,8 +80,17 @@ Memory:
    emulates _DSM access and writes the output data to it.
 
    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.
+   [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle.
+
+                The handle is completely QEMU internal thing, the values in
+                range [1, 0xFFFF] indicate nvdimm device. Other values are
+                reserved for other purposes.
+
+                Reserved handles:
+                0 is reserved for nvdimm root device named NVDR.
+                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.
@@ -132,28 +141,12 @@ NVDIMM hotplug
 ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
 hot-add event.
 
-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.
-
 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.
+   _FIT method uses _DSM method 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}
@@ -161,29 +154,34 @@ 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          |
-   +----------+-------------+-------------+-----------------------------------+
-
-   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
-   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).
+   +----------+--------+--------+-------------------------------------------+
+   |  Field   | Length | Offset |                 Description               |
+   +----------+--------+--------+-------------------------------------------+
+   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
+   |          |        |        | read from                                 |
+   +----------+--------+--------+-------------------------------------------+
+
+   Output layout in the dsm memory page:
+   +----------+--------+--------+-------------------------------------------+
+   |  Field   | Length | Offset |                 Description               |
+   +----------+--------+--------+-------------------------------------------+
+   | length   |   4    |   0    | length of entire returned data            |
+   |          |        |        | (including this header)                   |
+   +----------+-----------------+-------------------------------------------+
+   |          |        |        | return status codes                       |
+   |          |        |        | 0x0 - success                             |
+   |          |        |        | 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    | contains FIT data, this field is present  |
+   |          |        |        | if status field is 0;                     |
+   +----------+--------+--------+-------------------------------------------+
+
+   The FIT offset is maintained by the OSPM itself, current offset plus
+   the size of the fit data returned by the function is the next offset
+   OSPM should read. When all FIT data has been read out, zero fit data
+   size is returned.
+
+   If it returns status code 0x100, OSPM should restart to read FIT (read
+   from offset 0 again).
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 06/14] docs: improve the doc of Read FIT method

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:41PM +0800, Xiao Guangrong wrote:
> Improve the description and clearly document the length field
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  docs/specs/acpi_nvdimm.txt | 96 +++++++++++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 49 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 06/14] docs: improve the doc of Read FIT method

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:41 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Improve the description and clearly document the length field
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  docs/specs/acpi_nvdimm.txt | 96 +++++++++++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 49 deletions(-)
> 
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index d244147..3f322e6 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.
>  
> @@ -80,8 +80,17 @@ Memory:
>     emulates _DSM access and writes the output data to it.
>  
>     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.
> +   [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle.
> +
> +                The handle is completely QEMU internal thing, the values in
> +                range [1, 0xFFFF] indicate nvdimm device. Other values are
> +                reserved for other purposes.
> +
> +                Reserved handles:
> +                0 is reserved for nvdimm root device named NVDR.
> +                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.
> @@ -132,28 +141,12 @@ NVDIMM hotplug
>  ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
>  hot-add event.
>  
> -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.
> -
>  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.
> +   _FIT method uses _DSM method 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}
> @@ -161,29 +154,34 @@ 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          |
> -   +----------+-------------+-------------+-----------------------------------+
> -
> -   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
> -   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).
> +   +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset |                 Description               |
> +   +----------+--------+--------+-------------------------------------------+
> +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
> +   |          |        |        | read from                                 |
> +   +----------+--------+--------+-------------------------------------------+
> +
> +   Output layout in the dsm memory page:
> +   +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset |                 Description               |
> +   +----------+--------+--------+-------------------------------------------+
> +   | length   |   4    |   0    | length of entire returned data            |
> +   |          |        |        | (including this header)                   |
> +   +----------+-----------------+-------------------------------------------+
> +   |          |        |        | return status codes                       |
> +   |          |        |        | 0x0 - success                             |
> +   |          |        |        | 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    | contains FIT data, this field is present  |
> +   |          |        |        | if status field is 0;                     |
> +   +----------+--------+--------+-------------------------------------------+
> +
> +   The FIT offset is maintained by the OSPM itself, current offset plus
> +   the size of the fit data returned by the function is the next offset
> +   OSPM should read. When all FIT data has been read out, zero fit data
> +   size is returned.
> +
> +   If it returns status code 0x100, OSPM should restart to read FIT (read
> +   from offset 0 again).

[Qemu-devel] [PATCH 07/14] nvdimm acpi: rename nvdimm_plugged_device_list

[Xiao Guangrong]

Its behavior has been changed as the nvdimm device which is being
realized also will be handled in this function, so rename it to
reflect the fact

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 65eb6c9..f2c0659 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -33,7 +33,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "hw/mem/nvdimm.h"
 
-static int nvdimm_plugged_device_list(Object *obj, void *opaque)
+static int nvdimm_device_list(Object *obj, void *opaque)
 {
     GSList **list = opaque;
 
@@ -41,23 +41,22 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque)
         *list = g_slist_append(*list, DEVICE(obj));
     }
 
-    object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
+    object_child_foreach(obj, nvdimm_device_list, opaque);
     return 0;
 }
 
 /*
- * inquire plugged NVDIMM devices and link them into the list which is
+ * inquire NVDIMM devices and link them into the list which is
  * returned to the caller.
  *
  * Note: it is the caller's responsibility to free the list to avoid
  * memory leak.
  */
-static GSList *nvdimm_get_plugged_device_list(void)
+static GSList *nvdimm_get_device_list(void)
 {
     GSList *list = NULL;
 
-    object_child_foreach(qdev_get_machine(), nvdimm_plugged_device_list,
-                         &list);
+    object_child_foreach(qdev_get_machine(), nvdimm_device_list, &list);
     return list;
 }
 
@@ -215,7 +214,7 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
 static NVDIMMDevice *nvdimm_get_device_by_handle(uint32_t handle)
 {
     NVDIMMDevice *nvdimm = NULL;
-    GSList *list, *device_list = nvdimm_get_plugged_device_list();
+    GSList *list, *device_list = nvdimm_get_device_list();
 
     for (list = device_list; list; list = list->next) {
         NVDIMMDevice *nvd = list->data;
@@ -346,7 +345,7 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
 
 static GArray *nvdimm_build_device_structure(void)
 {
-    GSList *device_list = nvdimm_get_plugged_device_list();
+    GSList *device_list = nvdimm_get_device_list();
     GArray *structures = g_array_new(false, true /* clear */, 1);
 
     for (; device_list; device_list = device_list->next) {
@@ -1279,7 +1278,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
     nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
                       ram_slots);
 
-    device_list = nvdimm_get_plugged_device_list();
+    device_list = nvdimm_get_device_list();
     /* no NVDIMM device is plugged. */
     if (!device_list) {
         return;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/14] nvdimm acpi: rename nvdimm_plugged_device_list

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:42PM +0800, Xiao Guangrong wrote:
> Its behavior has been changed as the nvdimm device which is being
> realized also will be handled in this function, so rename it to
> reflect the fact
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 07/14] nvdimm acpi: rename nvdimm_plugged_device_list

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:42 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Its behavior has been changed as the nvdimm device which is being
> realized also will be handled in this function, so rename it to
> reflect the fact
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 65eb6c9..f2c0659 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -33,7 +33,7 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/mem/nvdimm.h"
>  
> -static int nvdimm_plugged_device_list(Object *obj, void *opaque)
> +static int nvdimm_device_list(Object *obj, void *opaque)
>  {
>      GSList **list = opaque;
>  
> @@ -41,23 +41,22 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque)
>          *list = g_slist_append(*list, DEVICE(obj));
>      }
>  
> -    object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
> +    object_child_foreach(obj, nvdimm_device_list, opaque);
>      return 0;
>  }
>  
>  /*
> - * inquire plugged NVDIMM devices and link them into the list which is
> + * inquire NVDIMM devices and link them into the list which is
>   * returned to the caller.
>   *
>   * Note: it is the caller's responsibility to free the list to avoid
>   * memory leak.
>   */
> -static GSList *nvdimm_get_plugged_device_list(void)
> +static GSList *nvdimm_get_device_list(void)
>  {
>      GSList *list = NULL;
>  
> -    object_child_foreach(qdev_get_machine(), nvdimm_plugged_device_list,
> -                         &list);
> +    object_child_foreach(qdev_get_machine(), nvdimm_device_list, &list);
>      return list;
>  }
>  
> @@ -215,7 +214,7 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
>  static NVDIMMDevice *nvdimm_get_device_by_handle(uint32_t handle)
>  {
>      NVDIMMDevice *nvdimm = NULL;
> -    GSList *list, *device_list = nvdimm_get_plugged_device_list();
> +    GSList *list, *device_list = nvdimm_get_device_list();
>  
>      for (list = device_list; list; list = list->next) {
>          NVDIMMDevice *nvd = list->data;
> @@ -346,7 +345,7 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
>  
>  static GArray *nvdimm_build_device_structure(void)
>  {
> -    GSList *device_list = nvdimm_get_plugged_device_list();
> +    GSList *device_list = nvdimm_get_device_list();
>      GArray *structures = g_array_new(false, true /* clear */, 1);
>  
>      for (; device_list; device_list = device_list->next) {
> @@ -1279,7 +1278,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>      nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
>                        ram_slots);
>  
> -    device_list = nvdimm_get_plugged_device_list();
> +    device_list = nvdimm_get_device_list();
>      /* no NVDIMM device is plugged. */
>      if (!device_list) {
>          return;

[Qemu-devel] [PATCH 08/14] nvdimm acpi: cleanup nvdimm_build_fit

[Xiao Guangrong]

inline buf_size to refine the code a bit

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index f2c0659..148999e 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1134,11 +1134,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_shiftleft(buf_size, aml_int(3)),
-                                 buf_size));
     aml_append(method, aml_create_field(buf,
                             aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
-                            buf_size, "BUFF"));
+                            aml_shiftleft(buf_size, aml_int(3)), "BUFF"));
     aml_append(method, aml_return(aml_name("BUFF")));
     aml_append(dev, method);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 08/14] nvdimm acpi: cleanup nvdimm_build_fit

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:43PM +0800, Xiao Guangrong wrote:
> inline buf_size to refine the code a bit
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 08/14] nvdimm acpi: cleanup nvdimm_build_fit

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:43 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> inline buf_size to refine the code a bit
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index f2c0659..148999e 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1134,11 +1134,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_shiftleft(buf_size, aml_int(3)),
> -                                 buf_size));
>      aml_append(method, aml_create_field(buf,
>                              aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
> -                            buf_size, "BUFF"));
> +                            aml_shiftleft(buf_size, aml_int(3)), "BUFF"));
>      aml_append(method, aml_return(aml_name("BUFF")));
>      aml_append(dev, method);
>  

[Qemu-devel] [PATCH 09/14] nvdimm acpi: rename nvdimm_acpi_hotplug

[Xiao Guangrong]

Rename it to nvdimm_plug()

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c        | 2 +-
 hw/i386/pc.c            | 2 +-
 include/hw/mem/nvdimm.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 148999e..7733f14 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -380,7 +380,7 @@ static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
     fit_buf->dirty = true;
 }
 
-void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
+void nvdimm_plug(AcpiNVDIMMState *state)
 {
     nvdimm_build_fit_buffer(&state->fit_buf);
 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 97e23c4..edc96a6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1716,7 +1716,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
     }
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
-        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
+        nvdimm_plug(&pcms->acpi_nvdimm_state);
     }
 
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 60585c3..03e1ff9 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -130,6 +130,6 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                        BIOSLinker *linker, AcpiNVDIMMState *state,
                        uint32_t ram_slots);
-void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
+void nvdimm_plug(AcpiNVDIMMState *state);
 void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 09/14] nvdimm acpi: rename nvdimm_acpi_hotplug

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:44PM +0800, Xiao Guangrong wrote:
> Rename it to nvdimm_plug()
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c        | 2 +-
>  hw/i386/pc.c            | 2 +-
>  include/hw/mem/nvdimm.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 09/14] nvdimm acpi: rename nvdimm_acpi_hotplug

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:44 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Rename it to nvdimm_plug()
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>


> ---
>  hw/acpi/nvdimm.c        | 2 +-
>  hw/i386/pc.c            | 2 +-
>  include/hw/mem/nvdimm.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 148999e..7733f14 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -380,7 +380,7 @@ static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
>      fit_buf->dirty = true;
>  }
>  
> -void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
> +void nvdimm_plug(AcpiNVDIMMState *state)
>  {
>      nvdimm_build_fit_buffer(&state->fit_buf);
>  }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 97e23c4..edc96a6 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1716,7 +1716,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      }
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> -        nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
> +        nvdimm_plug(&pcms->acpi_nvdimm_state);
>      }
>  
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 60585c3..03e1ff9 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -130,6 +130,6 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         BIOSLinker *linker, AcpiNVDIMMState *state,
>                         uint32_t ram_slots);
> -void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
> +void nvdimm_plug(AcpiNVDIMMState *state);
>  void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
>  #endif

[Qemu-devel] [PATCH 10/14] nvdimm acpi: define DSM return codes

[Xiao Guangrong]

and use these codes to refine the code

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 7733f14..c7e7744 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -514,7 +514,13 @@ 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_NOMEMDEV       2 /* Non-Existing Memory Device */
+#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,
@@ -536,7 +542,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
                  read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
 
     if (read_fit->offset > fit->len) {
-        func_ret_status = 3 /* Invalid Input Parameters */;
+        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
         goto exit;
     }
 
@@ -544,11 +550,11 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
     if (!read_fit->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 */;
+    func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
     read_len = MIN(fit->len - read_fit->offset,
                    4096 - sizeof(NvdimmFuncReadFITOut));
 
@@ -577,7 +583,7 @@ static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
         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)
@@ -593,7 +599,7 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
     }
 
     /* No function except function 0 is supported yet. */
-    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
 }
 
 /*
@@ -639,7 +645,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);
 
@@ -650,7 +656,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,
@@ -670,7 +676,7 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
         return ret;
     }
 
-    return 0 /* Success */;
+    return NVDIMM_DSM_RET_STATUS_SUCCESS;
 }
 
 /*
@@ -694,7 +700,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;
     }
@@ -704,7 +710,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);
 
@@ -732,7 +739,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;
     }
@@ -742,7 +749,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)
@@ -766,7 +773,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
     }
 
     if (!nvdimm) {
-        nvdimm_dsm_no_payload(2 /* Non-Existing Memory Device */,
+        nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_NOMEMDEV,
                               dsm_mem_addr);
         return;
     }
@@ -793,7 +800,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
@@ -830,7 +837,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;
     }
 
@@ -1018,7 +1025,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);
 
@@ -1119,7 +1126,8 @@ static void nvdimm_build_fit(Aml *dev)
                                  aml_name(NVDIMM_DSM_RFIT_STATUS)));
 
      /* if something is wrong during _DSM. */
-    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
+    ifcond = aml_equal(aml_int(NVDIMM_DSM_RET_STATUS_SUCCESS),
+                       aml_name("STAU"));
     ifctx = aml_if(aml_lnot(ifcond));
     aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
     aml_append(method, ifctx);
@@ -1156,7 +1164,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);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 10/14] nvdimm acpi: define DSM return codes

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:45PM +0800, Xiao Guangrong wrote:
> and use these codes to refine the code
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 10/14] nvdimm acpi: define DSM return codes

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:45 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> and use these codes to refine the code
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c | 46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 7733f14..c7e7744 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -514,7 +514,13 @@ 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_NOMEMDEV       2 /* Non-Existing Memory Device */
> +#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,
> @@ -536,7 +542,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>                   read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
>  
>      if (read_fit->offset > fit->len) {
> -        func_ret_status = 3 /* Invalid Input Parameters */;
> +        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
>          goto exit;
>      }
>  
> @@ -544,11 +550,11 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>      if (!read_fit->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 */;
> +    func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
>      read_len = MIN(fit->len - read_fit->offset,
>                     4096 - sizeof(NvdimmFuncReadFITOut));
>  
> @@ -577,7 +583,7 @@ static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>          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)
> @@ -593,7 +599,7 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>      }
>  
>      /* No function except function 0 is supported yet. */
> -    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
>  }
>  
>  /*
> @@ -639,7 +645,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);
>  
> @@ -650,7 +656,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,
> @@ -670,7 +676,7 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
>          return ret;
>      }
>  
> -    return 0 /* Success */;
> +    return NVDIMM_DSM_RET_STATUS_SUCCESS;
>  }
>  
>  /*
> @@ -694,7 +700,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;
>      }
> @@ -704,7 +710,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);
>  
> @@ -732,7 +739,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;
>      }
> @@ -742,7 +749,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)
> @@ -766,7 +773,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>      }
>  
>      if (!nvdimm) {
> -        nvdimm_dsm_no_payload(2 /* Non-Existing Memory Device */,
> +        nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_NOMEMDEV,
>                                dsm_mem_addr);
>          return;
>      }
> @@ -793,7 +800,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
> @@ -830,7 +837,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;
>      }
>  
> @@ -1018,7 +1025,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);
>  
> @@ -1119,7 +1126,8 @@ static void nvdimm_build_fit(Aml *dev)
>                                   aml_name(NVDIMM_DSM_RFIT_STATUS)));
>  
>       /* if something is wrong during _DSM. */
> -    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
> +    ifcond = aml_equal(aml_int(NVDIMM_DSM_RET_STATUS_SUCCESS),
> +                       aml_name("STAU"));
>      ifctx = aml_if(aml_lnot(ifcond));
>      aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
>      aml_append(method, ifctx);
> @@ -1156,7 +1164,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);

[Qemu-devel] [PATCH 11/14] nvdimm acpi: fix two comments

[Xiao Guangrong]

fixed the English issue and code-style issue

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

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index c7e7744..6f6f51f 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -479,7 +479,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) +
@@ -578,7 +578,7 @@ 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;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 11/14] nvdimm acpi: fix two comments

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:46PM +0800, Xiao Guangrong wrote:
> fixed the English issue and code-style issue
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 11/14] nvdimm acpi: fix two comments

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:46 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> fixed the English issue and code-style issue
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index c7e7744..6f6f51f 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -479,7 +479,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) +
> @@ -578,7 +578,7 @@ 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;
>      }

[Qemu-devel] [PATCH 12/14] nvdimm acpi: rename nvdimm_dsm_reserved_root

[Xiao Guangrong]

Rename it to nvdimm_dsm_handle_reserved_root_method

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 6f6f51f..6692648 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -571,8 +571,9 @@ exit:
     g_free(read_fit_out);
 }
 
-static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
-                                     hwaddr dsm_mem_addr)
+static void
+nvdimm_dsm_handle_reserved_root_method(AcpiNVDIMMState *state,
+                                       NvdimmDsmIn *in, hwaddr dsm_mem_addr)
 {
     switch (in->function) {
     case 0x0:
@@ -842,7 +843,7 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     }
 
     if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
-        nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
+        nvdimm_dsm_handle_reserved_root_method(state, in, dsm_mem_addr);
         goto exit;
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 12/14] nvdimm acpi: rename nvdimm_dsm_reserved_root

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:47PM +0800, Xiao Guangrong wrote:
> Rename it to nvdimm_dsm_handle_reserved_root_method
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 12/14] nvdimm acpi: rename nvdimm_dsm_reserved_root

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:47 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Rename it to nvdimm_dsm_handle_reserved_root_method
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 6f6f51f..6692648 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -571,8 +571,9 @@ exit:
>      g_free(read_fit_out);
>  }
>  
> -static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> -                                     hwaddr dsm_mem_addr)
> +static void
> +nvdimm_dsm_handle_reserved_root_method(AcpiNVDIMMState *state,
> +                                       NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>  {
>      switch (in->function) {
>      case 0x0:
> @@ -842,7 +843,7 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      }
>  
>      if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> -        nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
> +        nvdimm_dsm_handle_reserved_root_method(state, in, dsm_mem_addr);
>          goto exit;
>      }
>  

[Qemu-devel] [PATCH 13/14] nvdimm acpi: use aml_name_decl to define named object

[Xiao Guangrong]

to make the code more clearer

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 6692648..5f48b75 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1098,13 +1098,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);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 13/14] nvdimm acpi: use aml_name_decl to define named object

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:48PM +0800, Xiao Guangrong wrote:
> to make the code more clearer
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 13/14] nvdimm acpi: use aml_name_decl to define named object

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:48 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> to make the code more clearer
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 6692648..5f48b75 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1098,13 +1098,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);

[Qemu-devel] [PATCH 14/14] nvdimm acpi: introduce NVDIMM_DSM_MEMORY_SIZE

[Xiao Guangrong]

and use it to replace the raw number

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5f48b75..8e7d6ec 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -404,6 +404,8 @@ static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
                  sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
 }
 
+#define NVDIMM_DSM_MEMORY_SIZE      4096
+
 struct NvdimmDsmIn {
     uint32_t handle;
     uint32_t revision;
@@ -414,7 +416,7 @@ struct NvdimmDsmIn {
     };
 } QEMU_PACKED;
 typedef struct NvdimmDsmIn NvdimmDsmIn;
-QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != 4096);
+QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != NVDIMM_DSM_MEMORY_SIZE);
 
 struct NvdimmDsmOut {
     /* the size of buffer filled by QEMU. */
@@ -422,7 +424,7 @@ struct NvdimmDsmOut {
     uint8_t data[4092];
 } QEMU_PACKED;
 typedef struct NvdimmDsmOut NvdimmDsmOut;
-QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmOut) != 4096);
+QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmOut) != NVDIMM_DSM_MEMORY_SIZE);
 
 struct NvdimmDsmFunc0Out {
     /* the size of buffer filled by QEMU. */
@@ -450,7 +452,7 @@ struct NvdimmFuncGetLabelSizeOut {
     uint32_t max_xfer;
 } QEMU_PACKED;
 typedef struct NvdimmFuncGetLabelSizeOut NvdimmFuncGetLabelSizeOut;
-QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelSizeOut) > 4096);
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelSizeOut) > NVDIMM_DSM_MEMORY_SIZE);
 
 struct NvdimmFuncGetLabelDataIn {
     uint32_t offset; /* the offset in the namespace label data area. */
@@ -458,7 +460,7 @@ struct NvdimmFuncGetLabelDataIn {
 } QEMU_PACKED;
 typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
-                  offsetof(NvdimmDsmIn, arg3) > 4096);
+                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
 
 struct NvdimmFuncGetLabelDataOut {
     /* the size of buffer filled by QEMU. */
@@ -467,7 +469,7 @@ struct NvdimmFuncGetLabelDataOut {
     uint8_t out_buf[0]; /* the data got via Get Namesapce Label function. */
 } QEMU_PACKED;
 typedef struct NvdimmFuncGetLabelDataOut NvdimmFuncGetLabelDataOut;
-QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataOut) > 4096);
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataOut) > NVDIMM_DSM_MEMORY_SIZE);
 
 struct NvdimmFuncSetLabelDataIn {
     uint32_t offset; /* the offset in the namespace label data area. */
@@ -476,14 +478,14 @@ struct NvdimmFuncSetLabelDataIn {
 } QEMU_PACKED;
 typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
-                  offsetof(NvdimmDsmIn, arg3) > 4096);
+                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
 
 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);
+                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
 
 struct NvdimmFuncReadFITOut {
     /* the size of buffer filled by QEMU. */
@@ -492,7 +494,7 @@ struct NvdimmFuncReadFITOut {
     uint8_t fit[0]; /* the FIT data. */
 } QEMU_PACKED;
 typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
-QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > NVDIMM_DSM_MEMORY_SIZE);
 
 static void
 nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
@@ -556,7 +558,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
 
     func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
     read_len = MIN(fit->len - read_fit->offset,
-                   4096 - sizeof(NvdimmFuncReadFITOut));
+                   NVDIMM_DSM_MEMORY_SIZE - sizeof(NvdimmFuncReadFITOut));
 
 exit:
     size = sizeof(NvdimmFuncReadFITOut) + read_len;
@@ -610,7 +612,9 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
  */
 static uint32_t nvdimm_get_max_xfer_label_size(void)
 {
-    uint32_t max_get_size, max_set_size, dsm_memory_size = 4096;
+    uint32_t max_get_size, max_set_size, dsm_memory_size;
+
+    dsm_memory_size = NVDIMM_DSM_MEMORY_SIZE;
 
     /*
      * the max data ACPI can read one time which is transferred by
@@ -707,7 +711,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
     }
 
     size = sizeof(*get_label_data_out) + get_label_data->length;
-    assert(size <= 4096);
+    assert(size <= NVDIMM_DSM_MEMORY_SIZE);
     get_label_data_out = g_malloc(size);
 
     get_label_data_out->len = cpu_to_le32(size);
@@ -745,8 +749,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
         return;
     }
 
-    assert(offsetof(NvdimmDsmIn, arg3) +
-           sizeof(*set_label_data) + set_label_data->length <= 4096);
+    assert(offsetof(NvdimmDsmIn, arg3) + sizeof(*set_label_data) +
+                    set_label_data->length <= NVDIMM_DSM_MEMORY_SIZE);
 
     nvc->write_label_data(nvdimm, set_label_data->in_buf,
                           set_label_data->length, set_label_data->offset);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 14/14] nvdimm acpi: introduce NVDIMM_DSM_MEMORY_SIZE

[Stefan Hajnoczi]

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

On Mon, Nov 07, 2016 at 07:13:49PM +0800, Xiao Guangrong wrote:
> and use it to replace the raw number
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 14/14] nvdimm acpi: introduce NVDIMM_DSM_MEMORY_SIZE

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:49 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> and use it to replace the raw number
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 5f48b75..8e7d6ec 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -404,6 +404,8 @@ static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
>                   sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
>  }
>  
> +#define NVDIMM_DSM_MEMORY_SIZE      4096
> +
>  struct NvdimmDsmIn {
>      uint32_t handle;
>      uint32_t revision;
> @@ -414,7 +416,7 @@ struct NvdimmDsmIn {
>      };
>  } QEMU_PACKED;
>  typedef struct NvdimmDsmIn NvdimmDsmIn;
> -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != 4096);
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmDsmOut {
>      /* the size of buffer filled by QEMU. */
> @@ -422,7 +424,7 @@ struct NvdimmDsmOut {
>      uint8_t data[4092];
>  } QEMU_PACKED;
>  typedef struct NvdimmDsmOut NvdimmDsmOut;
> -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmOut) != 4096);
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmOut) != NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmDsmFunc0Out {
>      /* the size of buffer filled by QEMU. */
> @@ -450,7 +452,7 @@ struct NvdimmFuncGetLabelSizeOut {
>      uint32_t max_xfer;
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncGetLabelSizeOut NvdimmFuncGetLabelSizeOut;
> -QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelSizeOut) > 4096);
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelSizeOut) > NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmFuncGetLabelDataIn {
>      uint32_t offset; /* the offset in the namespace label data area. */
> @@ -458,7 +460,7 @@ struct NvdimmFuncGetLabelDataIn {
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
> -                  offsetof(NvdimmDsmIn, arg3) > 4096);
> +                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmFuncGetLabelDataOut {
>      /* the size of buffer filled by QEMU. */
> @@ -467,7 +469,7 @@ struct NvdimmFuncGetLabelDataOut {
>      uint8_t out_buf[0]; /* the data got via Get Namesapce Label function. */
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncGetLabelDataOut NvdimmFuncGetLabelDataOut;
> -QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataOut) > 4096);
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataOut) > NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmFuncSetLabelDataIn {
>      uint32_t offset; /* the offset in the namespace label data area. */
> @@ -476,14 +478,14 @@ struct NvdimmFuncSetLabelDataIn {
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> -                  offsetof(NvdimmDsmIn, arg3) > 4096);
> +                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
>  
>  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);
> +                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmFuncReadFITOut {
>      /* the size of buffer filled by QEMU. */
> @@ -492,7 +494,7 @@ struct NvdimmFuncReadFITOut {
>      uint8_t fit[0]; /* the FIT data. */
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
> -QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > NVDIMM_DSM_MEMORY_SIZE);
>  
>  static void
>  nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
> @@ -556,7 +558,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>  
>      func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
>      read_len = MIN(fit->len - read_fit->offset,
> -                   4096 - sizeof(NvdimmFuncReadFITOut));
> +                   NVDIMM_DSM_MEMORY_SIZE - sizeof(NvdimmFuncReadFITOut));
>  
>  exit:
>      size = sizeof(NvdimmFuncReadFITOut) + read_len;
> @@ -610,7 +612,9 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>   */
>  static uint32_t nvdimm_get_max_xfer_label_size(void)
>  {
> -    uint32_t max_get_size, max_set_size, dsm_memory_size = 4096;
> +    uint32_t max_get_size, max_set_size, dsm_memory_size;
> +
> +    dsm_memory_size = NVDIMM_DSM_MEMORY_SIZE;
>  
>      /*
>       * the max data ACPI can read one time which is transferred by
> @@ -707,7 +711,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>      }
>  
>      size = sizeof(*get_label_data_out) + get_label_data->length;
> -    assert(size <= 4096);
> +    assert(size <= NVDIMM_DSM_MEMORY_SIZE);
>      get_label_data_out = g_malloc(size);
>  
>      get_label_data_out->len = cpu_to_le32(size);
> @@ -745,8 +749,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>          return;
>      }
>  
> -    assert(offsetof(NvdimmDsmIn, arg3) +
> -           sizeof(*set_label_data) + set_label_data->length <= 4096);
> +    assert(offsetof(NvdimmDsmIn, arg3) + sizeof(*set_label_data) +
> +                    set_label_data->length <= NVDIMM_DSM_MEMORY_SIZE);
>  
>      nvc->write_label_data(nvdimm, set_label_data->in_buf,
>                            set_label_data->length, set_label_data->offset);