[PATCH 0/8] nvdimm: hotplug support

[PATCH 0/8] nvdimm: hotplug support

[Xiao Guangrong]

This patchset is against commit 269fe4c3ab0c (vmw_pvscsi: remove unnecessary
internal msi state flag) on pci branch of Michael's git tree and can be found
at:
      https://github.com/xiaogr/qemu.git nvdimm-hotplug-v1

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

and unplug it as follows:
device_del nvdimm3
object_del mem3

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

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

-- 
1.8.3.1

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

[Xiao Guangrong]

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

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

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

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

[Xiao Guangrong]

For each NVDIMM present or intended to be supported by platform,
platform firmware also exposes an ACPI Namespace Device under
the root device

So it builds nvdimm devices for all slots to support vNVDIMM hotplug

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c        | 41 ++++++++++++++++++++++++-----------------
 hw/i386/acpi-build.c    |  2 +-
 include/hw/mem/nvdimm.h |  3 ++-
 3 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5454c0f..0e2b9f0 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -886,12 +886,11 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
     aml_append(dev, method);
 }
 
-static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
+static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
 {
-    for (; device_list; device_list = device_list->next) {
-        DeviceState *dev = device_list->data;
-        int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
-                                           NULL);
+    uint32_t slot;
+
+    for (slot = 0; slot < ram_slots; slot++) {
         uint32_t handle = nvdimm_slot_to_handle(slot);
         Aml *nvdimm_dev;
 
@@ -912,9 +911,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
     }
 }
 
-static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
-                              GArray *table_data, BIOSLinker *linker,
-                              GArray *dsm_dma_arrea)
+static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
+                              BIOSLinker *linker, GArray *dsm_dma_arrea,
+                              uint32_t ram_slots)
 {
     Aml *ssdt, *sb_scope, *dev, *field;
     int mem_addr_offset, nvdimm_ssdt;
@@ -1003,7 +1002,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
     /* 0 is reserved for root device. */
     nvdimm_build_device_dsm(dev, 0);
 
-    nvdimm_build_nvdimm_devices(device_list, dev);
+    nvdimm_build_nvdimm_devices(dev, ram_slots);
 
     aml_append(sb_scope, dev);
     aml_append(ssdt, sb_scope);
@@ -1028,17 +1027,25 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
 }
 
 void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
-                       BIOSLinker *linker, GArray *dsm_dma_arrea)
+                       BIOSLinker *linker, GArray *dsm_dma_arrea,
+                       uint32_t ram_slots)
 {
     GSList *device_list;
 
-    /* no NVDIMM device is plugged. */
     device_list = nvdimm_get_plugged_device_list();
-    if (!device_list) {
-        return;
+
+    /* NVDIMM device is plugged. */
+    if (device_list) {
+        nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
+        g_slist_free(device_list);
+    }
+
+    /*
+     * NVDIMM device is allowed to be plugged only if there has available
+     * slot.
+     */
+    if (ram_slots) {
+        nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
+                          ram_slots);
     }
-    nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
-    nvdimm_build_ssdt(device_list, table_offsets, table_data, linker,
-                      dsm_dma_arrea);
-    g_slist_free(device_list);
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fbba461..de4a603 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2632,7 +2632,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     }
     if (pcms->acpi_nvdimm_state.is_enabled) {
         nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
-                          pcms->acpi_nvdimm_state.dsm_mem);
+                          pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots);
     }
 
     /* Add tables supplied by user (if any) */
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 1cfe9e0..63a2b20 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -112,5 +112,6 @@ typedef struct AcpiNVDIMMState AcpiNVDIMMState;
 void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
                             FWCfgState *fw_cfg, Object *owner);
 void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
-                       BIOSLinker *linker, GArray *dsm_dma_arrea);
+                       BIOSLinker *linker, GArray *dsm_dma_arrea,
+                       uint32_t ram_slots);
 #endif
-- 
1.8.3.1

[PATCH 3/8] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

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

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

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

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

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

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

[Xiao Guangrong]

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

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

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

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

[Xiao Guangrong]

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

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

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

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

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

[Xiao Guangrong]

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

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

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

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

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

[Xiao Guangrong]

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

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

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

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

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

[Xiao Guangrong]

Add the specification of Read FIT function

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

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

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

[Igor Mammedov]

On Mon, 11 Jul 2016 21:45:10 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> This patchset is against commit 269fe4c3ab0c (vmw_pvscsi: remove unnecessary
> internal msi state flag) on pci branch of Michael's git tree and can be found
> at:
>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v1
> 
> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> for example, a new nvdimm device can be plugged as follows:
> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> device_add nvdimm,id=nvdimm3,memdev=mem3
> 
> and unplug it as follows:
> device_del nvdimm3
> object_del mem3
Could you fix crash in hmp related to nvdimm?
  https://bugzilla.redhat.com/show_bug.cgi?id=1352769
I've checked and it reproduces on upstream as well.


> 
> Xiao Guangrong (8):
>   acpi nvdimm: fix wrong buffer size returned by DSM method
>   nvdimm acpi: prebuild nvdimm devices for available slots
>   nvdimm acpi: introduce _FIT
>   nvdimm acpi: implement Read FIT function
>   pc-dimm: introduce prepare_unplug() callback
>   pc: memhp: do not export nvdimm's memory via _CRS
>   pc: acpi: memhp: nvdimm hotplug support
>   nvdimm docs: add nvdimm Read FIT function
> 
>  docs/specs/acpi_mem_hotplug.txt  |   4 +-
>  docs/specs/acpi_nvdimm.txt       |  38 +++++++-
>  hw/acpi/ich9.c                   |   3 +
>  hw/acpi/memory_hotplug.c         |  21 +++--
>  hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
>  hw/acpi/piix4.c                  |   3 +
>  hw/i386/acpi-build.c             |  28 +++++-
>  hw/mem/nvdimm.c                  |  12 ++-
>  hw/mem/pc-dimm.c                 |   5 +
>  include/hw/acpi/memory_hotplug.h |   1 +
>  include/hw/acpi/pc-hotplug.h     |   1 +
>  include/hw/mem/nvdimm.h          |   6 +-
>  include/hw/mem/pc-dimm.h         |   1 +
>  13 files changed, 278 insertions(+), 40 deletions(-)
> 

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

[Xiao Guangrong]

On 07/11/2016 10:12 PM, Igor Mammedov wrote:
> On Mon, 11 Jul 2016 21:45:10 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> This patchset is against commit 269fe4c3ab0c (vmw_pvscsi: remove unnecessary
>> internal msi state flag) on pci branch of Michael's git tree and can be found
>> at:
>>        https://github.com/xiaogr/qemu.git nvdimm-hotplug-v1
>>
>> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
>> for example, a new nvdimm device can be plugged as follows:
>> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
>> device_add nvdimm,id=nvdimm3,memdev=mem3
>>
>> and unplug it as follows:
>> device_del nvdimm3
>> object_del mem3
> Could you fix crash in hmp related to nvdimm?
>    https://bugzilla.redhat.com/show_bug.cgi?id=1352769
> I've checked and it reproduces on upstream as well.

I missed this bug report. Sure, i will work on it.

Thanks!

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

[Stefan Hajnoczi]

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

On Mon, Jul 11, 2016 at 09:45:14PM +0800, Xiao Guangrong wrote:
> Read FIT whose function index is 0xFFFFFFFF is reserved by QEMU to read
> the piece of FIT buffer. Please refer to docs/specs/acpi_nvdimm.txt for
> detailed info
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/nvdimm.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 4bbd1e7..d099ef1 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -466,6 +466,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>                    offsetof(NvdimmDsmIn, arg3) > 4096);
>  
> +struct NvdimmFuncReadFITIn {
> +    uint32_t offset; /* the offset of FIT buffer. */
> +} QEMU_PACKED;
> +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> +                  offsetof(NvdimmDsmIn, arg3) > 4096);
> +
> +struct NvdimmFuncReadFITOut {
> +    /* the size of buffer filled by QEMU. */
> +    uint32_t len;
> +    uint32_t func_ret_status; /* return status code. */
> +    uint8_t fit[0]; /* the FIT data. */
> +} QEMU_PACKED;
> +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
> +
>  static void
>  nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
>  {
> @@ -486,6 +502,46 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>  }
>  
> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
> +static void nvdimm_dsm_func_read_fit(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> +{
> +    NvdimmFuncReadFITIn *read_fit;
> +    NvdimmFuncReadFITOut *read_fit_out;
> +    GSList *device_list = nvdimm_get_plugged_device_list();
> +    GArray *fit = nvdimm_build_device_structure(device_list);
> +    uint32_t read_len = 0, func_ret_status;
> +    int left, size;
> +
> +    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> +    le32_to_cpus(&read_fit->offset);
> +
> +    nvdimm_debug("Read FIT: offset %#x FIT size %#x.\n", read_fit->offset,
> +                 fit->len);
> +
> +    left = fit->len - read_fit->offset;
> +    if (left < 0) {

Signed integer overflow leads to memory disclosure in memcpy() below.
The problem occurs when (guint)fit->len - (uint32_t)read_fit->offset >
INT_MAX.

Please perform the check like this:

  if (fit->offset >= fit->len) {

> +        func_ret_status = 3 /* Invalid Input Parameters */;
> +        goto build_out;
> +    }
> +
> +    func_ret_status = 0 /* Success */;
> +    read_len = MIN(left, 4096 - sizeof(NvdimmFuncReadFITOut));
> +
> +build_out:
> +    size = sizeof(NvdimmFuncReadFITOut) + read_len;
> +    read_fit_out = g_malloc(size);
> +
> +    read_fit_out->len = cpu_to_le32(size);
> +    read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
> +    memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
> +
> +    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
> +
> +    g_slist_free(device_list);
> +    g_array_free(fit, true);
> +    g_free(read_fit_out);
> +}
> +
>  static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>  {
>      /*
> @@ -498,6 +554,11 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>          return;
>      }
>  
> +    if (in->function == 0xFFFFFFFF /* Read FIT */) {
> +        nvdimm_dsm_func_read_fit(in, dsm_mem_addr);
> +        return;
> +    }
> +
>      /* No function except function 0 is supported yet. */
>      nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
>  }
> -- 
> 1.8.3.1
> 

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

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

[Stefan Hajnoczi]

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

On Mon, Jul 11, 2016 at 09:45:17PM +0800, Xiao Guangrong wrote:
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 73fa62d..d1c2e92 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -239,11 +239,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
>                           DeviceState *dev, Error **errp)
>  {
>      MemStatus *mdev;
> -    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> -
> -    if (!dc->hotpluggable) {
> -        return;
> -    }
>  
>      mdev = acpi_memory_slot_status(mem_st, dev, errp);
>      if (!mdev) {

Did you mean to include this hunk in the patch?  Looks like something
left over from debugging/prototyping.

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

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

[Stefan Hajnoczi]

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

On Mon, Jul 11, 2016 at 09:45:10PM +0800, Xiao Guangrong wrote:
> This patchset is against commit 269fe4c3ab0c (vmw_pvscsi: remove unnecessary
> internal msi state flag) on pci branch of Michael's git tree and can be found
> at:
>       https://github.com/xiaogr/qemu.git nvdimm-hotplug-v1
> 
> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> for example, a new nvdimm device can be plugged as follows:
> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> device_add nvdimm,id=nvdimm3,memdev=mem3
> 
> and unplug it as follows:
> device_del nvdimm3
> object_del mem3
> 
> Xiao Guangrong (8):
>   acpi nvdimm: fix wrong buffer size returned by DSM method
>   nvdimm acpi: prebuild nvdimm devices for available slots
>   nvdimm acpi: introduce _FIT
>   nvdimm acpi: implement Read FIT function
>   pc-dimm: introduce prepare_unplug() callback
>   pc: memhp: do not export nvdimm's memory via _CRS
>   pc: acpi: memhp: nvdimm hotplug support
>   nvdimm docs: add nvdimm Read FIT function
> 
>  docs/specs/acpi_mem_hotplug.txt  |   4 +-
>  docs/specs/acpi_nvdimm.txt       |  38 +++++++-
>  hw/acpi/ich9.c                   |   3 +
>  hw/acpi/memory_hotplug.c         |  21 +++--
>  hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
>  hw/acpi/piix4.c                  |   3 +
>  hw/i386/acpi-build.c             |  28 +++++-
>  hw/mem/nvdimm.c                  |  12 ++-
>  hw/mem/pc-dimm.c                 |   5 +
>  include/hw/acpi/memory_hotplug.h |   1 +
>  include/hw/acpi/pc-hotplug.h     |   1 +
>  include/hw/mem/nvdimm.h          |   6 +-
>  include/hw/mem/pc-dimm.h         |   1 +
>  13 files changed, 278 insertions(+), 40 deletions(-)

I left comments on two patches.  I'm not very familiar with ACPI and
memory hotplug but I've tried my best to review the series.

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

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

[Xiao Guangrong]

On 07/14/2016 08:17 PM, Stefan Hajnoczi wrote:

>> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
>> +static void nvdimm_dsm_func_read_fit(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>> +{
>> +    NvdimmFuncReadFITIn *read_fit;
>> +    NvdimmFuncReadFITOut *read_fit_out;
>> +    GSList *device_list = nvdimm_get_plugged_device_list();
>> +    GArray *fit = nvdimm_build_device_structure(device_list);
>> +    uint32_t read_len = 0, func_ret_status;
>> +    int left, size;
>> +
>> +    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
>> +    le32_to_cpus(&read_fit->offset);
>> +
>> +    nvdimm_debug("Read FIT: offset %#x FIT size %#x.\n", read_fit->offset,
>> +                 fit->len);
>> +
>> +    left = fit->len - read_fit->offset;
>> +    if (left < 0) {
>
> Signed integer overflow leads to memory disclosure in memcpy() below.
> The problem occurs when (guint)fit->len - (uint32_t)read_fit->offset >
> INT_MAX.
>
> Please perform the check like this:
>
>    if (fit->offset >= fit->len) {
>

Ah, yes, you are right, thank you for pointing it out. Will fix it.

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

[Xiao Guangrong]

On 07/14/2016 08:17 PM, Stefan Hajnoczi wrote:
> On Mon, Jul 11, 2016 at 09:45:17PM +0800, Xiao Guangrong wrote:
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index 73fa62d..d1c2e92 100644
>> --- a/hw/acpi/memory_hotplug.c
>> +++ b/hw/acpi/memory_hotplug.c
>> @@ -239,11 +239,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
>>                            DeviceState *dev, Error **errp)
>>   {
>>       MemStatus *mdev;
>> -    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> -
>> -    if (!dc->hotpluggable) {
>> -        return;
>> -    }
>>
>>       mdev = acpi_memory_slot_status(mem_st, dev, errp);
>>       if (!mdev) {
>
> Did you mean to include this hunk in the patch?  Looks like something
> left over from debugging/prototyping.
>

As all memory devices, both pc-dimm and nvdimm, have supported hotplug now, i
think this chunk can be removed.

In fact, this check was introduced by nvdimm. :)

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

[Xiao Guangrong]

On 07/14/2016 08:17 PM, Stefan Hajnoczi wrote:
> On Mon, Jul 11, 2016 at 09:45:10PM +0800, Xiao Guangrong wrote:
>> This patchset is against commit 269fe4c3ab0c (vmw_pvscsi: remove unnecessary
>> internal msi state flag) on pci branch of Michael's git tree and can be found
>> at:
>>        https://github.com/xiaogr/qemu.git nvdimm-hotplug-v1
>>
>> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
>> for example, a new nvdimm device can be plugged as follows:
>> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
>> device_add nvdimm,id=nvdimm3,memdev=mem3
>>
>> and unplug it as follows:
>> device_del nvdimm3
>> object_del mem3
>>
>> Xiao Guangrong (8):
>>    acpi nvdimm: fix wrong buffer size returned by DSM method
>>    nvdimm acpi: prebuild nvdimm devices for available slots
>>    nvdimm acpi: introduce _FIT
>>    nvdimm acpi: implement Read FIT function
>>    pc-dimm: introduce prepare_unplug() callback
>>    pc: memhp: do not export nvdimm's memory via _CRS
>>    pc: acpi: memhp: nvdimm hotplug support
>>    nvdimm docs: add nvdimm Read FIT function
>>
>>   docs/specs/acpi_mem_hotplug.txt  |   4 +-
>>   docs/specs/acpi_nvdimm.txt       |  38 +++++++-
>>   hw/acpi/ich9.c                   |   3 +
>>   hw/acpi/memory_hotplug.c         |  21 +++--
>>   hw/acpi/nvdimm.c                 | 195 +++++++++++++++++++++++++++++++++++----
>>   hw/acpi/piix4.c                  |   3 +
>>   hw/i386/acpi-build.c             |  28 +++++-
>>   hw/mem/nvdimm.c                  |  12 ++-
>>   hw/mem/pc-dimm.c                 |   5 +
>>   include/hw/acpi/memory_hotplug.h |   1 +
>>   include/hw/acpi/pc-hotplug.h     |   1 +
>>   include/hw/mem/nvdimm.h          |   6 +-
>>   include/hw/mem/pc-dimm.h         |   1 +
>>   13 files changed, 278 insertions(+), 40 deletions(-)
>
> I left comments on two patches.  I'm not very familiar with ACPI and
> memory hotplug but I've tried my best to review the series.
>

You are good enough. :)

I really appreciate your great efforts to continuously review and push nvdimm's
patches.

Thanks!

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

[Stefan Hajnoczi]

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

On Fri, Jul 15, 2016 at 03:49:12PM +0800, Xiao Guangrong wrote:
> 
> 
> On 07/14/2016 08:17 PM, Stefan Hajnoczi wrote:
> > On Mon, Jul 11, 2016 at 09:45:17PM +0800, Xiao Guangrong wrote:
> > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > > index 73fa62d..d1c2e92 100644
> > > --- a/hw/acpi/memory_hotplug.c
> > > +++ b/hw/acpi/memory_hotplug.c
> > > @@ -239,11 +239,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
> > >                            DeviceState *dev, Error **errp)
> > >   {
> > >       MemStatus *mdev;
> > > -    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > > -
> > > -    if (!dc->hotpluggable) {
> > > -        return;
> > > -    }
> > > 
> > >       mdev = acpi_memory_slot_status(mem_st, dev, errp);
> > >       if (!mdev) {
> > 
> > Did you mean to include this hunk in the patch?  Looks like something
> > left over from debugging/prototyping.
> > 
> 
> As all memory devices, both pc-dimm and nvdimm, have supported hotplug now, i
> think this chunk can be removed.
> 
> In fact, this check was introduced by nvdimm. :)

Fair enough.

Stefan

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