* [PATCH v3 0/4] nvdimm: hotplug support
@ 2016-10-28 16:35 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, Xiao Guangrong
It is based on my previous patchset,
"[PATCH 0/8] nvdimm acpi: bug fix and cleanup", these two patchset are
against commit dea651a95af6dad099 (intel-iommu: Check IOAPIC's Trigger Mode
against the one in IRTE) on pci branch of Michael's git tree and can be
found at:
https://github.com/xiaogr/qemu.git nvdimm-hotplug-v3
Changelog in v3:
1) use a dedicated interrupt for nvdimm device hotplug
2) stop nvdimm device hot unplug
3) reserve UUID and handle for QEMU internally used QEMU
5) redesign fit buffer to avoid OSPM reading incomplete fit info
6) bug fixes and cleanups
Changelog in v2:
Fixed signed integer overflow pointed out by Stefan Hajnoczi
This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
for example, a new nvdimm device can be plugged as follows:
object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
device_add nvdimm,id=nvdimm3,memdev=mem3
and unplug it as follows:
device_del nvdimm3
object_del mem3
Xiao Guangrong (4):
nvdimm acpi: prebuild nvdimm devices for available slots
nvdimm acpi: introduce fit buffer
nvdimm acpi: introduce _FIT
pc: memhp: enable nvdimm device hotplug
docs/specs/acpi_mem_hotplug.txt | 3 +
docs/specs/acpi_nvdimm.txt | 58 ++++++-
hw/acpi/memory_hotplug.c | 31 +++-
hw/acpi/nvdimm.c | 286 +++++++++++++++++++++++++++++++----
hw/core/hotplug.c | 11 ++
hw/core/qdev.c | 20 ++-
hw/i386/acpi-build.c | 9 +-
hw/i386/pc.c | 31 ++++
hw/mem/nvdimm.c | 4 -
include/hw/acpi/acpi_dev_interface.h | 1 +
include/hw/hotplug.h | 10 ++
include/hw/mem/nvdimm.h | 27 +++-
12 files changed, 443 insertions(+), 48 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v3 0/4] nvdimm: hotplug support
@ 2016-10-28 16:35 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, Xiao Guangrong
It is based on my previous patchset,
"[PATCH 0/8] nvdimm acpi: bug fix and cleanup", these two patchset are
against commit dea651a95af6dad099 (intel-iommu: Check IOAPIC's Trigger Mode
against the one in IRTE) on pci branch of Michael's git tree and can be
found at:
https://github.com/xiaogr/qemu.git nvdimm-hotplug-v3
Changelog in v3:
1) use a dedicated interrupt for nvdimm device hotplug
2) stop nvdimm device hot unplug
3) reserve UUID and handle for QEMU internally used QEMU
5) redesign fit buffer to avoid OSPM reading incomplete fit info
6) bug fixes and cleanups
Changelog in v2:
Fixed signed integer overflow pointed out by Stefan Hajnoczi
This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
for example, a new nvdimm device can be plugged as follows:
object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
device_add nvdimm,id=nvdimm3,memdev=mem3
and unplug it as follows:
device_del nvdimm3
object_del mem3
Xiao Guangrong (4):
nvdimm acpi: prebuild nvdimm devices for available slots
nvdimm acpi: introduce fit buffer
nvdimm acpi: introduce _FIT
pc: memhp: enable nvdimm device hotplug
docs/specs/acpi_mem_hotplug.txt | 3 +
docs/specs/acpi_nvdimm.txt | 58 ++++++-
hw/acpi/memory_hotplug.c | 31 +++-
hw/acpi/nvdimm.c | 286 +++++++++++++++++++++++++++++++----
hw/core/hotplug.c | 11 ++
hw/core/qdev.c | 20 ++-
hw/i386/acpi-build.c | 9 +-
hw/i386/pc.c | 31 ++++
hw/mem/nvdimm.c | 4 -
include/hw/acpi/acpi_dev_interface.h | 1 +
include/hw/hotplug.h | 10 ++
include/hw/mem/nvdimm.h | 27 +++-
12 files changed, 443 insertions(+), 48 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 1/4] nvdimm acpi: prebuild nvdimm devices for available slots
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-10-28 16:35 ` Xiao Guangrong
-1 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, 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
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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 bb896c9..b8a2e62 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -961,12 +961,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;
@@ -987,9 +986,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;
int mem_addr_offset, nvdimm_ssdt;
@@ -1021,7 +1020,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);
@@ -1046,17 +1045,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 is 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 e999654..6ae4769 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
}
if (pcms->acpi_nvdimm_state.is_enabled) {
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
- pcms->acpi_nvdimm_state.dsm_mem);
+ 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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v3 1/4] nvdimm acpi: prebuild nvdimm devices for available slots
@ 2016-10-28 16:35 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, 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
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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 bb896c9..b8a2e62 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -961,12 +961,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;
@@ -987,9 +986,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;
int mem_addr_offset, nvdimm_ssdt;
@@ -1021,7 +1020,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);
@@ -1046,17 +1045,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 is 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 e999654..6ae4769 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
}
if (pcms->acpi_nvdimm_state.is_enabled) {
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
- pcms->acpi_nvdimm_state.dsm_mem);
+ 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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-10-28 16:35 ` Xiao Guangrong
-1 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, Xiao Guangrong
The buffer is used to save the FIT info for all the presented nvdimm
devices which is updated after the nvdimm device is plugged or
unplugged. In the later patch, it will be used to construct NVDIMM
ACPI _FIT method which reflects the presented nvdimm devices after
nvdimm hotplug
As FIT buffer can not completely mapped into guest address space,
OSPM will exit to QEMU multiple times, however, there is the race
condition - FIT may be changed during these multiple exits, so that
some rules are introduced:
1) the user should hold the @lock to access the buffer and
2) mark @dirty whenever the buffer is updated.
@dirty is cleared for the first time OSPM gets fit buffer, if
dirty is detected in the later access, OSPM will restart the
access
As fit should be updated after nvdimm device is successfully realized
so that a new hotplug callback, post_hotplug, is introduced
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
hw/acpi/nvdimm.c | 59 +++++++++++++++++++++++++++++++++++--------------
hw/core/hotplug.c | 11 +++++++++
hw/core/qdev.c | 20 +++++++++++++----
hw/i386/acpi-build.c | 2 +-
hw/i386/pc.c | 19 ++++++++++++++++
include/hw/hotplug.h | 10 +++++++++
include/hw/mem/nvdimm.h | 26 +++++++++++++++++++++-
7 files changed, 124 insertions(+), 23 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index b8a2e62..5f728a6 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -348,8 +348,9 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
(DSM) in DSM Spec Rev1.*/);
}
-static GArray *nvdimm_build_device_structure(GSList *device_list)
+static GArray *nvdimm_build_device_structure(void)
{
+ GSList *device_list = nvdimm_get_plugged_device_list();
GArray *structures = g_array_new(false, true /* clear */, 1);
for (; device_list; device_list = device_list->next) {
@@ -367,28 +368,58 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
/* build NVDIMM Control Region Structure. */
nvdimm_build_structure_dcr(structures, dev);
}
+ g_slist_free(device_list);
return structures;
}
-static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
+static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
+{
+ 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)
+{
+ nvdimm_build_fit_buffer(&state->fit_buf);
+}
+
+static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
GArray *table_data, BIOSLinker *linker)
{
- GArray *structures = nvdimm_build_device_structure(device_list);
+ NvdimmFitBuffer *fit_buf = &state->fit_buf;
unsigned int header;
+ qemu_mutex_lock(&fit_buf->lock);
+
+ /* NVDIMM device is not plugged? */
+ if (!fit_buf->fit->len) {
+ goto exit;
+ }
+
acpi_add_table(table_offsets, table_data);
/* NFIT header. */
header = table_data->len;
acpi_data_push(table_data, sizeof(NvdimmNfitHeader));
/* NVDIMM device structures. */
- g_array_append_vals(table_data, structures->data, structures->len);
+ g_array_append_vals(table_data, fit_buf->fit->data, fit_buf->fit->len);
build_header(linker, table_data,
(void *)(table_data->data + header), "NFIT",
- sizeof(NvdimmNfitHeader) + structures->len, 1, NULL, NULL);
- g_array_free(structures, true);
+ sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
+
+exit:
+ qemu_mutex_unlock(&fit_buf->lock);
}
struct NvdimmDsmIn {
@@ -771,6 +802,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
state->dsm_mem->len);
+
+ nvdimm_init_fit_buffer(&state->fit_buf);
}
#define NVDIMM_COMMON_DSM "NCAL"
@@ -1045,25 +1078,17 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
}
void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
- BIOSLinker *linker, GArray *dsm_dma_arrea,
+ BIOSLinker *linker, AcpiNVDIMMState *state,
uint32_t ram_slots)
{
- GSList *device_list;
-
- device_list = nvdimm_get_plugged_device_list();
-
- /* NVDIMM device is plugged. */
- if (device_list) {
- nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
- g_slist_free(device_list);
- }
+ nvdimm_build_nfit(state, table_offsets, table_data, linker);
/*
* NVDIMM device is allowed to be plugged only if there is available
* slot.
*/
if (ram_slots) {
- nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
+ nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
ram_slots);
}
}
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986..ab34c19 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -35,6 +35,17 @@ 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 5783442..d835e62 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -945,10 +945,21 @@ 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) {
@@ -965,13 +976,14 @@ 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;
+ if (local_err != NULL) {
+ goto fail;
+ }
+
+ dev->realized = value;
}
- dev->realized = value;
return;
child_realize_fail:
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6ae4769..bc49958 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
}
if (pcms->acpi_nvdimm_state.is_enabled) {
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
- pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots);
+ &pcms->acpi_nvdimm_state, machine->ram_slots);
}
/* Add tables supplied by user (if any) */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 93ff49c..3be6304 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1706,6 +1706,16 @@ 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)
{
@@ -1966,6 +1976,14 @@ 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)
{
@@ -2300,6 +2318,7 @@ 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 c0db869..10ca5b6 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -47,6 +47,7 @@ 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.
@@ -61,6 +62,7 @@ typedef struct HotplugHandlerClass {
/* <public> */
hotplug_fn pre_plug;
hotplug_fn plug;
+ hotplug_fn post_plug;
hotplug_fn unplug_request;
hotplug_fn unplug;
} HotplugHandlerClass;
@@ -83,6 +85,14 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
DeviceState *plugged_dev,
Error **errp);
+/**
+ * hotplug_handler_post_plug:
+ *
+ * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
+ */
+void hotplug_handler_post_plug(HotplugHandler *plug_handler,
+ DeviceState *plugged_dev,
+ Error **errp);
/**
* hotplug_handler_unplug_request:
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 63a2b20..33cd421 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -98,12 +98,35 @@ typedef struct NVDIMMClass NVDIMMClass;
#define NVDIMM_ACPI_IO_BASE 0x0a18
#define NVDIMM_ACPI_IO_LEN 4
+/*
+ * The buffer, @fit, saves the FIT info for all the presented NVDIMM
+ * devices which is updated after the NVDIMM device is plugged or
+ * unplugged.
+ *
+ * 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.
+ */
+struct NvdimmFitBuffer {
+ QemuMutex lock;
+ GArray *fit;
+ bool dirty;
+};
+typedef struct NvdimmFitBuffer NvdimmFitBuffer;
+
struct AcpiNVDIMMState {
/* detect if NVDIMM support is enabled. */
bool is_enabled;
/* the data of the fw_cfg file NVDIMM_DSM_MEM_FILE. */
GArray *dsm_mem;
+
+ NvdimmFitBuffer fit_buf;
+
/* the IO region used by OSPM to transfer control to QEMU. */
MemoryRegion io_mr;
};
@@ -112,6 +135,7 @@ typedef struct AcpiNVDIMMState AcpiNVDIMMState;
void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
FWCfgState *fw_cfg, Object *owner);
void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
- BIOSLinker *linker, GArray *dsm_dma_arrea,
+ BIOSLinker *linker, AcpiNVDIMMState *state,
uint32_t ram_slots);
+void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
@ 2016-10-28 16:35 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, Xiao Guangrong
The buffer is used to save the FIT info for all the presented nvdimm
devices which is updated after the nvdimm device is plugged or
unplugged. In the later patch, it will be used to construct NVDIMM
ACPI _FIT method which reflects the presented nvdimm devices after
nvdimm hotplug
As FIT buffer can not completely mapped into guest address space,
OSPM will exit to QEMU multiple times, however, there is the race
condition - FIT may be changed during these multiple exits, so that
some rules are introduced:
1) the user should hold the @lock to access the buffer and
2) mark @dirty whenever the buffer is updated.
@dirty is cleared for the first time OSPM gets fit buffer, if
dirty is detected in the later access, OSPM will restart the
access
As fit should be updated after nvdimm device is successfully realized
so that a new hotplug callback, post_hotplug, is introduced
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
hw/acpi/nvdimm.c | 59 +++++++++++++++++++++++++++++++++++--------------
hw/core/hotplug.c | 11 +++++++++
hw/core/qdev.c | 20 +++++++++++++----
hw/i386/acpi-build.c | 2 +-
hw/i386/pc.c | 19 ++++++++++++++++
include/hw/hotplug.h | 10 +++++++++
include/hw/mem/nvdimm.h | 26 +++++++++++++++++++++-
7 files changed, 124 insertions(+), 23 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index b8a2e62..5f728a6 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -348,8 +348,9 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
(DSM) in DSM Spec Rev1.*/);
}
-static GArray *nvdimm_build_device_structure(GSList *device_list)
+static GArray *nvdimm_build_device_structure(void)
{
+ GSList *device_list = nvdimm_get_plugged_device_list();
GArray *structures = g_array_new(false, true /* clear */, 1);
for (; device_list; device_list = device_list->next) {
@@ -367,28 +368,58 @@ static GArray *nvdimm_build_device_structure(GSList *device_list)
/* build NVDIMM Control Region Structure. */
nvdimm_build_structure_dcr(structures, dev);
}
+ g_slist_free(device_list);
return structures;
}
-static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
+static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
+{
+ 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)
+{
+ nvdimm_build_fit_buffer(&state->fit_buf);
+}
+
+static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
GArray *table_data, BIOSLinker *linker)
{
- GArray *structures = nvdimm_build_device_structure(device_list);
+ NvdimmFitBuffer *fit_buf = &state->fit_buf;
unsigned int header;
+ qemu_mutex_lock(&fit_buf->lock);
+
+ /* NVDIMM device is not plugged? */
+ if (!fit_buf->fit->len) {
+ goto exit;
+ }
+
acpi_add_table(table_offsets, table_data);
/* NFIT header. */
header = table_data->len;
acpi_data_push(table_data, sizeof(NvdimmNfitHeader));
/* NVDIMM device structures. */
- g_array_append_vals(table_data, structures->data, structures->len);
+ g_array_append_vals(table_data, fit_buf->fit->data, fit_buf->fit->len);
build_header(linker, table_data,
(void *)(table_data->data + header), "NFIT",
- sizeof(NvdimmNfitHeader) + structures->len, 1, NULL, NULL);
- g_array_free(structures, true);
+ sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL, NULL);
+
+exit:
+ qemu_mutex_unlock(&fit_buf->lock);
}
struct NvdimmDsmIn {
@@ -771,6 +802,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
state->dsm_mem->len);
+
+ nvdimm_init_fit_buffer(&state->fit_buf);
}
#define NVDIMM_COMMON_DSM "NCAL"
@@ -1045,25 +1078,17 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
}
void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
- BIOSLinker *linker, GArray *dsm_dma_arrea,
+ BIOSLinker *linker, AcpiNVDIMMState *state,
uint32_t ram_slots)
{
- GSList *device_list;
-
- device_list = nvdimm_get_plugged_device_list();
-
- /* NVDIMM device is plugged. */
- if (device_list) {
- nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
- g_slist_free(device_list);
- }
+ nvdimm_build_nfit(state, table_offsets, table_data, linker);
/*
* NVDIMM device is allowed to be plugged only if there is available
* slot.
*/
if (ram_slots) {
- nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
+ nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
ram_slots);
}
}
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986..ab34c19 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -35,6 +35,17 @@ 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 5783442..d835e62 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -945,10 +945,21 @@ 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) {
@@ -965,13 +976,14 @@ 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;
+ if (local_err != NULL) {
+ goto fail;
+ }
+
+ dev->realized = value;
}
- dev->realized = value;
return;
child_realize_fail:
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6ae4769..bc49958 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
}
if (pcms->acpi_nvdimm_state.is_enabled) {
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
- pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots);
+ &pcms->acpi_nvdimm_state, machine->ram_slots);
}
/* Add tables supplied by user (if any) */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 93ff49c..3be6304 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1706,6 +1706,16 @@ 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)
{
@@ -1966,6 +1976,14 @@ 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)
{
@@ -2300,6 +2318,7 @@ 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 c0db869..10ca5b6 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -47,6 +47,7 @@ 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.
@@ -61,6 +62,7 @@ typedef struct HotplugHandlerClass {
/* <public> */
hotplug_fn pre_plug;
hotplug_fn plug;
+ hotplug_fn post_plug;
hotplug_fn unplug_request;
hotplug_fn unplug;
} HotplugHandlerClass;
@@ -83,6 +85,14 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
DeviceState *plugged_dev,
Error **errp);
+/**
+ * hotplug_handler_post_plug:
+ *
+ * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
+ */
+void hotplug_handler_post_plug(HotplugHandler *plug_handler,
+ DeviceState *plugged_dev,
+ Error **errp);
/**
* hotplug_handler_unplug_request:
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 63a2b20..33cd421 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -98,12 +98,35 @@ typedef struct NVDIMMClass NVDIMMClass;
#define NVDIMM_ACPI_IO_BASE 0x0a18
#define NVDIMM_ACPI_IO_LEN 4
+/*
+ * The buffer, @fit, saves the FIT info for all the presented NVDIMM
+ * devices which is updated after the NVDIMM device is plugged or
+ * unplugged.
+ *
+ * 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.
+ */
+struct NvdimmFitBuffer {
+ QemuMutex lock;
+ GArray *fit;
+ bool dirty;
+};
+typedef struct NvdimmFitBuffer NvdimmFitBuffer;
+
struct AcpiNVDIMMState {
/* detect if NVDIMM support is enabled. */
bool is_enabled;
/* the data of the fw_cfg file NVDIMM_DSM_MEM_FILE. */
GArray *dsm_mem;
+
+ NvdimmFitBuffer fit_buf;
+
/* the IO region used by OSPM to transfer control to QEMU. */
MemoryRegion io_mr;
};
@@ -112,6 +135,7 @@ typedef struct AcpiNVDIMMState AcpiNVDIMMState;
void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
FWCfgState *fw_cfg, Object *owner);
void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
- BIOSLinker *linker, GArray *dsm_dma_arrea,
+ BIOSLinker *linker, AcpiNVDIMMState *state,
uint32_t ram_slots);
+void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-10-28 16:35 ` Xiao Guangrong
-1 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, Xiao Guangrong
_FIT is required for hotplug support, guest will inquire the updated
device info from it if a hotplug event is received
As FIT buffer is not completely mapped into guest address space, so a
new function, Read FIT whose UUID is UUID
648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
is concatenated before _FIT return
Refer to docs/specs/acpi-nvdimm.txt for detailed design
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
docs/specs/acpi_nvdimm.txt | 58 ++++++++++++-
hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 257 insertions(+), 5 deletions(-)
diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 0fdd251..4aa5e3d 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -127,6 +127,58 @@ _DSM process diagram:
| result from the page | | |
+--------------------------+ +--------------+
- _FIT implementation
- -------------------
- TODO (will fill it when nvdimm hotplug is introduced)
+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.
+
+ Input parameters:
+ Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
+ Arg1 – Revision ID (set to 1)
+ Arg2 - Function Index, 0x1
+ Arg3 - A package containing a buffer whose layout is as follows:
+
+ +----------+-------------+-------------+-----------------------------------+
+ | 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).
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5f728a6..fc1a012 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -496,6 +496,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)
{
@@ -516,6 +532,74 @@ 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
+
+/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
+static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+ hwaddr dsm_mem_addr)
+{
+ NvdimmFitBuffer *fit_buf = &state->fit_buf;
+ NvdimmFuncReadFITIn *read_fit;
+ NvdimmFuncReadFITOut *read_fit_out;
+ GArray *fit;
+ uint32_t read_len = 0, func_ret_status;
+ int size;
+
+ 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",
+ read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
+
+ if (read_fit->offset > fit->len) {
+ func_ret_status = 3 /* Invalid Input Parameters */;
+ goto exit;
+ }
+
+ /* It is the first time to read FIT. */
+ 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 */;
+ goto exit;
+ }
+
+ func_ret_status = 0 /* Success */;
+ read_len = MIN(fit->len - read_fit->offset,
+ 4096 - sizeof(NvdimmFuncReadFITOut));
+
+exit:
+ size = sizeof(NvdimmFuncReadFITOut) + read_len;
+ read_fit_out = g_malloc(size);
+
+ read_fit_out->len = cpu_to_le32(size);
+ read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
+ memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
+
+ cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
+
+ g_free(read_fit_out);
+ qemu_mutex_unlock(&fit_buf->lock);
+}
+
+static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+ hwaddr dsm_mem_addr)
+{
+ switch (in->function) {
+ case 0x0:
+ nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
+ return;
+ case 0x1 /*Read FIT */:
+ nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
+ return;
+ }
+
+ nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+}
+
static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
{
/*
@@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
static void
nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
{
+ AcpiNVDIMMState *state = opaque;
NvdimmDsmIn *in;
hwaddr dsm_mem_addr = val;
@@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
goto exit;
}
+ if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
+ nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
+ goto exit;
+ }
+
/* Handle 0 is reserved for NVDIMM Root Device. */
if (!in->handle) {
nvdimm_dsm_root(in, dsm_mem_addr);
@@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
#define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
#define NVDIMM_DSM_OUT_BUF "ODAT"
+#define NVDIMM_DSM_RFIT_STATUS "RSTA"
+
+#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
+
static void nvdimm_build_common_dsm(Aml *dev)
{
- Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
+ Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
uint8_t byte_list[1];
@@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev)
/* UUID for NVDIMM Root Device */, expected_uuid));
aml_append(method, ifctx);
elsectx = aml_else();
- aml_append(elsectx, aml_store(
+ ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
+ aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
+ /* UUID for QEMU internal use */), expected_uuid));
+ aml_append(elsectx, ifctx);
+ elsectx2 = aml_else();
+ aml_append(elsectx2, aml_store(
aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
/* UUID for NVDIMM Devices */, expected_uuid));
+ aml_append(elsectx, elsectx2);
aml_append(method, elsectx);
uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
@@ -994,6 +1094,105 @@ 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, *elsectx, *fit;
+
+ buf = aml_local(0);
+ 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));
+
+ /* 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"));
+
+ /* prepare input package. */
+ pkg = aml_package(1);
+ aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
+ aml_append(pkg, aml_name("OFST"));
+
+ /* call Read_FIT function. */
+ call_result = aml_call5(NVDIMM_COMMON_DSM,
+ aml_touuid(NVDIMM_QEMU_RSVD_UUID),
+ aml_int(1) /* Revision 1 */,
+ aml_int(0x1) /* Read FIT */,
+ pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
+ aml_append(method, aml_store(call_result, buf));
+
+ /* handle _DSM result. */
+ aml_append(method, aml_create_dword_field(buf,
+ aml_int(0) /* offset at byte 0 */, "STAU"));
+
+ aml_append(method, aml_store(aml_name("STAU"),
+ aml_name(NVDIMM_DSM_RFIT_STATUS)));
+
+ /* if something is wrong during _DSM. */
+ ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
+ ifctx = aml_if(aml_lnot(ifcond));
+ aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
+ aml_append(method, ifctx);
+
+ aml_append(method, aml_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_SERIALIZED);
+ offset = aml_local(3);
+
+ aml_append(method, aml_store(aml_buffer(0, NULL), fit));
+ aml_append(method, aml_store(aml_int(0), offset));
+
+ whilectx = aml_while(aml_int(1));
+ aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
+ aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
+
+ /*
+ * if fit buffer was changed during RFIT, read from the beginning
+ * again.
+ */
+ ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
+ aml_int(0x100 /* fit changed */)));
+ aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
+ aml_append(ifctx, aml_store(aml_int(0), offset));
+ aml_append(whilectx, ifctx);
+
+ elsectx = aml_else();
+
+ /* finish fit read if no data is read out. */
+ ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+ aml_append(ifctx, aml_return(fit));
+ aml_append(elsectx, ifctx);
+
+ /* update the offset. */
+ aml_append(elsectx, aml_add(offset, buf_size, offset));
+ /* append the data we read out to the fit buffer. */
+ aml_append(elsectx, aml_concatenate(fit, buf, fit));
+ aml_append(whilectx, elsectx);
+ aml_append(method, whilectx);
+
+ aml_append(dev, method);
+}
+
static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
{
uint32_t slot;
@@ -1052,6 +1251,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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
@ 2016-10-28 16:35 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, Xiao Guangrong
_FIT is required for hotplug support, guest will inquire the updated
device info from it if a hotplug event is received
As FIT buffer is not completely mapped into guest address space, so a
new function, Read FIT whose UUID is UUID
648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
is concatenated before _FIT return
Refer to docs/specs/acpi-nvdimm.txt for detailed design
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
docs/specs/acpi_nvdimm.txt | 58 ++++++++++++-
hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 257 insertions(+), 5 deletions(-)
diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 0fdd251..4aa5e3d 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -127,6 +127,58 @@ _DSM process diagram:
| result from the page | | |
+--------------------------+ +--------------+
- _FIT implementation
- -------------------
- TODO (will fill it when nvdimm hotplug is introduced)
+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.
+
+ Input parameters:
+ Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
+ Arg1 – Revision ID (set to 1)
+ Arg2 - Function Index, 0x1
+ Arg3 - A package containing a buffer whose layout is as follows:
+
+ +----------+-------------+-------------+-----------------------------------+
+ | 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).
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5f728a6..fc1a012 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -496,6 +496,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)
{
@@ -516,6 +532,74 @@ 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
+
+/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
+static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+ hwaddr dsm_mem_addr)
+{
+ NvdimmFitBuffer *fit_buf = &state->fit_buf;
+ NvdimmFuncReadFITIn *read_fit;
+ NvdimmFuncReadFITOut *read_fit_out;
+ GArray *fit;
+ uint32_t read_len = 0, func_ret_status;
+ int size;
+
+ 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",
+ read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
+
+ if (read_fit->offset > fit->len) {
+ func_ret_status = 3 /* Invalid Input Parameters */;
+ goto exit;
+ }
+
+ /* It is the first time to read FIT. */
+ 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 */;
+ goto exit;
+ }
+
+ func_ret_status = 0 /* Success */;
+ read_len = MIN(fit->len - read_fit->offset,
+ 4096 - sizeof(NvdimmFuncReadFITOut));
+
+exit:
+ size = sizeof(NvdimmFuncReadFITOut) + read_len;
+ read_fit_out = g_malloc(size);
+
+ read_fit_out->len = cpu_to_le32(size);
+ read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
+ memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
+
+ cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
+
+ g_free(read_fit_out);
+ qemu_mutex_unlock(&fit_buf->lock);
+}
+
+static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+ hwaddr dsm_mem_addr)
+{
+ switch (in->function) {
+ case 0x0:
+ nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
+ return;
+ case 0x1 /*Read FIT */:
+ nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
+ return;
+ }
+
+ nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+}
+
static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
{
/*
@@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
static void
nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
{
+ AcpiNVDIMMState *state = opaque;
NvdimmDsmIn *in;
hwaddr dsm_mem_addr = val;
@@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
goto exit;
}
+ if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
+ nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
+ goto exit;
+ }
+
/* Handle 0 is reserved for NVDIMM Root Device. */
if (!in->handle) {
nvdimm_dsm_root(in, dsm_mem_addr);
@@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
#define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
#define NVDIMM_DSM_OUT_BUF "ODAT"
+#define NVDIMM_DSM_RFIT_STATUS "RSTA"
+
+#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
+
static void nvdimm_build_common_dsm(Aml *dev)
{
- Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
+ Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
uint8_t byte_list[1];
@@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev)
/* UUID for NVDIMM Root Device */, expected_uuid));
aml_append(method, ifctx);
elsectx = aml_else();
- aml_append(elsectx, aml_store(
+ ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
+ aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
+ /* UUID for QEMU internal use */), expected_uuid));
+ aml_append(elsectx, ifctx);
+ elsectx2 = aml_else();
+ aml_append(elsectx2, aml_store(
aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
/* UUID for NVDIMM Devices */, expected_uuid));
+ aml_append(elsectx, elsectx2);
aml_append(method, elsectx);
uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
@@ -994,6 +1094,105 @@ 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, *elsectx, *fit;
+
+ buf = aml_local(0);
+ 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));
+
+ /* 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"));
+
+ /* prepare input package. */
+ pkg = aml_package(1);
+ aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
+ aml_append(pkg, aml_name("OFST"));
+
+ /* call Read_FIT function. */
+ call_result = aml_call5(NVDIMM_COMMON_DSM,
+ aml_touuid(NVDIMM_QEMU_RSVD_UUID),
+ aml_int(1) /* Revision 1 */,
+ aml_int(0x1) /* Read FIT */,
+ pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
+ aml_append(method, aml_store(call_result, buf));
+
+ /* handle _DSM result. */
+ aml_append(method, aml_create_dword_field(buf,
+ aml_int(0) /* offset at byte 0 */, "STAU"));
+
+ aml_append(method, aml_store(aml_name("STAU"),
+ aml_name(NVDIMM_DSM_RFIT_STATUS)));
+
+ /* if something is wrong during _DSM. */
+ ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
+ ifctx = aml_if(aml_lnot(ifcond));
+ aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
+ aml_append(method, ifctx);
+
+ aml_append(method, aml_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_SERIALIZED);
+ offset = aml_local(3);
+
+ aml_append(method, aml_store(aml_buffer(0, NULL), fit));
+ aml_append(method, aml_store(aml_int(0), offset));
+
+ whilectx = aml_while(aml_int(1));
+ aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
+ aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
+
+ /*
+ * if fit buffer was changed during RFIT, read from the beginning
+ * again.
+ */
+ ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
+ aml_int(0x100 /* fit changed */)));
+ aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
+ aml_append(ifctx, aml_store(aml_int(0), offset));
+ aml_append(whilectx, ifctx);
+
+ elsectx = aml_else();
+
+ /* finish fit read if no data is read out. */
+ ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+ aml_append(ifctx, aml_return(fit));
+ aml_append(elsectx, ifctx);
+
+ /* update the offset. */
+ aml_append(elsectx, aml_add(offset, buf_size, offset));
+ /* append the data we read out to the fit buffer. */
+ aml_append(elsectx, aml_concatenate(fit, buf, fit));
+ aml_append(whilectx, elsectx);
+ aml_append(method, whilectx);
+
+ aml_append(dev, method);
+}
+
static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
{
uint32_t slot;
@@ -1052,6 +1251,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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-10-28 16:35 ` Xiao Guangrong
-1 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, Xiao Guangrong
_GPE.E04 is dedicated for nvdimm device hotplug
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
docs/specs/acpi_mem_hotplug.txt | 3 +++
hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++--------
hw/i386/acpi-build.c | 7 +++++++
hw/i386/pc.c | 12 ++++++++++++
hw/mem/nvdimm.c | 4 ----
include/hw/acpi/acpi_dev_interface.h | 1 +
6 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 3df3620..cb26dd2 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -4,6 +4,9 @@ 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/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ec4e64b..70f6451 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"
@@ -232,11 +233,8 @@ 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;
- }
+ AcpiEventStatusBits event;
+ bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
mdev = acpi_memory_slot_status(mem_st, dev, errp);
if (!mdev) {
@@ -244,10 +242,23 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
}
mdev->dimm = dev;
- mdev->is_enabled = true;
+
+ /*
+ * 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;
+ }
+
if (dev->hotplugged) {
- mdev->is_inserting = true;
- acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
+ if (!is_nvdimm) {
+ mdev->is_inserting = true;
+ }
+ acpi_send_event(DEVICE(hotplug_dev), event);
}
}
@@ -262,6 +273,8 @@ 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);
}
@@ -276,6 +289,8 @@ 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/i386/acpi-build.c b/hw/i386/acpi-build.c
index bc49958..32270c3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2039,6 +2039,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
method = aml_method("_E03", 0, AML_NOTSERIALIZED);
aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
aml_append(scope, method);
+
+ if (pcms->acpi_nvdimm_state.is_enabled) {
+ method = aml_method("_E04", 0, AML_NOTSERIALIZED);
+ aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
+ aml_int(0x80)));
+ aml_append(scope, method);
+ }
}
aml_append(dsdt, scope);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3be6304..200963f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1729,6 +1729,12 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
goto out;
}
+ if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+ error_setg(&local_err,
+ "nvdimm device hot unplug is not supported yet.");
+ goto out;
+ }
+
hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
@@ -1746,6 +1752,12 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
HotplugHandlerClass *hhc;
Error *local_err = NULL;
+ if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+ error_setg(&local_err,
+ "nvdimm device hot unplug is not supported yet.");
+ goto out;
+ }
+
hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7895805..db896b0 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -148,13 +148,9 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
static void nvdimm_class_init(ObjectClass *oc, void *data)
{
- DeviceClass *dc = DEVICE_CLASS(oc);
PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
NVDIMMClass *nvc = NVDIMM_CLASS(oc);
- /* nvdimm hotplug has not been supported yet. */
- dc->hotpluggable = false;
-
ddc->realize = nvdimm_realize;
ddc->get_memory_region = nvdimm_get_memory_region;
ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index da4ef7f..901a4ae 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -10,6 +10,7 @@ typedef enum {
ACPI_PCI_HOTPLUG_STATUS = 2,
ACPI_CPU_HOTPLUG_STATUS = 4,
ACPI_MEMORY_HOTPLUG_STATUS = 8,
+ ACPI_NVDIMM_HOTPLUG_STATUS = 16,
} AcpiEventStatusBits;
#define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug
@ 2016-10-28 16:35 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-10-28 16:35 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, dan.j.williams,
kvm, qemu-devel, Xiao Guangrong
_GPE.E04 is dedicated for nvdimm device hotplug
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
docs/specs/acpi_mem_hotplug.txt | 3 +++
hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++--------
hw/i386/acpi-build.c | 7 +++++++
hw/i386/pc.c | 12 ++++++++++++
hw/mem/nvdimm.c | 4 ----
include/hw/acpi/acpi_dev_interface.h | 1 +
6 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 3df3620..cb26dd2 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -4,6 +4,9 @@ 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/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ec4e64b..70f6451 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"
@@ -232,11 +233,8 @@ 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;
- }
+ AcpiEventStatusBits event;
+ bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
mdev = acpi_memory_slot_status(mem_st, dev, errp);
if (!mdev) {
@@ -244,10 +242,23 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
}
mdev->dimm = dev;
- mdev->is_enabled = true;
+
+ /*
+ * 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;
+ }
+
if (dev->hotplugged) {
- mdev->is_inserting = true;
- acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
+ if (!is_nvdimm) {
+ mdev->is_inserting = true;
+ }
+ acpi_send_event(DEVICE(hotplug_dev), event);
}
}
@@ -262,6 +273,8 @@ 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);
}
@@ -276,6 +289,8 @@ 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/i386/acpi-build.c b/hw/i386/acpi-build.c
index bc49958..32270c3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2039,6 +2039,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
method = aml_method("_E03", 0, AML_NOTSERIALIZED);
aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
aml_append(scope, method);
+
+ if (pcms->acpi_nvdimm_state.is_enabled) {
+ method = aml_method("_E04", 0, AML_NOTSERIALIZED);
+ aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
+ aml_int(0x80)));
+ aml_append(scope, method);
+ }
}
aml_append(dsdt, scope);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3be6304..200963f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1729,6 +1729,12 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
goto out;
}
+ if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+ error_setg(&local_err,
+ "nvdimm device hot unplug is not supported yet.");
+ goto out;
+ }
+
hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
@@ -1746,6 +1752,12 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
HotplugHandlerClass *hhc;
Error *local_err = NULL;
+ if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+ error_setg(&local_err,
+ "nvdimm device hot unplug is not supported yet.");
+ goto out;
+ }
+
hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7895805..db896b0 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -148,13 +148,9 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
static void nvdimm_class_init(ObjectClass *oc, void *data)
{
- DeviceClass *dc = DEVICE_CLASS(oc);
PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
NVDIMMClass *nvc = NVDIMM_CLASS(oc);
- /* nvdimm hotplug has not been supported yet. */
- dc->hotpluggable = false;
-
ddc->realize = nvdimm_realize;
ddc->get_memory_region = nvdimm_get_memory_region;
ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index da4ef7f..901a4ae 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -10,6 +10,7 @@ typedef enum {
ACPI_PCI_HOTPLUG_STATUS = 2,
ACPI_CPU_HOTPLUG_STATUS = 4,
ACPI_MEMORY_HOTPLUG_STATUS = 8,
+ ACPI_NVDIMM_HOTPLUG_STATUS = 16,
} AcpiEventStatusBits;
#define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v3 1/4] nvdimm acpi: prebuild nvdimm devices for available slots
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-11-01 15:16 ` Igor Mammedov
-1 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-01 15:16 UTC (permalink / raw)
To: Xiao Guangrong
Cc: ehabkost, kvm, mst, gleb, mtosatti, qemu-devel, stefanha,
pbonzini, dan.j.williams, rth
On Sat, 29 Oct 2016 00:35:37 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> For each NVDIMM present or intended to be supported by platform,
> platform firmware also exposes an ACPI Namespace Device under
> the root device
>
> So it builds nvdimm devices for all slots to support vNVDIMM hotplug
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 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 bb896c9..b8a2e62 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -961,12 +961,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;
>
> @@ -987,9 +986,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;
> int mem_addr_offset, nvdimm_ssdt;
> @@ -1021,7 +1020,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);
> @@ -1046,17 +1045,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 is available
> + * slot.
> + */
> + if (ram_slots) {
put this check before assigning to device_list
and bail out early if ram slots 0 as there is no point in proceeding further
> + nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
> + ram_slots);
> }
after that you'd just keep the hunk below unchanged,
which would simplify the patch
> - 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 e999654..6ae4769 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> }
> if (pcms->acpi_nvdimm_state.is_enabled) {
> nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> - pcms->acpi_nvdimm_state.dsm_mem);
> + 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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] nvdimm acpi: prebuild nvdimm devices for available slots
@ 2016-11-01 15:16 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-01 15:16 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On Sat, 29 Oct 2016 00:35:37 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> For each NVDIMM present or intended to be supported by platform,
> platform firmware also exposes an ACPI Namespace Device under
> the root device
>
> So it builds nvdimm devices for all slots to support vNVDIMM hotplug
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 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 bb896c9..b8a2e62 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -961,12 +961,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;
>
> @@ -987,9 +986,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;
> int mem_addr_offset, nvdimm_ssdt;
> @@ -1021,7 +1020,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);
> @@ -1046,17 +1045,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 is available
> + * slot.
> + */
> + if (ram_slots) {
put this check before assigning to device_list
and bail out early if ram slots 0 as there is no point in proceeding further
> + nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
> + ram_slots);
> }
after that you'd just keep the hunk below unchanged,
which would simplify the patch
> - 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 e999654..6ae4769 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> }
> if (pcms->acpi_nvdimm_state.is_enabled) {
> nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> - pcms->acpi_nvdimm_state.dsm_mem);
> + 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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-11-01 15:17 ` Stefan Hajnoczi
-1 siblings, 0 replies; 40+ messages in thread
From: Stefan Hajnoczi @ 2016-11-01 15:17 UTC (permalink / raw)
To: Xiao Guangrong
Cc: ehabkost, kvm, mst, gleb, mtosatti, qemu-devel, stefanha,
imammedo, pbonzini, dan.j.williams, rth
[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]
On Sat, Oct 29, 2016 at 12:35:38AM +0800, Xiao Guangrong wrote:
> The buffer is used to save the FIT info for all the presented nvdimm
> devices which is updated after the nvdimm device is plugged or
> unplugged. In the later patch, it will be used to construct NVDIMM
> ACPI _FIT method which reflects the presented nvdimm devices after
> nvdimm hotplug
>
> As FIT buffer can not completely mapped into guest address space,
> OSPM will exit to QEMU multiple times, however, there is the race
> condition - FIT may be changed during these multiple exits, so that
> some rules are introduced:
> 1) the user should hold the @lock to access the buffer and
I don't understand the purpose of the QEMUMutex lock. Don't all threads
calling nvdimm/acpi code hold the QEMU global mutex (main loop and vcpu
threads)?
> -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
> +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);
Is it possible to call nvdimm_build_device_structure() here? That way
we don't duplicate the g_array_new() details.
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5783442..d835e62 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -945,10 +945,21 @@ 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;
dev->realized = value was already set above.
In order to preserve semantics you would need a bool old_value =
dev->realized which you can restore in post_realize_fail. QEMU current
does not assign dev->realized = value when there is a failure!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
@ 2016-11-01 15:17 ` Stefan Hajnoczi
0 siblings, 0 replies; 40+ messages in thread
From: Stefan Hajnoczi @ 2016-11-01 15:17 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, imammedo, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]
On Sat, Oct 29, 2016 at 12:35:38AM +0800, Xiao Guangrong wrote:
> The buffer is used to save the FIT info for all the presented nvdimm
> devices which is updated after the nvdimm device is plugged or
> unplugged. In the later patch, it will be used to construct NVDIMM
> ACPI _FIT method which reflects the presented nvdimm devices after
> nvdimm hotplug
>
> As FIT buffer can not completely mapped into guest address space,
> OSPM will exit to QEMU multiple times, however, there is the race
> condition - FIT may be changed during these multiple exits, so that
> some rules are introduced:
> 1) the user should hold the @lock to access the buffer and
I don't understand the purpose of the QEMUMutex lock. Don't all threads
calling nvdimm/acpi code hold the QEMU global mutex (main loop and vcpu
threads)?
> -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
> +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);
Is it possible to call nvdimm_build_device_structure() here? That way
we don't duplicate the g_array_new() details.
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5783442..d835e62 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -945,10 +945,21 @@ 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;
dev->realized = value was already set above.
In order to preserve semantics you would need a bool old_value =
dev->realized which you can restore in post_realize_fail. QEMU current
does not assign dev->realized = value when there is a failure!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
2016-11-01 15:17 ` [Qemu-devel] " Stefan Hajnoczi
@ 2016-11-01 15:25 ` Igor Mammedov
-1 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-01 15:25 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Xiao Guangrong, pbonzini, gleb, mtosatti, stefanha, mst, rth,
ehabkost, dan.j.williams, kvm, qemu-devel
On Tue, 1 Nov 2016 15:17:01 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sat, Oct 29, 2016 at 12:35:38AM +0800, Xiao Guangrong wrote:
> > The buffer is used to save the FIT info for all the presented nvdimm
> > devices which is updated after the nvdimm device is plugged or
> > unplugged. In the later patch, it will be used to construct NVDIMM
> > ACPI _FIT method which reflects the presented nvdimm devices after
> > nvdimm hotplug
> >
> > As FIT buffer can not completely mapped into guest address space,
> > OSPM will exit to QEMU multiple times, however, there is the race
> > condition - FIT may be changed during these multiple exits, so that
> > some rules are introduced:
> > 1) the user should hold the @lock to access the buffer and
>
> I don't understand the purpose of the QEMUMutex lock. Don't all threads
> calling nvdimm/acpi code hold the QEMU global mutex (main loop and vcpu
> threads)?
>
> > -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
> > +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);
>
> Is it possible to call nvdimm_build_device_structure() here? That way
> we don't duplicate the g_array_new() details.
>
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 5783442..d835e62 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -945,10 +945,21 @@ 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;
>
> dev->realized = value was already set above.
>
> In order to preserve semantics you would need a bool old_value =
> dev->realized which you can restore in post_realize_fail. QEMU current
> does not assign dev->realized = value when there is a failure!
Stefan,
as agreed on
"Re: [Qemu-devel] [PULL 00/47] virtio, pc: fixes and features" thread
this series would be merged as is and then as follow up author
will fixup all issues with it.
For this patch it means a patch on top of Michael pull req
to drop lock and drop hotplug_handler_post_plug() code in favor
of existing hotplug_handler_plug().
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
@ 2016-11-01 15:25 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-01 15:25 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Xiao Guangrong, pbonzini, gleb, mtosatti, stefanha, mst, rth,
ehabkost, dan.j.williams, kvm, qemu-devel
On Tue, 1 Nov 2016 15:17:01 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sat, Oct 29, 2016 at 12:35:38AM +0800, Xiao Guangrong wrote:
> > The buffer is used to save the FIT info for all the presented nvdimm
> > devices which is updated after the nvdimm device is plugged or
> > unplugged. In the later patch, it will be used to construct NVDIMM
> > ACPI _FIT method which reflects the presented nvdimm devices after
> > nvdimm hotplug
> >
> > As FIT buffer can not completely mapped into guest address space,
> > OSPM will exit to QEMU multiple times, however, there is the race
> > condition - FIT may be changed during these multiple exits, so that
> > some rules are introduced:
> > 1) the user should hold the @lock to access the buffer and
>
> I don't understand the purpose of the QEMUMutex lock. Don't all threads
> calling nvdimm/acpi code hold the QEMU global mutex (main loop and vcpu
> threads)?
>
> > -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
> > +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);
>
> Is it possible to call nvdimm_build_device_structure() here? That way
> we don't duplicate the g_array_new() details.
>
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 5783442..d835e62 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -945,10 +945,21 @@ 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;
>
> dev->realized = value was already set above.
>
> In order to preserve semantics you would need a bool old_value =
> dev->realized which you can restore in post_realize_fail. QEMU current
> does not assign dev->realized = value when there is a failure!
Stefan,
as agreed on
"Re: [Qemu-devel] [PULL 00/47] virtio, pc: fixes and features" thread
this series would be merged as is and then as follow up author
will fixup all issues with it.
For this patch it means a patch on top of Michael pull req
to drop lock and drop hotplug_handler_post_plug() code in favor
of existing hotplug_handler_plug().
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
2016-11-01 15:25 ` [Qemu-devel] " Igor Mammedov
@ 2016-11-01 16:00 ` Xiao Guangrong
-1 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-01 16:00 UTC (permalink / raw)
To: Igor Mammedov, Stefan Hajnoczi
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On 11/01/2016 11:25 PM, Igor Mammedov wrote:
> On Tue, 1 Nov 2016 15:17:01 +0000
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> On Sat, Oct 29, 2016 at 12:35:38AM +0800, Xiao Guangrong wrote:
>>> The buffer is used to save the FIT info for all the presented nvdimm
>>> devices which is updated after the nvdimm device is plugged or
>>> unplugged. In the later patch, it will be used to construct NVDIMM
>>> ACPI _FIT method which reflects the presented nvdimm devices after
>>> nvdimm hotplug
>>>
>>> As FIT buffer can not completely mapped into guest address space,
>>> OSPM will exit to QEMU multiple times, however, there is the race
>>> condition - FIT may be changed during these multiple exits, so that
>>> some rules are introduced:
>>> 1) the user should hold the @lock to access the buffer and
>>
>> I don't understand the purpose of the QEMUMutex lock. Don't all threads
>> calling nvdimm/acpi code hold the QEMU global mutex (main loop and vcpu
>> threads)?
>>
>>> -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
>>> +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);
>>
>> Is it possible to call nvdimm_build_device_structure() here? That way
>> we don't duplicate the g_array_new() details.
>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 5783442..d835e62 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -945,10 +945,21 @@ 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;
>>
>> dev->realized = value was already set above.
>>
>> In order to preserve semantics you would need a bool old_value =
>> dev->realized which you can restore in post_realize_fail. QEMU current
>> does not assign dev->realized = value when there is a failure!
>
> Stefan,
>
> as agreed on
> "Re: [Qemu-devel] [PULL 00/47] virtio, pc: fixes and features" thread
> this series would be merged as is and then as follow up author
> will fixup all issues with it.
>
> For this patch it means a patch on top of Michael pull req
> to drop lock and drop hotplug_handler_post_plug() code in favor
> of existing hotplug_handler_plug().
>
Thank you, Igor, Michael and Stefan. I will do it asap after
this patchset is merged. :)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
@ 2016-11-01 16:00 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-01 16:00 UTC (permalink / raw)
To: Igor Mammedov, Stefan Hajnoczi
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On 11/01/2016 11:25 PM, Igor Mammedov wrote:
> On Tue, 1 Nov 2016 15:17:01 +0000
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> On Sat, Oct 29, 2016 at 12:35:38AM +0800, Xiao Guangrong wrote:
>>> The buffer is used to save the FIT info for all the presented nvdimm
>>> devices which is updated after the nvdimm device is plugged or
>>> unplugged. In the later patch, it will be used to construct NVDIMM
>>> ACPI _FIT method which reflects the presented nvdimm devices after
>>> nvdimm hotplug
>>>
>>> As FIT buffer can not completely mapped into guest address space,
>>> OSPM will exit to QEMU multiple times, however, there is the race
>>> condition - FIT may be changed during these multiple exits, so that
>>> some rules are introduced:
>>> 1) the user should hold the @lock to access the buffer and
>>
>> I don't understand the purpose of the QEMUMutex lock. Don't all threads
>> calling nvdimm/acpi code hold the QEMU global mutex (main loop and vcpu
>> threads)?
>>
>>> -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
>>> +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);
>>
>> Is it possible to call nvdimm_build_device_structure() here? That way
>> we don't duplicate the g_array_new() details.
>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 5783442..d835e62 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -945,10 +945,21 @@ 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;
>>
>> dev->realized = value was already set above.
>>
>> In order to preserve semantics you would need a bool old_value =
>> dev->realized which you can restore in post_realize_fail. QEMU current
>> does not assign dev->realized = value when there is a failure!
>
> Stefan,
>
> as agreed on
> "Re: [Qemu-devel] [PULL 00/47] virtio, pc: fixes and features" thread
> this series would be merged as is and then as follow up author
> will fixup all issues with it.
>
> For this patch it means a patch on top of Michael pull req
> to drop lock and drop hotplug_handler_post_plug() code in favor
> of existing hotplug_handler_plug().
>
Thank you, Igor, Michael and Stefan. I will do it asap after
this patchset is merged. :)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-11-01 16:24 ` Igor Mammedov
-1 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-01 16:24 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On Sat, 29 Oct 2016 00:35:39 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> _FIT is required for hotplug support, guest will inquire the updated
> device info from it if a hotplug event is received
>
> As FIT buffer is not completely mapped into guest address space, so a
> new function, Read FIT whose UUID is UUID
> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> is concatenated before _FIT return
>
> Refer to docs/specs/acpi-nvdimm.txt for detailed design
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
> docs/specs/acpi_nvdimm.txt | 58 ++++++++++++-
> hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 257 insertions(+), 5 deletions(-)
>
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 0fdd251..4aa5e3d 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -127,6 +127,58 @@ _DSM process diagram:
> | result from the page | | |
> +--------------------------+ +--------------+
>
> - _FIT implementation
> - -------------------
> - TODO (will fill it when nvdimm hotplug is introduced)
> +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.
Above part should go to section where 'handle' is defined, i.e. earlier in the file:
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.
> +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
UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function
(private QEMU function)
> + 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 Read_FIT function to fetch NFIT structures blob from QEMU
in 1 page sized increments which are then concatenated and returned as _FIT method result.
> +
> + Input parameters:
> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> + Arg1 – Revision ID (set to 1)
> + Arg2 - Function Index, 0x1
> + Arg3 - A package containing a buffer whose layout is as follows:
> +
> + +----------+-------------+-------------+-----------------------------------+
> + | Filed | Byte Length | Byte Offset | Description |
^ field, s/Byte//, s/Byte//
> + +----------+-------------+-------------+-----------------------------------+
> + | offset | 4 | 0 | the offset of FIT buffer |
offset in QEMU's NFIT structures blob to read from
> + +----------+-------------+-------------+-----------------------------------+
> +
> + Output:
> + +----------+-------------+-------------+-----------------------------------+
> + | Filed | Byte Length | Byte Offset | Description |
> + +----------+-------------+-------------+-----------------------------------+
> + | | | | return status codes |
> + | | | | 0x100 indicates fit has been |
> + | status | 4 | 0 | updated |
0x100 - error caused by NFIT update while read by _FIT wasn't completed
> + | | | | other follows Chapter 3 in DSM |
s/other follows/other codes follow/
> + | | | | Spec Rev1 |
> + +----------+-------------+-------------+-----------------------------------+
> + | fit data | Varies | 4 | FIT data |
> + | | | | |
> + +----------+-------------+-------------+-----------------------------------+
what does "Varies" mean, how would I know reading this how much data Read_FIT should read
from shared page?
> +
> + The FIT offset is maintained by the caller itself,
probably is not necessary sentence, or specify a caller (for example OSPM)
> 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).
[...]
that's all for doc part, I'll do the code part later.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
@ 2016-11-01 16:24 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-01 16:24 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On Sat, 29 Oct 2016 00:35:39 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> _FIT is required for hotplug support, guest will inquire the updated
> device info from it if a hotplug event is received
>
> As FIT buffer is not completely mapped into guest address space, so a
> new function, Read FIT whose UUID is UUID
> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> is concatenated before _FIT return
>
> Refer to docs/specs/acpi-nvdimm.txt for detailed design
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
> docs/specs/acpi_nvdimm.txt | 58 ++++++++++++-
> hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 257 insertions(+), 5 deletions(-)
>
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 0fdd251..4aa5e3d 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -127,6 +127,58 @@ _DSM process diagram:
> | result from the page | | |
> +--------------------------+ +--------------+
>
> - _FIT implementation
> - -------------------
> - TODO (will fill it when nvdimm hotplug is introduced)
> +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.
Above part should go to section where 'handle' is defined, i.e. earlier in the file:
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.
> +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
UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function
(private QEMU function)
> + 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 Read_FIT function to fetch NFIT structures blob from QEMU
in 1 page sized increments which are then concatenated and returned as _FIT method result.
> +
> + Input parameters:
> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> + Arg1 – Revision ID (set to 1)
> + Arg2 - Function Index, 0x1
> + Arg3 - A package containing a buffer whose layout is as follows:
> +
> + +----------+-------------+-------------+-----------------------------------+
> + | Filed | Byte Length | Byte Offset | Description |
^ field, s/Byte//, s/Byte//
> + +----------+-------------+-------------+-----------------------------------+
> + | offset | 4 | 0 | the offset of FIT buffer |
offset in QEMU's NFIT structures blob to read from
> + +----------+-------------+-------------+-----------------------------------+
> +
> + Output:
> + +----------+-------------+-------------+-----------------------------------+
> + | Filed | Byte Length | Byte Offset | Description |
> + +----------+-------------+-------------+-----------------------------------+
> + | | | | return status codes |
> + | | | | 0x100 indicates fit has been |
> + | status | 4 | 0 | updated |
0x100 - error caused by NFIT update while read by _FIT wasn't completed
> + | | | | other follows Chapter 3 in DSM |
s/other follows/other codes follow/
> + | | | | Spec Rev1 |
> + +----------+-------------+-------------+-----------------------------------+
> + | fit data | Varies | 4 | FIT data |
> + | | | | |
> + +----------+-------------+-------------+-----------------------------------+
what does "Varies" mean, how would I know reading this how much data Read_FIT should read
from shared page?
> +
> + The FIT offset is maintained by the caller itself,
probably is not necessary sentence, or specify a caller (for example OSPM)
> 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).
[...]
that's all for doc part, I'll do the code part later.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-11-01 16:41 ` Stefan Hajnoczi
-1 siblings, 0 replies; 40+ messages in thread
From: Stefan Hajnoczi @ 2016-11-01 16:41 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, imammedo, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]
On Sat, Oct 29, 2016 at 12:35:39AM +0800, Xiao Guangrong wrote:
> +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.
> +
> + Input parameters:
> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> + Arg1 – Revision ID (set to 1)
> + Arg2 - Function Index, 0x1
> + Arg3 - A package containing a buffer whose layout is as follows:
> +
> + +----------+-------------+-------------+-----------------------------------+
> + | Filed | Byte Length | Byte Offset | Description |
s/Filed/Field/
The same applies below too.
> + +----------+-------------+-------------+-----------------------------------+
> + | offset | 4 | 0 | the offset of FIT buffer |
> + +----------+-------------+-------------+-----------------------------------+
s/offset of FIT buffer/offset into 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
s/plugs/plus/
> +struct NvdimmFuncReadFITIn {
> + uint32_t offset; /* the offset of FIT buffer. */
s/offset of FIT buffer/offset into FIT buffer/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
@ 2016-11-01 16:41 ` Stefan Hajnoczi
0 siblings, 0 replies; 40+ messages in thread
From: Stefan Hajnoczi @ 2016-11-01 16:41 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, imammedo, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]
On Sat, Oct 29, 2016 at 12:35:39AM +0800, Xiao Guangrong wrote:
> +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.
> +
> + Input parameters:
> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> + Arg1 – Revision ID (set to 1)
> + Arg2 - Function Index, 0x1
> + Arg3 - A package containing a buffer whose layout is as follows:
> +
> + +----------+-------------+-------------+-----------------------------------+
> + | Filed | Byte Length | Byte Offset | Description |
s/Filed/Field/
The same applies below too.
> + +----------+-------------+-------------+-----------------------------------+
> + | offset | 4 | 0 | the offset of FIT buffer |
> + +----------+-------------+-------------+-----------------------------------+
s/offset of FIT buffer/offset into 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
s/plugs/plus/
> +struct NvdimmFuncReadFITIn {
> + uint32_t offset; /* the offset of FIT buffer. */
s/offset of FIT buffer/offset into FIT buffer/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-11-01 16:44 ` Stefan Hajnoczi
-1 siblings, 0 replies; 40+ messages in thread
From: Stefan Hajnoczi @ 2016-11-01 16:44 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, imammedo, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 642 bytes --]
On Sat, Oct 29, 2016 at 12:35:40AM +0800, Xiao Guangrong wrote:
> _GPE.E04 is dedicated for nvdimm device hotplug
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
> docs/specs/acpi_mem_hotplug.txt | 3 +++
> hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++--------
> hw/i386/acpi-build.c | 7 +++++++
> hw/i386/pc.c | 12 ++++++++++++
> hw/mem/nvdimm.c | 4 ----
> include/hw/acpi/acpi_dev_interface.h | 1 +
> 6 files changed, 46 insertions(+), 12 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug
@ 2016-11-01 16:44 ` Stefan Hajnoczi
0 siblings, 0 replies; 40+ messages in thread
From: Stefan Hajnoczi @ 2016-11-01 16:44 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, imammedo, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 642 bytes --]
On Sat, Oct 29, 2016 at 12:35:40AM +0800, Xiao Guangrong wrote:
> _GPE.E04 is dedicated for nvdimm device hotplug
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
> docs/specs/acpi_mem_hotplug.txt | 3 +++
> hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++--------
> hw/i386/acpi-build.c | 7 +++++++
> hw/i386/pc.c | 12 ++++++++++++
> hw/mem/nvdimm.c | 4 ----
> include/hw/acpi/acpi_dev_interface.h | 1 +
> 6 files changed, 46 insertions(+), 12 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
@ 2016-11-02 11:21 ` Igor Mammedov
-1 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-02 11:21 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On Sat, 29 Oct 2016 00:35:40 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> _GPE.E04 is dedicated for nvdimm device hotplug
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
> docs/specs/acpi_mem_hotplug.txt | 3 +++
> hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++--------
> hw/i386/acpi-build.c | 7 +++++++
> hw/i386/pc.c | 12 ++++++++++++
> hw/mem/nvdimm.c | 4 ----
> include/hw/acpi/acpi_dev_interface.h | 1 +
> 6 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index 3df3620..cb26dd2 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -4,6 +4,9 @@ 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.
that should be part of nvdimm spec
> Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
> ---------------------------------------------------------------
> 0xa00:
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ec4e64b..70f6451 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
so far nvdimm hotplug has nothing do with memory hotplug so revert
all changes you did to this file and put nvdimm hotplug handler
code into hw/acpi/nvdimm.c
and call it from respective ich9/piix4_pm handlers.
[...]
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index bc49958..32270c3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2039,6 +2039,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> method = aml_method("_E03", 0, AML_NOTSERIALIZED);
> aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
> aml_append(scope, method);
> +
> + if (pcms->acpi_nvdimm_state.is_enabled) {
> + method = aml_method("_E04", 0, AML_NOTSERIALIZED);
> + aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
> + aml_int(0x80)));
> + aml_append(scope, method);
> + }
> }
> aml_append(dsdt, scope);
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3be6304..200963f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1729,6 +1729,12 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
> goto out;
> }
>
> + if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> + error_setg(&local_err,
> + "nvdimm device hot unplug is not supported yet.");
> + goto out;
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>
> @@ -1746,6 +1752,12 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> HotplugHandlerClass *hhc;
> Error *local_err = NULL;
>
> + if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> + error_setg(&local_err,
> + "nvdimm device hot unplug is not supported yet.");
> + goto out;
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7895805..db896b0 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -148,13 +148,9 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>
> static void nvdimm_class_init(ObjectClass *oc, void *data)
> {
> - DeviceClass *dc = DEVICE_CLASS(oc);
> PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
> NVDIMMClass *nvc = NVDIMM_CLASS(oc);
>
> - /* nvdimm hotplug has not been supported yet. */
> - dc->hotpluggable = false;
> -
> ddc->realize = nvdimm_realize;
> ddc->get_memory_region = nvdimm_get_memory_region;
> ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
> index da4ef7f..901a4ae 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -10,6 +10,7 @@ typedef enum {
> ACPI_PCI_HOTPLUG_STATUS = 2,
> ACPI_CPU_HOTPLUG_STATUS = 4,
> ACPI_MEMORY_HOTPLUG_STATUS = 8,
> + ACPI_NVDIMM_HOTPLUG_STATUS = 16,
> } AcpiEventStatusBits;
>
> #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug
@ 2016-11-02 11:21 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-02 11:21 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On Sat, 29 Oct 2016 00:35:40 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> _GPE.E04 is dedicated for nvdimm device hotplug
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
> docs/specs/acpi_mem_hotplug.txt | 3 +++
> hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++--------
> hw/i386/acpi-build.c | 7 +++++++
> hw/i386/pc.c | 12 ++++++++++++
> hw/mem/nvdimm.c | 4 ----
> include/hw/acpi/acpi_dev_interface.h | 1 +
> 6 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index 3df3620..cb26dd2 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -4,6 +4,9 @@ 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.
that should be part of nvdimm spec
> Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
> ---------------------------------------------------------------
> 0xa00:
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ec4e64b..70f6451 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
so far nvdimm hotplug has nothing do with memory hotplug so revert
all changes you did to this file and put nvdimm hotplug handler
code into hw/acpi/nvdimm.c
and call it from respective ich9/piix4_pm handlers.
[...]
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index bc49958..32270c3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2039,6 +2039,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> method = aml_method("_E03", 0, AML_NOTSERIALIZED);
> aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
> aml_append(scope, method);
> +
> + if (pcms->acpi_nvdimm_state.is_enabled) {
> + method = aml_method("_E04", 0, AML_NOTSERIALIZED);
> + aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
> + aml_int(0x80)));
> + aml_append(scope, method);
> + }
> }
> aml_append(dsdt, scope);
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3be6304..200963f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1729,6 +1729,12 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
> goto out;
> }
>
> + if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> + error_setg(&local_err,
> + "nvdimm device hot unplug is not supported yet.");
> + goto out;
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>
> @@ -1746,6 +1752,12 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> HotplugHandlerClass *hhc;
> Error *local_err = NULL;
>
> + if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> + error_setg(&local_err,
> + "nvdimm device hot unplug is not supported yet.");
> + goto out;
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7895805..db896b0 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -148,13 +148,9 @@ static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>
> static void nvdimm_class_init(ObjectClass *oc, void *data)
> {
> - DeviceClass *dc = DEVICE_CLASS(oc);
> PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
> NVDIMMClass *nvc = NVDIMM_CLASS(oc);
>
> - /* nvdimm hotplug has not been supported yet. */
> - dc->hotpluggable = false;
> -
> ddc->realize = nvdimm_realize;
> ddc->get_memory_region = nvdimm_get_memory_region;
> ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
> index da4ef7f..901a4ae 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -10,6 +10,7 @@ typedef enum {
> ACPI_PCI_HOTPLUG_STATUS = 2,
> ACPI_CPU_HOTPLUG_STATUS = 4,
> ACPI_MEMORY_HOTPLUG_STATUS = 8,
> + ACPI_NVDIMM_HOTPLUG_STATUS = 16,
> } AcpiEventStatusBits;
>
> #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
` (2 preceding siblings ...)
(?)
@ 2016-11-02 13:56 ` Igor Mammedov
2016-11-02 15:54 ` Xiao Guangrong
-1 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2016-11-02 13:56 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, ehabkost, kvm, mst, gleb, mtosatti, qemu-devel,
stefanha, dan.j.williams, rth
On Sat, 29 Oct 2016 00:35:39 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> _FIT is required for hotplug support, guest will inquire the updated
> device info from it if a hotplug event is received
>
> As FIT buffer is not completely mapped into guest address space, so a
> new function, Read FIT whose UUID is UUID
> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> is concatenated before _FIT return
>
> Refer to docs/specs/acpi-nvdimm.txt for detailed design
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
[...]
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 5f728a6..fc1a012 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -496,6 +496,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)
> {
> @@ -516,6 +532,74 @@ 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
> +
> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> + hwaddr dsm_mem_addr)
> +{
> + NvdimmFitBuffer *fit_buf = &state->fit_buf;
> + NvdimmFuncReadFITIn *read_fit;
> + NvdimmFuncReadFITOut *read_fit_out;
> + GArray *fit;
> + uint32_t read_len = 0, func_ret_status;
> + int size;
> +
> + read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> + le32_to_cpus(&read_fit->offset);
I'd prefer if you'd not do inplace conversion, just do
offset = le32_to_cpu(read_fit->offset);
> +
> + qemu_mutex_lock(&fit_buf->lock);
> + fit = fit_buf->fit;
> +
> + nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
> + read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
as follow up path replace nvdimm_debug() with trace events
> +
> + if (read_fit->offset > fit->len) {
> + func_ret_status = 3 /* Invalid Input Parameters */;
should be macros instead of magic value
> + goto exit;
> + }
> +
> + /* It is the first time to read FIT. */
> + 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 */;
should be macros instead of magic value
> + goto exit;
> + }
> +
> + func_ret_status = 0 /* Success */;
> + read_len = MIN(fit->len - read_fit->offset,
> + 4096 - sizeof(NvdimmFuncReadFITOut));
> +
> +exit:
> + size = sizeof(NvdimmFuncReadFITOut) + read_len;
> + read_fit_out = g_malloc(size);
> +
> + read_fit_out->len = cpu_to_le32(size);
> + read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
> + memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
> +
> + cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
> +
> + g_free(read_fit_out);
> + qemu_mutex_unlock(&fit_buf->lock);
> +}
> +
> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> + hwaddr dsm_mem_addr)
> +{
> + switch (in->function) {
> + case 0x0:
> + nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
> + return;
> + case 0x1 /*Read FIT */:
> + nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
> + return;
> + }
> +
> + nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
should be macros instead of magic value
> +}
> +
> static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> {
> /*
> @@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> static void
> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> {
> + AcpiNVDIMMState *state = opaque;
> NvdimmDsmIn *in;
> hwaddr dsm_mem_addr = val;
>
> @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> goto exit;
> }
>
> + if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> + nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
> + goto exit;
> + }
> +
> /* Handle 0 is reserved for NVDIMM Root Device. */
> if (!in->handle) {
> nvdimm_dsm_root(in, dsm_mem_addr);
> @@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
> #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
> #define NVDIMM_DSM_OUT_BUF "ODAT"
>
> +#define NVDIMM_DSM_RFIT_STATUS "RSTA"
> +
> +#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
> +
> static void nvdimm_build_common_dsm(Aml *dev)
> {
> - Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
> + Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
> Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
> Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
> uint8_t byte_list[1];
> @@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev)
> /* UUID for NVDIMM Root Device */, expected_uuid));
> aml_append(method, ifctx);
> elsectx = aml_else();
> - aml_append(elsectx, aml_store(
> + ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
> + aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
> + /* UUID for QEMU internal use */), expected_uuid));
> + aml_append(elsectx, ifctx);
> + elsectx2 = aml_else();
> + aml_append(elsectx2, aml_store(
> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
> /* UUID for NVDIMM Devices */, expected_uuid));
> + aml_append(elsectx, elsectx2);
> aml_append(method, elsectx);
>
> uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
> @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
> aml_append(dev, method);
> }
>
> +static void nvdimm_build_fit(Aml *dev)
nvdimm_build_fit_method()
> +{
> + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
> + Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
> +
> + buf = aml_local(0);
> + buf_size = aml_local(1);
> + fit = aml_local(2);
> +
> + aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL),
> + aml_int(0), NVDIMM_DSM_RFIT_STATUS));
it doesn't have to be buffer as it's internal ASL integer object
so it could be just named variable.
I'd also move it to _FIT method instead of making it device global
and if it could work try to pass it as argument to RFIT
RefOf/DerefOf may help here
or make return value instead of buffer and pass buffer as reference.
Alternatively you can return buffer from RFIT with status field included
and check/discard status value there.
> +
> + /* 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"));
> +
> + /* prepare input package. */
> + pkg = aml_package(1);
> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
> + aml_append(pkg, aml_name("OFST"));
> +
> + /* call Read_FIT function. */
> + call_result = aml_call5(NVDIMM_COMMON_DSM,
> + aml_touuid(NVDIMM_QEMU_RSVD_UUID),
> + aml_int(1) /* Revision 1 */,
> + aml_int(0x1) /* Read FIT */,
> + pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
> + aml_append(method, aml_store(call_result, buf));
> +
> + /* handle _DSM result. */
> + aml_append(method, aml_create_dword_field(buf,
> + aml_int(0) /* offset at byte 0 */, "STAU"));
> +
> + aml_append(method, aml_store(aml_name("STAU"),
> + aml_name(NVDIMM_DSM_RFIT_STATUS)));
> +
> + /* if something is wrong during _DSM. */
> + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
> + ifctx = aml_if(aml_lnot(ifcond));
> + aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> + aml_append(method, ifctx);
> +
> + aml_append(method, aml_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));
there isn't need to convert bytes to bits and store it in the same variable
it creates confusion
> + aml_append(method, aml_create_field(buf,
> + aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
> + buf_size, "BUFF"));
just inline conversion in aml_create_field()
> + aml_append(method, aml_return(aml_name("BUFF")));
> + aml_append(dev, method);
> +
> + /* build _FIT. */
> + method = aml_method("_FIT", 0, AML_SERIALIZED);
> + offset = aml_local(3);
> +
> + aml_append(method, aml_store(aml_buffer(0, NULL), fit));
> + aml_append(method, aml_store(aml_int(0), offset));
> +
> + whilectx = aml_while(aml_int(1));
> + aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
> + aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
> +
> + /*
> + * if fit buffer was changed during RFIT, read from the beginning
> + * again.
> + */
> + ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
> + aml_int(0x100 /* fit changed */)));
> + aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
> + aml_append(ifctx, aml_store(aml_int(0), offset));
> + aml_append(whilectx, ifctx);
> +
> + elsectx = aml_else();
> +
> + /* finish fit read if no data is read out. */
> + ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> + aml_append(ifctx, aml_return(fit));
> + aml_append(elsectx, ifctx);
> +
> + /* update the offset. */
> + aml_append(elsectx, aml_add(offset, buf_size, offset));
> + /* append the data we read out to the fit buffer. */
> + aml_append(elsectx, aml_concatenate(fit, buf, fit));
> + aml_append(whilectx, elsectx);
> + aml_append(method, whilectx);
> +
> + aml_append(dev, method);
> +}
> +
> static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
> {
> uint32_t slot;
> @@ -1052,6 +1251,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);
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] nvdimm: hotplug support
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
` (4 preceding siblings ...)
(?)
@ 2016-11-02 14:01 ` Igor Mammedov
2016-11-03 4:53 ` Michael S. Tsirkin
-1 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2016-11-02 14:01 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, ehabkost, kvm, mst, gleb, mtosatti, qemu-devel,
stefanha, dan.j.williams, rth
On Sat, 29 Oct 2016 00:35:36 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> It is based on my previous patchset,
> "[PATCH 0/8] nvdimm acpi: bug fix and cleanup", these two patchset are
> against commit dea651a95af6dad099 (intel-iommu: Check IOAPIC's Trigger Mode
> against the one in IRTE) on pci branch of Michael's git tree and can be
> found at:
> https://github.com/xiaogr/qemu.git nvdimm-hotplug-v3
>
> Changelog in v3:
> 1) use a dedicated interrupt for nvdimm device hotplug
> 2) stop nvdimm device hot unplug
> 3) reserve UUID and handle for QEMU internally used QEMU
> 5) redesign fit buffer to avoid OSPM reading incomplete fit info
> 6) bug fixes and cleanups
>
> Changelog in v2:
> Fixed signed integer overflow pointed out by Stefan Hajnoczi
>
> This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> for example, a new nvdimm device can be plugged as follows:
> object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> device_add nvdimm,id=nvdimm3,memdev=mem3
>
> and unplug it as follows:
> device_del nvdimm3
> object_del mem3
there is no unplug support
instead of incremental fixups on top merged patches in followup series,
I'd prefer it to make a clean revert for patches 2-4/4 first and
them amended versions of them to follow.
>
> Xiao Guangrong (4):
> nvdimm acpi: prebuild nvdimm devices for available slots
> nvdimm acpi: introduce fit buffer
> nvdimm acpi: introduce _FIT
> pc: memhp: enable nvdimm device hotplug
>
> docs/specs/acpi_mem_hotplug.txt | 3 +
> docs/specs/acpi_nvdimm.txt | 58 ++++++-
> hw/acpi/memory_hotplug.c | 31 +++-
> hw/acpi/nvdimm.c | 286 +++++++++++++++++++++++++++++++----
> hw/core/hotplug.c | 11 ++
> hw/core/qdev.c | 20 ++-
> hw/i386/acpi-build.c | 9 +-
> hw/i386/pc.c | 31 ++++
> hw/mem/nvdimm.c | 4 -
> include/hw/acpi/acpi_dev_interface.h | 1 +
> include/hw/hotplug.h | 10 ++
> include/hw/mem/nvdimm.h | 27 +++-
> 12 files changed, 443 insertions(+), 48 deletions(-)
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-11-01 16:24 ` [Qemu-devel] " Igor Mammedov
@ 2016-11-02 15:40 ` Xiao Guangrong
-1 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-02 15:40 UTC (permalink / raw)
To: Igor Mammedov
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On 11/02/2016 12:24 AM, Igor Mammedov wrote:
> On Sat, 29 Oct 2016 00:35:39 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _FIT is required for hotplug support, guest will inquire the updated
>> device info from it if a hotplug event is received
>>
>> As FIT buffer is not completely mapped into guest address space, so a
>> new function, Read FIT whose UUID is UUID
>> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
>> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
>> is concatenated before _FIT return
>>
>> Refer to docs/specs/acpi-nvdimm.txt for detailed design
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>> docs/specs/acpi_nvdimm.txt | 58 ++++++++++++-
>> hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 257 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
>> index 0fdd251..4aa5e3d 100644
>> --- a/docs/specs/acpi_nvdimm.txt
>> +++ b/docs/specs/acpi_nvdimm.txt
>> @@ -127,6 +127,58 @@ _DSM process diagram:
>> | result from the page | | |
>> +--------------------------+ +--------------+
>>
>> - _FIT implementation
>> - -------------------
>> - TODO (will fill it when nvdimm hotplug is introduced)
>> +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.
> Above part should go to section where 'handle' is defined, i.e. earlier in the file:
>
> 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.
>
Okay.
>
>> +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
> UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function
> (private QEMU function)
>
okay.
>> + 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 Read_FIT function to fetch NFIT structures blob from QEMU
> in 1 page sized increments which are then concatenated and returned as _FIT method result.
>
okay.
>
>> +
>> + Input parameters:
>> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
>> + Arg1 – Revision ID (set to 1)
>> + Arg2 - Function Index, 0x1
>> + Arg3 - A package containing a buffer whose layout is as follows:
>> +
>> + +----------+-------------+-------------+-----------------------------------+
>> + | Filed | Byte Length | Byte Offset | Description |
> ^ field, s/Byte//, s/Byte//
>> + +----------+-------------+-------------+-----------------------------------+
>> + | offset | 4 | 0 | the offset of FIT buffer |
> offset in QEMU's NFIT structures blob to read from
okay.
>
>> + +----------+-------------+-------------+-----------------------------------+
>> +
>> + Output:
>> + +----------+-------------+-------------+-----------------------------------+
>> + | Filed | Byte Length | Byte Offset | Description |
>> + +----------+-------------+-------------+-----------------------------------+
>> + | | | | return status codes |
>> + | | | | 0x100 indicates fit has been |
>> + | status | 4 | 0 | updated |
> 0x100 - error caused by NFIT update while read by _FIT wasn't completed
okay.
>
>> + | | | | other follows Chapter 3 in DSM |
> s/other follows/other codes follow/
okay.
>
>> + | | | | Spec Rev1 |
>> + +----------+-------------+-------------+-----------------------------------+
>> + | fit data | Varies | 4 | FIT data |
>> + | | | | |
>> + +----------+-------------+-------------+-----------------------------------+
> what does "Varies" mean, how would I know reading this how much data Read_FIT should read
> from shared page?
I mean other bytes in the buffer returned by this function except the 'status' field
will be used as fit data. Maybe it is has a 'length' field?
>
>> +
>> + The FIT offset is maintained by the caller itself,
> probably is not necessary sentence, or specify a caller (for example OSPM)
>
Okay.
>> current offset plugs
> ^^^^?
Typo. should be plus.
>
>> + the length returned by the function is the next offset we should read.
>> + When all the FIT data has been read out, zero length is returned.
>> +
>> + If it returns 0x100, OSPM should restart to read FIT (read from offset 0
>> + again).
> [...]
> that's all for doc part, I'll do the code part later.
>
Thank you, Igor.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
@ 2016-11-02 15:40 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-02 15:40 UTC (permalink / raw)
To: Igor Mammedov
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On 11/02/2016 12:24 AM, Igor Mammedov wrote:
> On Sat, 29 Oct 2016 00:35:39 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _FIT is required for hotplug support, guest will inquire the updated
>> device info from it if a hotplug event is received
>>
>> As FIT buffer is not completely mapped into guest address space, so a
>> new function, Read FIT whose UUID is UUID
>> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
>> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
>> is concatenated before _FIT return
>>
>> Refer to docs/specs/acpi-nvdimm.txt for detailed design
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>> docs/specs/acpi_nvdimm.txt | 58 ++++++++++++-
>> hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 257 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
>> index 0fdd251..4aa5e3d 100644
>> --- a/docs/specs/acpi_nvdimm.txt
>> +++ b/docs/specs/acpi_nvdimm.txt
>> @@ -127,6 +127,58 @@ _DSM process diagram:
>> | result from the page | | |
>> +--------------------------+ +--------------+
>>
>> - _FIT implementation
>> - -------------------
>> - TODO (will fill it when nvdimm hotplug is introduced)
>> +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.
> Above part should go to section where 'handle' is defined, i.e. earlier in the file:
>
> 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.
>
Okay.
>
>> +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
> UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function
> (private QEMU function)
>
okay.
>> + 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 Read_FIT function to fetch NFIT structures blob from QEMU
> in 1 page sized increments which are then concatenated and returned as _FIT method result.
>
okay.
>
>> +
>> + Input parameters:
>> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
>> + Arg1 – Revision ID (set to 1)
>> + Arg2 - Function Index, 0x1
>> + Arg3 - A package containing a buffer whose layout is as follows:
>> +
>> + +----------+-------------+-------------+-----------------------------------+
>> + | Filed | Byte Length | Byte Offset | Description |
> ^ field, s/Byte//, s/Byte//
>> + +----------+-------------+-------------+-----------------------------------+
>> + | offset | 4 | 0 | the offset of FIT buffer |
> offset in QEMU's NFIT structures blob to read from
okay.
>
>> + +----------+-------------+-------------+-----------------------------------+
>> +
>> + Output:
>> + +----------+-------------+-------------+-----------------------------------+
>> + | Filed | Byte Length | Byte Offset | Description |
>> + +----------+-------------+-------------+-----------------------------------+
>> + | | | | return status codes |
>> + | | | | 0x100 indicates fit has been |
>> + | status | 4 | 0 | updated |
> 0x100 - error caused by NFIT update while read by _FIT wasn't completed
okay.
>
>> + | | | | other follows Chapter 3 in DSM |
> s/other follows/other codes follow/
okay.
>
>> + | | | | Spec Rev1 |
>> + +----------+-------------+-------------+-----------------------------------+
>> + | fit data | Varies | 4 | FIT data |
>> + | | | | |
>> + +----------+-------------+-------------+-----------------------------------+
> what does "Varies" mean, how would I know reading this how much data Read_FIT should read
> from shared page?
I mean other bytes in the buffer returned by this function except the 'status' field
will be used as fit data. Maybe it is has a 'length' field?
>
>> +
>> + The FIT offset is maintained by the caller itself,
> probably is not necessary sentence, or specify a caller (for example OSPM)
>
Okay.
>> current offset plugs
> ^^^^?
Typo. should be plus.
>
>> + the length returned by the function is the next offset we should read.
>> + When all the FIT data has been read out, zero length is returned.
>> +
>> + If it returns 0x100, OSPM should restart to read FIT (read from offset 0
>> + again).
> [...]
> that's all for doc part, I'll do the code part later.
>
Thank you, Igor.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-11-01 16:41 ` [Qemu-devel] " Stefan Hajnoczi
@ 2016-11-02 15:42 ` Xiao Guangrong
-1 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-02 15:42 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: pbonzini, imammedo, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On 11/02/2016 12:41 AM, Stefan Hajnoczi wrote:
> On Sat, Oct 29, 2016 at 12:35:39AM +0800, Xiao Guangrong wrote:
>> +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.
>> +
>> + Input parameters:
>> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
>> + Arg1 – Revision ID (set to 1)
>> + Arg2 - Function Index, 0x1
>> + Arg3 - A package containing a buffer whose layout is as follows:
>> +
>> + +----------+-------------+-------------+-----------------------------------+
>> + | Filed | Byte Length | Byte Offset | Description |
>
> s/Filed/Field/
>
> The same applies below too.
Will fix.
>
>> + +----------+-------------+-------------+-----------------------------------+
>> + | offset | 4 | 0 | the offset of FIT buffer |
>> + +----------+-------------+-------------+-----------------------------------+
>
> s/offset of FIT buffer/offset into FIT buffer/
will fix.
>
>> +
>> + 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
>
> s/plugs/plus/
Yes, indeed.
>
>> +struct NvdimmFuncReadFITIn {
>> + uint32_t offset; /* the offset of FIT buffer. */
>
> s/offset of FIT buffer/offset into FIT buffer/
will fix.
Thank you, Stefan!
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
@ 2016-11-02 15:42 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-02 15:42 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: pbonzini, imammedo, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On 11/02/2016 12:41 AM, Stefan Hajnoczi wrote:
> On Sat, Oct 29, 2016 at 12:35:39AM +0800, Xiao Guangrong wrote:
>> +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.
>> +
>> + Input parameters:
>> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
>> + Arg1 – Revision ID (set to 1)
>> + Arg2 - Function Index, 0x1
>> + Arg3 - A package containing a buffer whose layout is as follows:
>> +
>> + +----------+-------------+-------------+-----------------------------------+
>> + | Filed | Byte Length | Byte Offset | Description |
>
> s/Filed/Field/
>
> The same applies below too.
Will fix.
>
>> + +----------+-------------+-------------+-----------------------------------+
>> + | offset | 4 | 0 | the offset of FIT buffer |
>> + +----------+-------------+-------------+-----------------------------------+
>
> s/offset of FIT buffer/offset into FIT buffer/
will fix.
>
>> +
>> + 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
>
> s/plugs/plus/
Yes, indeed.
>
>> +struct NvdimmFuncReadFITIn {
>> + uint32_t offset; /* the offset of FIT buffer. */
>
> s/offset of FIT buffer/offset into FIT buffer/
will fix.
Thank you, Stefan!
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-11-02 13:56 ` Igor Mammedov
@ 2016-11-02 15:54 ` Xiao Guangrong
2016-11-03 9:22 ` Igor Mammedov
0 siblings, 1 reply; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-02 15:54 UTC (permalink / raw)
To: Igor Mammedov
Cc: pbonzini, ehabkost, kvm, mst, gleb, mtosatti, qemu-devel,
stefanha, dan.j.williams, rth
On 11/02/2016 09:56 PM, Igor Mammedov wrote:
> On Sat, 29 Oct 2016 00:35:39 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _FIT is required for hotplug support, guest will inquire the updated
>> device info from it if a hotplug event is received
>>
>> As FIT buffer is not completely mapped into guest address space, so a
>> new function, Read FIT whose UUID is UUID
>> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
>> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
>> is concatenated before _FIT return
>>
>> Refer to docs/specs/acpi-nvdimm.txt for detailed design
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
> [...]
>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 5f728a6..fc1a012 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -496,6 +496,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)
>> {
>> @@ -516,6 +532,74 @@ 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
>> +
>> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
>> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>> + hwaddr dsm_mem_addr)
>> +{
>> + NvdimmFitBuffer *fit_buf = &state->fit_buf;
>> + NvdimmFuncReadFITIn *read_fit;
>> + NvdimmFuncReadFITOut *read_fit_out;
>> + GArray *fit;
>> + uint32_t read_len = 0, func_ret_status;
>> + int size;
>> +
>> + read_fit = (NvdimmFuncReadFITIn *)in->arg3;
>> + le32_to_cpus(&read_fit->offset);
> I'd prefer if you'd not do inplace conversion, just do
> offset = le32_to_cpu(read_fit->offset);
okay.
>
>> +
>> + qemu_mutex_lock(&fit_buf->lock);
>> + fit = fit_buf->fit;
>> +
>> + nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
>> + read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> as follow up path replace nvdimm_debug() with trace events
I will do it as a separate patch.
>
>> +
>> + if (read_fit->offset > fit->len) {
>> + func_ret_status = 3 /* Invalid Input Parameters */;
> should be macros instead of magic value
Yes.
>
>> + goto exit;
>> + }
>> +
>> + /* It is the first time to read FIT. */
>> + 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 */;
> should be macros instead of magic value
okay.
>
>> + goto exit;
>> + }
>> +
>> + func_ret_status = 0 /* Success */;
>> + read_len = MIN(fit->len - read_fit->offset,
>> + 4096 - sizeof(NvdimmFuncReadFITOut));
>> +
>> +exit:
>> + size = sizeof(NvdimmFuncReadFITOut) + read_len;
>> + read_fit_out = g_malloc(size);
>> +
>> + read_fit_out->len = cpu_to_le32(size);
>> + read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
>> + memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
>> +
>> + cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
>> +
>> + g_free(read_fit_out);
>> + qemu_mutex_unlock(&fit_buf->lock);
>> +}
>> +
>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>> + hwaddr dsm_mem_addr)
>> +{
>> + switch (in->function) {
>> + case 0x0:
>> + nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
>> + return;
>> + case 0x1 /*Read FIT */:
>> + nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
>> + return;
>> + }
>> +
>> + nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> should be macros instead of magic value
Okay.
>
>> +}
>> +
>> static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>> {
>> /*
>> @@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
>> static void
>> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> {
>> + AcpiNVDIMMState *state = opaque;
>> NvdimmDsmIn *in;
>> hwaddr dsm_mem_addr = val;
>>
>> @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> goto exit;
>> }
>>
>> + if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
>> + nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
>> + goto exit;
>> + }
>> +
>> /* Handle 0 is reserved for NVDIMM Root Device. */
>> if (!in->handle) {
>> nvdimm_dsm_root(in, dsm_mem_addr);
>> @@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>> #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
>> #define NVDIMM_DSM_OUT_BUF "ODAT"
>>
>> +#define NVDIMM_DSM_RFIT_STATUS "RSTA"
>> +
>> +#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
>> +
>> static void nvdimm_build_common_dsm(Aml *dev)
>> {
>> - Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
>> + Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
>> Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
>> Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
>> uint8_t byte_list[1];
>> @@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev)
>> /* UUID for NVDIMM Root Device */, expected_uuid));
>> aml_append(method, ifctx);
>> elsectx = aml_else();
>> - aml_append(elsectx, aml_store(
>> + ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
>> + aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
>> + /* UUID for QEMU internal use */), expected_uuid));
>> + aml_append(elsectx, ifctx);
>> + elsectx2 = aml_else();
>> + aml_append(elsectx2, aml_store(
>> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
>> /* UUID for NVDIMM Devices */, expected_uuid));
>> + aml_append(elsectx, elsectx2);
>> aml_append(method, elsectx);
>>
>> uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
>> @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>> aml_append(dev, method);
>> }
>>
>> +static void nvdimm_build_fit(Aml *dev)
> nvdimm_build_fit_method()
okay.
>
>> +{
>> + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
>> + Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
>> +
>> + buf = aml_local(0);
>> + buf_size = aml_local(1);
>> + fit = aml_local(2);
>> +
>> + aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL),
>> + aml_int(0), NVDIMM_DSM_RFIT_STATUS));
> it doesn't have to be buffer as it's internal ASL integer object
> so it could be just named variable.
Let me try.
>
> I'd also move it to _FIT method instead of making it device global
We can not as it is used both in _FIT method and RFIT method.
>
> and if it could work try to pass it as argument to RFIT
> RefOf/DerefOf may help here
> or make return value instead of buffer and pass buffer as reference.
>
Let me try.
> Alternatively you can return buffer from RFIT with status field included
> and check/discard status value there.
>
As we can not create name object in a while-loop, it is not easy to check
the status in _FIT.
>> +
>> + /* 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"));
>> +
>> + /* prepare input package. */
>> + pkg = aml_package(1);
>> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
>> + aml_append(pkg, aml_name("OFST"));
>> +
>> + /* call Read_FIT function. */
>> + call_result = aml_call5(NVDIMM_COMMON_DSM,
>> + aml_touuid(NVDIMM_QEMU_RSVD_UUID),
>> + aml_int(1) /* Revision 1 */,
>> + aml_int(0x1) /* Read FIT */,
>> + pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
>> + aml_append(method, aml_store(call_result, buf));
>> +
>> + /* handle _DSM result. */
>> + aml_append(method, aml_create_dword_field(buf,
>> + aml_int(0) /* offset at byte 0 */, "STAU"));
>> +
>> + aml_append(method, aml_store(aml_name("STAU"),
>> + aml_name(NVDIMM_DSM_RFIT_STATUS)));
>> +
>> + /* if something is wrong during _DSM. */
>> + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
>> + ifctx = aml_if(aml_lnot(ifcond));
>> + aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
>> + aml_append(method, ifctx);
>> +
>> + aml_append(method, aml_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));
> there isn't need to convert bytes to bits and store it in the same variable
> it creates confusion
Okay, i will introduce a new variable named buf_size_bits.
>
>> + aml_append(method, aml_create_field(buf,
>> + aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
>> + buf_size, "BUFF"));
> just inline conversion in aml_create_field()
okay.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug
2016-11-02 11:21 ` [Qemu-devel] " Igor Mammedov
@ 2016-11-02 15:55 ` Xiao Guangrong
-1 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-02 15:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On 11/02/2016 07:21 PM, Igor Mammedov wrote:
> On Sat, 29 Oct 2016 00:35:40 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _GPE.E04 is dedicated for nvdimm device hotplug
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>> docs/specs/acpi_mem_hotplug.txt | 3 +++
>> hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++--------
>> hw/i386/acpi-build.c | 7 +++++++
>> hw/i386/pc.c | 12 ++++++++++++
>> hw/mem/nvdimm.c | 4 ----
>> include/hw/acpi/acpi_dev_interface.h | 1 +
>> 6 files changed, 46 insertions(+), 12 deletions(-)
>>
>> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
>> index 3df3620..cb26dd2 100644
>> --- a/docs/specs/acpi_mem_hotplug.txt
>> +++ b/docs/specs/acpi_mem_hotplug.txt
>> @@ -4,6 +4,9 @@ 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.
> that should be part of nvdimm spec
Okay.
>
>> Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>> ---------------------------------------------------------------
>> 0xa00:
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index ec4e64b..70f6451 100644
>> --- a/hw/acpi/memory_hotplug.c
>> +++ b/hw/acpi/memory_hotplug.c
> so far nvdimm hotplug has nothing do with memory hotplug so revert
> all changes you did to this file and put nvdimm hotplug handler
> code into hw/acpi/nvdimm.c
>
> and call it from respective ich9/piix4_pm handlers.
Okay.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug
@ 2016-11-02 15:55 ` Xiao Guangrong
0 siblings, 0 replies; 40+ messages in thread
From: Xiao Guangrong @ 2016-11-02 15:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On 11/02/2016 07:21 PM, Igor Mammedov wrote:
> On Sat, 29 Oct 2016 00:35:40 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _GPE.E04 is dedicated for nvdimm device hotplug
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>> docs/specs/acpi_mem_hotplug.txt | 3 +++
>> hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++--------
>> hw/i386/acpi-build.c | 7 +++++++
>> hw/i386/pc.c | 12 ++++++++++++
>> hw/mem/nvdimm.c | 4 ----
>> include/hw/acpi/acpi_dev_interface.h | 1 +
>> 6 files changed, 46 insertions(+), 12 deletions(-)
>>
>> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
>> index 3df3620..cb26dd2 100644
>> --- a/docs/specs/acpi_mem_hotplug.txt
>> +++ b/docs/specs/acpi_mem_hotplug.txt
>> @@ -4,6 +4,9 @@ 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.
> that should be part of nvdimm spec
Okay.
>
>> Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>> ---------------------------------------------------------------
>> 0xa00:
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index ec4e64b..70f6451 100644
>> --- a/hw/acpi/memory_hotplug.c
>> +++ b/hw/acpi/memory_hotplug.c
> so far nvdimm hotplug has nothing do with memory hotplug so revert
> all changes you did to this file and put nvdimm hotplug handler
> code into hw/acpi/nvdimm.c
>
> and call it from respective ich9/piix4_pm handlers.
Okay.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] nvdimm: hotplug support
2016-11-02 14:01 ` [Qemu-devel] [PATCH v3 0/4] nvdimm: hotplug support Igor Mammedov
@ 2016-11-03 4:53 ` Michael S. Tsirkin
2016-11-03 9:27 ` Igor Mammedov
0 siblings, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2016-11-03 4:53 UTC (permalink / raw)
To: Igor Mammedov
Cc: Xiao Guangrong, pbonzini, ehabkost, kvm, gleb, mtosatti,
qemu-devel, stefanha, dan.j.williams, rth
On Wed, Nov 02, 2016 at 03:01:33PM +0100, Igor Mammedov wrote:
> On Sat, 29 Oct 2016 00:35:36 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
> > It is based on my previous patchset,
> > "[PATCH 0/8] nvdimm acpi: bug fix and cleanup", these two patchset are
> > against commit dea651a95af6dad099 (intel-iommu: Check IOAPIC's Trigger Mode
> > against the one in IRTE) on pci branch of Michael's git tree and can be
> > found at:
> > https://github.com/xiaogr/qemu.git nvdimm-hotplug-v3
> >
> > Changelog in v3:
> > 1) use a dedicated interrupt for nvdimm device hotplug
> > 2) stop nvdimm device hot unplug
> > 3) reserve UUID and handle for QEMU internally used QEMU
> > 5) redesign fit buffer to avoid OSPM reading incomplete fit info
> > 6) bug fixes and cleanups
> >
> > Changelog in v2:
> > Fixed signed integer overflow pointed out by Stefan Hajnoczi
> >
> > This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> > for example, a new nvdimm device can be plugged as follows:
> > object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> > device_add nvdimm,id=nvdimm3,memdev=mem3
> >
> > and unplug it as follows:
> > device_del nvdimm3
> > object_del mem3
> there is no unplug support
>
>
> instead of incremental fixups on top merged patches in followup series,
> I'd prefer it to make a clean revert for patches 2-4/4 first and
> them amended versions of them to follow.
Let's get the fixes reviewed first, how to apply them is
a minor issue in my eyes.
> >
> > Xiao Guangrong (4):
> > nvdimm acpi: prebuild nvdimm devices for available slots
> > nvdimm acpi: introduce fit buffer
> > nvdimm acpi: introduce _FIT
> > pc: memhp: enable nvdimm device hotplug
> >
> > docs/specs/acpi_mem_hotplug.txt | 3 +
> > docs/specs/acpi_nvdimm.txt | 58 ++++++-
> > hw/acpi/memory_hotplug.c | 31 +++-
> > hw/acpi/nvdimm.c | 286 +++++++++++++++++++++++++++++++----
> > hw/core/hotplug.c | 11 ++
> > hw/core/qdev.c | 20 ++-
> > hw/i386/acpi-build.c | 9 +-
> > hw/i386/pc.c | 31 ++++
> > hw/mem/nvdimm.c | 4 -
> > include/hw/acpi/acpi_dev_interface.h | 1 +
> > include/hw/hotplug.h | 10 ++
> > include/hw/mem/nvdimm.h | 27 +++-
> > 12 files changed, 443 insertions(+), 48 deletions(-)
> >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-11-02 15:40 ` [Qemu-devel] " Xiao Guangrong
@ 2016-11-03 9:15 ` Igor Mammedov
-1 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-03 9:15 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On Wed, 2 Nov 2016 23:40:56 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> On 11/02/2016 12:24 AM, Igor Mammedov wrote:
> > On Sat, 29 Oct 2016 00:35:39 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> >> _FIT is required for hotplug support, guest will inquire the updated
> >> device info from it if a hotplug event is received
> >>
> >> As FIT buffer is not completely mapped into guest address space, so a
> >> new function, Read FIT whose UUID is UUID
> >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> >> is concatenated before _FIT return
> >>
> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> >>
> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> ---
> >> docs/specs/acpi_nvdimm.txt | 58 ++++++++++++-
> >> hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++-
> >> 2 files changed, 257 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> >> index 0fdd251..4aa5e3d 100644
> >> --- a/docs/specs/acpi_nvdimm.txt
> >> +++ b/docs/specs/acpi_nvdimm.txt
> >> @@ -127,6 +127,58 @@ _DSM process diagram:
> >> | result from the page | | |
> >> +--------------------------+ +--------------+
> >>
> >> - _FIT implementation
> >> - -------------------
> >> - TODO (will fill it when nvdimm hotplug is introduced)
> >> +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.
> > Above part should go to section where 'handle' is defined, i.e. earlier in the file:
> >
> > 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.
> >
>
> Okay.
>
> >
> >> +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
> > UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function
> > (private QEMU function)
> >
>
> okay.
>
> >> + 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 Read_FIT function to fetch NFIT structures blob from QEMU
> > in 1 page sized increments which are then concatenated and returned as _FIT method result.
> >
>
> okay.
>
> >
> >> +
> >> + Input parameters:
> >> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> >> + Arg1 – Revision ID (set to 1)
> >> + Arg2 - Function Index, 0x1
> >> + Arg3 - A package containing a buffer whose layout is as follows:
> >> +
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | Filed | Byte Length | Byte Offset | Description |
> > ^ field, s/Byte//, s/Byte//
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | offset | 4 | 0 | the offset of FIT buffer |
> > offset in QEMU's NFIT structures blob to read from
>
> okay.
>
> >
> >> + +----------+-------------+-------------+-----------------------------------+
> >> +
> >> + Output:
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | Filed | Byte Length | Byte Offset | Description |
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | | | | return status codes |
> >> + | | | | 0x100 indicates fit has been |
> >> + | status | 4 | 0 | updated |
> > 0x100 - error caused by NFIT update while read by _FIT wasn't completed
>
> okay.
>
> >
> >> + | | | | other follows Chapter 3 in DSM |
> > s/other follows/other codes follow/
>
> okay.
>
> >
> >> + | | | | Spec Rev1 |
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | fit data | Varies | 4 | FIT data |
> >> + | | | | |
> >> + +----------+-------------+-------------+-----------------------------------+
> > what does "Varies" mean, how would I know reading this how much data Read_FIT should read
> > from shared page?
>
> I mean other bytes in the buffer returned by this function except the 'status' field
> will be used as fit data. Maybe it is has a 'length' field?
you fill in length in C and access it in AML code and it's a part of output buffer format
so document it properly here.
>
> >
> >> +
> >> + The FIT offset is maintained by the caller itself,
> > probably is not necessary sentence, or specify a caller (for example OSPM)
> >
>
> Okay.
>
> >> current offset plugs
> > ^^^^?
>
> Typo. should be plus.
>
> >
> >> + the length returned by the function is the next offset we should read.
> >> + When all the FIT data has been read out, zero length is returned.
> >> +
> >> + If it returns 0x100, OSPM should restart to read FIT (read from offset 0
> >> + again).
> > [...]
> > that's all for doc part, I'll do the code part later.
> >
>
> Thank you, Igor.
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
@ 2016-11-03 9:15 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-03 9:15 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost,
dan.j.williams, kvm, qemu-devel
On Wed, 2 Nov 2016 23:40:56 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> On 11/02/2016 12:24 AM, Igor Mammedov wrote:
> > On Sat, 29 Oct 2016 00:35:39 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> >> _FIT is required for hotplug support, guest will inquire the updated
> >> device info from it if a hotplug event is received
> >>
> >> As FIT buffer is not completely mapped into guest address space, so a
> >> new function, Read FIT whose UUID is UUID
> >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> >> is concatenated before _FIT return
> >>
> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> >>
> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> ---
> >> docs/specs/acpi_nvdimm.txt | 58 ++++++++++++-
> >> hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++-
> >> 2 files changed, 257 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> >> index 0fdd251..4aa5e3d 100644
> >> --- a/docs/specs/acpi_nvdimm.txt
> >> +++ b/docs/specs/acpi_nvdimm.txt
> >> @@ -127,6 +127,58 @@ _DSM process diagram:
> >> | result from the page | | |
> >> +--------------------------+ +--------------+
> >>
> >> - _FIT implementation
> >> - -------------------
> >> - TODO (will fill it when nvdimm hotplug is introduced)
> >> +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.
> > Above part should go to section where 'handle' is defined, i.e. earlier in the file:
> >
> > 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.
> >
>
> Okay.
>
> >
> >> +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
> > UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function
> > (private QEMU function)
> >
>
> okay.
>
> >> + 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 Read_FIT function to fetch NFIT structures blob from QEMU
> > in 1 page sized increments which are then concatenated and returned as _FIT method result.
> >
>
> okay.
>
> >
> >> +
> >> + Input parameters:
> >> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> >> + Arg1 – Revision ID (set to 1)
> >> + Arg2 - Function Index, 0x1
> >> + Arg3 - A package containing a buffer whose layout is as follows:
> >> +
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | Filed | Byte Length | Byte Offset | Description |
> > ^ field, s/Byte//, s/Byte//
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | offset | 4 | 0 | the offset of FIT buffer |
> > offset in QEMU's NFIT structures blob to read from
>
> okay.
>
> >
> >> + +----------+-------------+-------------+-----------------------------------+
> >> +
> >> + Output:
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | Filed | Byte Length | Byte Offset | Description |
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | | | | return status codes |
> >> + | | | | 0x100 indicates fit has been |
> >> + | status | 4 | 0 | updated |
> > 0x100 - error caused by NFIT update while read by _FIT wasn't completed
>
> okay.
>
> >
> >> + | | | | other follows Chapter 3 in DSM |
> > s/other follows/other codes follow/
>
> okay.
>
> >
> >> + | | | | Spec Rev1 |
> >> + +----------+-------------+-------------+-----------------------------------+
> >> + | fit data | Varies | 4 | FIT data |
> >> + | | | | |
> >> + +----------+-------------+-------------+-----------------------------------+
> > what does "Varies" mean, how would I know reading this how much data Read_FIT should read
> > from shared page?
>
> I mean other bytes in the buffer returned by this function except the 'status' field
> will be used as fit data. Maybe it is has a 'length' field?
you fill in length in C and access it in AML code and it's a part of output buffer format
so document it properly here.
>
> >
> >> +
> >> + The FIT offset is maintained by the caller itself,
> > probably is not necessary sentence, or specify a caller (for example OSPM)
> >
>
> Okay.
>
> >> current offset plugs
> > ^^^^?
>
> Typo. should be plus.
>
> >
> >> + the length returned by the function is the next offset we should read.
> >> + When all the FIT data has been read out, zero length is returned.
> >> +
> >> + If it returns 0x100, OSPM should restart to read FIT (read from offset 0
> >> + again).
> > [...]
> > that's all for doc part, I'll do the code part later.
> >
>
> Thank you, Igor.
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT
2016-11-02 15:54 ` Xiao Guangrong
@ 2016-11-03 9:22 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-03 9:22 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, ehabkost, kvm, mst, gleb, mtosatti, qemu-devel,
stefanha, dan.j.williams, rth
On Wed, 2 Nov 2016 23:54:05 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> On 11/02/2016 09:56 PM, Igor Mammedov wrote:
> > On Sat, 29 Oct 2016 00:35:39 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> >> _FIT is required for hotplug support, guest will inquire the updated
> >> device info from it if a hotplug event is received
> >>
> >> As FIT buffer is not completely mapped into guest address space, so a
> >> new function, Read FIT whose UUID is UUID
> >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> >> is concatenated before _FIT return
> >>
> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> >>
> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> ---
> > [...]
> >
> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> >> index 5f728a6..fc1a012 100644
> >> --- a/hw/acpi/nvdimm.c
> >> +++ b/hw/acpi/nvdimm.c
> >> @@ -496,6 +496,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)
> >> {
> >> @@ -516,6 +532,74 @@ 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
> >> +
> >> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
> >> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> >> + hwaddr dsm_mem_addr)
> >> +{
> >> + NvdimmFitBuffer *fit_buf = &state->fit_buf;
> >> + NvdimmFuncReadFITIn *read_fit;
> >> + NvdimmFuncReadFITOut *read_fit_out;
> >> + GArray *fit;
> >> + uint32_t read_len = 0, func_ret_status;
> >> + int size;
> >> +
> >> + read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> >> + le32_to_cpus(&read_fit->offset);
> > I'd prefer if you'd not do inplace conversion, just do
> > offset = le32_to_cpu(read_fit->offset);
>
> okay.
>
> >
> >> +
> >> + qemu_mutex_lock(&fit_buf->lock);
> >> + fit = fit_buf->fit;
> >> +
> >> + nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
> >> + read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> > as follow up path replace nvdimm_debug() with trace events
>
> I will do it as a separate patch.
>
> >
> >> +
> >> + if (read_fit->offset > fit->len) {
> >> + func_ret_status = 3 /* Invalid Input Parameters */;
> > should be macros instead of magic value
>
> Yes.
>
> >
> >> + goto exit;
> >> + }
> >> +
> >> + /* It is the first time to read FIT. */
> >> + 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 */;
> > should be macros instead of magic value
>
> okay.
>
> >
> >> + goto exit;
> >> + }
> >> +
> >> + func_ret_status = 0 /* Success */;
> >> + read_len = MIN(fit->len - read_fit->offset,
> >> + 4096 - sizeof(NvdimmFuncReadFITOut));
> >> +
> >> +exit:
> >> + size = sizeof(NvdimmFuncReadFITOut) + read_len;
> >> + read_fit_out = g_malloc(size);
> >> +
> >> + read_fit_out->len = cpu_to_le32(size);
> >> + read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
> >> + memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
> >> +
> >> + cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
> >> +
> >> + g_free(read_fit_out);
> >> + qemu_mutex_unlock(&fit_buf->lock);
> >> +}
> >> +
> >> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> >> + hwaddr dsm_mem_addr)
> >> +{
> >> + switch (in->function) {
> >> + case 0x0:
> >> + nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
> >> + return;
> >> + case 0x1 /*Read FIT */:
> >> + nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
> >> + return;
> >> + }
> >> +
> >> + nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> > should be macros instead of magic value
>
> Okay.
>
> >
> >> +}
> >> +
> >> static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> >> {
> >> /*
> >> @@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> >> static void
> >> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> >> {
> >> + AcpiNVDIMMState *state = opaque;
> >> NvdimmDsmIn *in;
> >> hwaddr dsm_mem_addr = val;
> >>
> >> @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> >> goto exit;
> >> }
> >>
> >> + if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> >> + nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
> >> + goto exit;
> >> + }
> >> +
> >> /* Handle 0 is reserved for NVDIMM Root Device. */
> >> if (!in->handle) {
> >> nvdimm_dsm_root(in, dsm_mem_addr);
> >> @@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
> >> #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
> >> #define NVDIMM_DSM_OUT_BUF "ODAT"
> >>
> >> +#define NVDIMM_DSM_RFIT_STATUS "RSTA"
> >> +
> >> +#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
> >> +
> >> static void nvdimm_build_common_dsm(Aml *dev)
> >> {
> >> - Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
> >> + Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
> >> Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
> >> Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
> >> uint8_t byte_list[1];
> >> @@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev)
> >> /* UUID for NVDIMM Root Device */, expected_uuid));
> >> aml_append(method, ifctx);
> >> elsectx = aml_else();
> >> - aml_append(elsectx, aml_store(
> >> + ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
> >> + aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
> >> + /* UUID for QEMU internal use */), expected_uuid));
> >> + aml_append(elsectx, ifctx);
> >> + elsectx2 = aml_else();
> >> + aml_append(elsectx2, aml_store(
> >> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
> >> /* UUID for NVDIMM Devices */, expected_uuid));
> >> + aml_append(elsectx, elsectx2);
> >> aml_append(method, elsectx);
> >>
> >> uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
> >> @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
> >> aml_append(dev, method);
> >> }
> >>
> >> +static void nvdimm_build_fit(Aml *dev)
> > nvdimm_build_fit_method()
>
> okay.
>
> >
> >> +{
> >> + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
> >> + Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
> >> +
> >> + buf = aml_local(0);
> >> + buf_size = aml_local(1);
> >> + fit = aml_local(2);
> >> +
> >> + aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL),
> >> + aml_int(0), NVDIMM_DSM_RFIT_STATUS));
> > it doesn't have to be buffer as it's internal ASL integer object
> > so it could be just named variable.
>
> Let me try.
>
> >
> > I'd also move it to _FIT method instead of making it device global
>
> We can not as it is used both in _FIT method and RFIT method.
>
> >
> > and if it could work try to pass it as argument to RFIT
> > RefOf/DerefOf may help here
> > or make return value instead of buffer and pass buffer as reference.
> >
>
> Let me try.
>
> > Alternatively you can return buffer from RFIT with status field included
> > and check/discard status value there.
> >
>
> As we can not create name object in a while-loop, it is not easy to check
> the status in _FIT.
index() method might help there
or even better create a separate method to extract dword from buffer
and call it from all other places where you do similar thing
instead of creating a bunch of fields.
>
> >> +
> >> + /* 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"));
> >> +
> >> + /* prepare input package. */
> >> + pkg = aml_package(1);
> >> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
> >> + aml_append(pkg, aml_name("OFST"));
> >> +
> >> + /* call Read_FIT function. */
> >> + call_result = aml_call5(NVDIMM_COMMON_DSM,
> >> + aml_touuid(NVDIMM_QEMU_RSVD_UUID),
> >> + aml_int(1) /* Revision 1 */,
> >> + aml_int(0x1) /* Read FIT */,
> >> + pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
> >> + aml_append(method, aml_store(call_result, buf));
> >> +
> >> + /* handle _DSM result. */
> >> + aml_append(method, aml_create_dword_field(buf,
> >> + aml_int(0) /* offset at byte 0 */, "STAU"));
> >> +
> >> + aml_append(method, aml_store(aml_name("STAU"),
> >> + aml_name(NVDIMM_DSM_RFIT_STATUS)));
> >> +
> >> + /* if something is wrong during _DSM. */
> >> + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
> >> + ifctx = aml_if(aml_lnot(ifcond));
> >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> >> + aml_append(method, ifctx);
> >> +
> >> + aml_append(method, aml_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));
> > there isn't need to convert bytes to bits and store it in the same variable
> > it creates confusion
>
> Okay, i will introduce a new variable named buf_size_bits.
>
> >
> >> + aml_append(method, aml_create_field(buf,
> >> + aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
> >> + buf_size, "BUFF"));
> > just inline conversion in aml_create_field()
>
> okay.
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] nvdimm: hotplug support
2016-11-03 4:53 ` Michael S. Tsirkin
@ 2016-11-03 9:27 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2016-11-03 9:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Xiao Guangrong, pbonzini, ehabkost, kvm, gleb, mtosatti,
qemu-devel, stefanha, dan.j.williams, rth
On Thu, 3 Nov 2016 06:53:33 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Nov 02, 2016 at 03:01:33PM +0100, Igor Mammedov wrote:
> > On Sat, 29 Oct 2016 00:35:36 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >
> > > It is based on my previous patchset,
> > > "[PATCH 0/8] nvdimm acpi: bug fix and cleanup", these two patchset are
> > > against commit dea651a95af6dad099 (intel-iommu: Check IOAPIC's Trigger Mode
> > > against the one in IRTE) on pci branch of Michael's git tree and can be
> > > found at:
> > > https://github.com/xiaogr/qemu.git nvdimm-hotplug-v3
> > >
> > > Changelog in v3:
> > > 1) use a dedicated interrupt for nvdimm device hotplug
> > > 2) stop nvdimm device hot unplug
> > > 3) reserve UUID and handle for QEMU internally used QEMU
> > > 5) redesign fit buffer to avoid OSPM reading incomplete fit info
> > > 6) bug fixes and cleanups
> > >
> > > Changelog in v2:
> > > Fixed signed integer overflow pointed out by Stefan Hajnoczi
> > >
> > > This patchset enables nvdimm hotplug support, it is used as pc-dimm hotplug,
> > > for example, a new nvdimm device can be plugged as follows:
> > > object_add memory-backend-file,id=mem3,size=10G,mem-path=/home/eric/nvdimm3
> > > device_add nvdimm,id=nvdimm3,memdev=mem3
> > >
> > > and unplug it as follows:
> > > device_del nvdimm3
> > > object_del mem3
> > there is no unplug support
> >
> >
> > instead of incremental fixups on top merged patches in followup series,
> > I'd prefer it to make a clean revert for patches 2-4/4 first and
> > them amended versions of them to follow.
>
> Let's get the fixes reviewed first, how to apply them is
> a minor issue in my eyes.
Incremental fixes would be a pain to review,
clean revert + rewritten patches on top should make review
much easier as one won't have to compare 3 variants
of code.
>
> > >
> > > Xiao Guangrong (4):
> > > nvdimm acpi: prebuild nvdimm devices for available slots
> > > nvdimm acpi: introduce fit buffer
> > > nvdimm acpi: introduce _FIT
> > > pc: memhp: enable nvdimm device hotplug
> > >
> > > docs/specs/acpi_mem_hotplug.txt | 3 +
> > > docs/specs/acpi_nvdimm.txt | 58 ++++++-
> > > hw/acpi/memory_hotplug.c | 31 +++-
> > > hw/acpi/nvdimm.c | 286 +++++++++++++++++++++++++++++++----
> > > hw/core/hotplug.c | 11 ++
> > > hw/core/qdev.c | 20 ++-
> > > hw/i386/acpi-build.c | 9 +-
> > > hw/i386/pc.c | 31 ++++
> > > hw/mem/nvdimm.c | 4 -
> > > include/hw/acpi/acpi_dev_interface.h | 1 +
> > > include/hw/hotplug.h | 10 ++
> > > include/hw/mem/nvdimm.h | 27 +++-
> > > 12 files changed, 443 insertions(+), 48 deletions(-)
> > >
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2016-11-03 9:27 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-28 16:35 [PATCH v3 0/4] nvdimm: hotplug support Xiao Guangrong
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
2016-10-28 16:35 ` [PATCH v3 1/4] nvdimm acpi: prebuild nvdimm devices for available slots Xiao Guangrong
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
2016-11-01 15:16 ` Igor Mammedov
2016-11-01 15:16 ` [Qemu-devel] " Igor Mammedov
2016-10-28 16:35 ` [PATCH v3 2/4] nvdimm acpi: introduce fit buffer Xiao Guangrong
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
2016-11-01 15:17 ` Stefan Hajnoczi
2016-11-01 15:17 ` [Qemu-devel] " Stefan Hajnoczi
2016-11-01 15:25 ` Igor Mammedov
2016-11-01 15:25 ` [Qemu-devel] " Igor Mammedov
2016-11-01 16:00 ` Xiao Guangrong
2016-11-01 16:00 ` [Qemu-devel] " Xiao Guangrong
2016-10-28 16:35 ` [PATCH v3 3/4] nvdimm acpi: introduce _FIT Xiao Guangrong
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
2016-11-01 16:24 ` Igor Mammedov
2016-11-01 16:24 ` [Qemu-devel] " Igor Mammedov
2016-11-02 15:40 ` Xiao Guangrong
2016-11-02 15:40 ` [Qemu-devel] " Xiao Guangrong
2016-11-03 9:15 ` Igor Mammedov
2016-11-03 9:15 ` [Qemu-devel] " Igor Mammedov
2016-11-01 16:41 ` Stefan Hajnoczi
2016-11-01 16:41 ` [Qemu-devel] " Stefan Hajnoczi
2016-11-02 15:42 ` Xiao Guangrong
2016-11-02 15:42 ` [Qemu-devel] " Xiao Guangrong
2016-11-02 13:56 ` Igor Mammedov
2016-11-02 15:54 ` Xiao Guangrong
2016-11-03 9:22 ` Igor Mammedov
2016-10-28 16:35 ` [PATCH v3 4/4] pc: memhp: enable nvdimm device hotplug Xiao Guangrong
2016-10-28 16:35 ` [Qemu-devel] " Xiao Guangrong
2016-11-01 16:44 ` Stefan Hajnoczi
2016-11-01 16:44 ` [Qemu-devel] " Stefan Hajnoczi
2016-11-02 11:21 ` Igor Mammedov
2016-11-02 11:21 ` [Qemu-devel] " Igor Mammedov
2016-11-02 15:55 ` Xiao Guangrong
2016-11-02 15:55 ` [Qemu-devel] " Xiao Guangrong
2016-11-02 14:01 ` [Qemu-devel] [PATCH v3 0/4] nvdimm: hotplug support Igor Mammedov
2016-11-03 4:53 ` Michael S. Tsirkin
2016-11-03 9:27 ` Igor Mammedov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.