* [Qemu-devel] [PATCH v3 0/2] machine: Move nvdimms state into struct MachineState
@ 2019-03-08 15:25 Eric Auger
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 1/2] nvdimm: Rename AcpiNVDIMMState into NVDIMMState Eric Auger
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState Eric Auger
0 siblings, 2 replies; 9+ messages in thread
From: Eric Auger @ 2019-03-08 15:25 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
shameerali.kolothum.thodi, imammedo, david, pbonzini, ehabkost,
richard.henderson, sbhat, philmd
As NVDIMM support is looming for ARM and SPAPR, let's
move the acpi_nvdimm_state to the generic machine struct
instead of duplicating the same code in several machines.
nvdimm and nvdimm-persistence become generic machine options.
They become guarded by a nvdimm_supported machine class member.
Best regards
Eric
https://github.com/eauger/qemu/tree/nvdimms_state_v3
Eric Auger (2):
nvdimm: Rename AcpiNVDIMMState into NVDIMMState
machine: Move nvdimms state into struct MachineState
hw/acpi/nvdimm.c | 18 ++++++------
hw/core/machine.c | 65 +++++++++++++++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 6 ++--
hw/i386/pc.c | 57 ++++--------------------------------
hw/i386/pc_piix.c | 4 +--
hw/i386/pc_q35.c | 4 +--
include/hw/boards.h | 2 ++
include/hw/i386/pc.h | 4 ---
include/hw/mem/nvdimm.h | 10 +++----
9 files changed, 93 insertions(+), 77 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v3 1/2] nvdimm: Rename AcpiNVDIMMState into NVDIMMState
2019-03-08 15:25 [Qemu-devel] [PATCH v3 0/2] machine: Move nvdimms state into struct MachineState Eric Auger
@ 2019-03-08 15:25 ` Eric Auger
2019-03-08 17:59 ` Philippe Mathieu-Daudé
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState Eric Auger
1 sibling, 1 reply; 9+ messages in thread
From: Eric Auger @ 2019-03-08 15:25 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
shameerali.kolothum.thodi, imammedo, david, pbonzini, ehabkost,
richard.henderson, sbhat, philmd
As we intend to migrate the acpi_nvdimm_state into
the base machine with a new dimms_state name, let's
also rename the datatype.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/nvdimm.c | 18 +++++++++---------
hw/i386/pc.c | 2 +-
include/hw/i386/pc.h | 2 +-
include/hw/mem/nvdimm.h | 10 +++++-----
4 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index e53b2cb681..f73cfb9d90 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -382,7 +382,7 @@ nvdimm_build_structure_caps(GArray *structures, uint32_t capabilities)
nfit_caps->capabilities = cpu_to_le32(capabilities);
}
-static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
+static GArray *nvdimm_build_device_structure(NVDIMMState *state)
{
GSList *device_list = nvdimm_get_device_list();
GArray *structures = g_array_new(false, true /* clear */, 1);
@@ -416,7 +416,7 @@ static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
fit_buf->fit = g_array_new(false, true /* clear */, 1);
}
-static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
+static void nvdimm_build_fit_buffer(NVDIMMState *state)
{
NvdimmFitBuffer *fit_buf = &state->fit_buf;
@@ -425,12 +425,12 @@ static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
fit_buf->dirty = true;
}
-void nvdimm_plug(AcpiNVDIMMState *state)
+void nvdimm_plug(NVDIMMState *state)
{
nvdimm_build_fit_buffer(state);
}
-static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
+static void nvdimm_build_nfit(NVDIMMState *state, GArray *table_offsets,
GArray *table_data, BIOSLinker *linker)
{
NvdimmFitBuffer *fit_buf = &state->fit_buf;
@@ -570,7 +570,7 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
#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,
+static void nvdimm_dsm_func_read_fit(NVDIMMState *state, NvdimmDsmIn *in,
hwaddr dsm_mem_addr)
{
NvdimmFitBuffer *fit_buf = &state->fit_buf;
@@ -619,7 +619,7 @@ exit:
}
static void
-nvdimm_dsm_handle_reserved_root_method(AcpiNVDIMMState *state,
+nvdimm_dsm_handle_reserved_root_method(NVDIMMState *state,
NvdimmDsmIn *in, hwaddr dsm_mem_addr)
{
switch (in->function) {
@@ -863,7 +863,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;
+ NVDIMMState *state = opaque;
NvdimmDsmIn *in;
hwaddr dsm_mem_addr = val;
@@ -925,7 +925,7 @@ void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
}
}
-void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
+void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
FWCfgState *fw_cfg, Object *owner)
{
memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
@@ -1319,7 +1319,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
}
void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
- BIOSLinker *linker, AcpiNVDIMMState *state,
+ BIOSLinker *linker, NVDIMMState *state,
uint32_t ram_slots)
{
GSList *device_list;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 42128183e9..0338dbe9da 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2571,7 +2571,7 @@ static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
Error **errp)
{
PCMachineState *pcms = PC_MACHINE(obj);
- AcpiNVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
+ NVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
if (strcmp(value, "cpu") == 0)
nvdimm_state->persistence = 3;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 54222a202d..94fb620d65 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -45,7 +45,7 @@ struct PCMachineState {
OnOffAuto vmport;
OnOffAuto smm;
- AcpiNVDIMMState acpi_nvdimm_state;
+ NVDIMMState acpi_nvdimm_state;
bool acpi_build_enabled;
bool smbus_enabled;
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index c5c9b3c7f8..523a9b3d4a 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -123,7 +123,7 @@ struct NvdimmFitBuffer {
};
typedef struct NvdimmFitBuffer NvdimmFitBuffer;
-struct AcpiNVDIMMState {
+struct NVDIMMState {
/* detect if NVDIMM support is enabled. */
bool is_enabled;
@@ -141,13 +141,13 @@ struct AcpiNVDIMMState {
int32_t persistence;
char *persistence_string;
};
-typedef struct AcpiNVDIMMState AcpiNVDIMMState;
+typedef struct NVDIMMState NVDIMMState;
-void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
+void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
FWCfgState *fw_cfg, Object *owner);
void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
- BIOSLinker *linker, AcpiNVDIMMState *state,
+ BIOSLinker *linker, NVDIMMState *state,
uint32_t ram_slots);
-void nvdimm_plug(AcpiNVDIMMState *state);
+void nvdimm_plug(NVDIMMState *state);
void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState
2019-03-08 15:25 [Qemu-devel] [PATCH v3 0/2] machine: Move nvdimms state into struct MachineState Eric Auger
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 1/2] nvdimm: Rename AcpiNVDIMMState into NVDIMMState Eric Auger
@ 2019-03-08 15:25 ` Eric Auger
2019-03-08 17:42 ` Eduardo Habkost
2019-03-08 18:02 ` Philippe Mathieu-Daudé
1 sibling, 2 replies; 9+ messages in thread
From: Eric Auger @ 2019-03-08 15:25 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
shameerali.kolothum.thodi, imammedo, david, pbonzini, ehabkost,
richard.henderson, sbhat, philmd
As NVDIMM support is looming for ARM and SPAPR, let's
move the acpi_nvdimm_state to the generic machine struct
instead of duplicating the same code in several machines.
It is also renamed into nvdimms_state and becomes a pointer.
nvdimm and nvdimm-persistence become generic machine options.
They become guarded by a nvdimm_supported machine class member.
We also add a description for those options.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
---
v2 -> v3
- nvdimms_state becomes a pointer
- add machine class nvdimm_supported
v1 -> v2:
- s/acpi_nvdimm_state/nvdimms_state
- remove ms->nvdimms_state.is_enabled initialization to false.
---
hw/core/machine.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 6 ++--
hw/i386/pc.c | 57 ++++----------------------------------
hw/i386/pc_piix.c | 4 +--
hw/i386/pc_q35.c | 4 +--
include/hw/boards.h | 2 ++
include/hw/i386/pc.h | 4 ---
7 files changed, 79 insertions(+), 63 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 766ca5899d..29a33194a9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -22,6 +22,7 @@
#include "qemu/error-report.h"
#include "sysemu/qtest.h"
#include "hw/pci/pci.h"
+#include "hw/mem/nvdimm.h"
GlobalProperty hw_compat_3_1[] = {
{ "pcie-root-port", "x-speed", "2_5" },
@@ -481,6 +482,47 @@ static void machine_set_memory_encryption(Object *obj, const char *value,
ms->memory_encryption = g_strdup(value);
}
+static bool machine_get_nvdimm(Object *obj, Error **errp)
+{
+ MachineState *ms = MACHINE(obj);
+
+ return ms->nvdimms_state->is_enabled;
+}
+
+static void machine_set_nvdimm(Object *obj, bool value, Error **errp)
+{
+ MachineState *ms = MACHINE(obj);
+
+ ms->nvdimms_state->is_enabled = value;
+}
+
+static char *machine_get_nvdimm_persistence(Object *obj, Error **errp)
+{
+ MachineState *ms = MACHINE(obj);
+
+ return g_strdup(ms->nvdimms_state->persistence_string);
+}
+
+static void machine_set_nvdimm_persistence(Object *obj, const char *value,
+ Error **errp)
+{
+ MachineState *ms = MACHINE(obj);
+ NVDIMMState *nvdimms_state = ms->nvdimms_state;
+
+ if (strcmp(value, "cpu") == 0) {
+ nvdimms_state->persistence = 3;
+ } else if (strcmp(value, "mem-ctrl") == 0) {
+ nvdimms_state->persistence = 2;
+ } else {
+ error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
+ value);
+ return;
+ }
+
+ g_free(nvdimms_state->persistence_string);
+ nvdimms_state->persistence_string = g_strdup(value);
+}
+
void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
{
strList *item = g_new0(strList, 1);
@@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
ms->mem_merge = true;
ms->enable_graphics = true;
+ if (mc->nvdimm_supported) {
+ ObjectClass *oc = OBJECT_CLASS(mc);
+
+ ms->nvdimms_state = g_new0(NVDIMMState, 1);
+ object_class_property_add_bool(oc, "nvdimm",
+ machine_get_nvdimm, machine_set_nvdimm,
+ &error_abort);
+ object_class_property_set_description(oc, "nvdimm",
+ "Set on/off to enable/disable "
+ "NVDIMM instantiation", NULL);
+
+ object_class_property_add_str(oc, "nvdimm-persistence",
+ machine_get_nvdimm_persistence,
+ machine_set_nvdimm_persistence,
+ &error_abort);
+ object_class_property_set_description(oc, "nvdimm-persistence",
+ "Set NVDIMM persistence"
+ "Valid values are cpu, mem-ctrl",
+ NULL);
+ }
+
+
/* Register notifier when init is done for sysbus sanity checks */
ms->sysbus_notifier.notify = machine_init_notify;
qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
@@ -809,6 +873,7 @@ static void machine_finalize(Object *obj)
g_free(ms->dt_compatible);
g_free(ms->firmware);
g_free(ms->device_memory);
+ g_free(ms->nvdimms_state);
}
bool machine_usb(MachineState *machine)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9ecc96dcc7..416da318ae 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1867,7 +1867,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
aml_append(scope, method);
}
- if (pcms->acpi_nvdimm_state.is_enabled) {
+ if (machine->nvdimms_state->is_enabled) {
method = aml_method("_E04", 0, AML_NOTSERIALIZED);
aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
aml_int(0x80)));
@@ -2704,9 +2704,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
build_dmar_q35(tables_blob, tables->linker);
}
}
- if (pcms->acpi_nvdimm_state.is_enabled) {
+ if (machine->nvdimms_state->is_enabled) {
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
- &pcms->acpi_nvdimm_state, machine->ram_slots);
+ machine->nvdimms_state, machine->ram_slots);
}
/* Add tables supplied by user (if any) */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0338dbe9da..71385cfac9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2069,6 +2069,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
{
const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+ const MachineState *ms = MACHINE(hotplug_dev);
const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
const uint64_t legacy_align = TARGET_PAGE_SIZE;
@@ -2083,7 +2084,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
return;
}
- if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
+ if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
return;
}
@@ -2097,6 +2098,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
{
Error *local_err = NULL;
PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+ MachineState *ms = MACHINE(hotplug_dev);
bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
@@ -2105,7 +2107,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
}
if (is_nvdimm) {
- nvdimm_plug(&pcms->acpi_nvdimm_state);
+ nvdimm_plug(ms->nvdimms_state);
}
hotplug_handler_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
@@ -2546,47 +2548,6 @@ static void pc_machine_set_smm(Object *obj, Visitor *v, const char *name,
visit_type_OnOffAuto(v, name, &pcms->smm, errp);
}
-static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
-{
- PCMachineState *pcms = PC_MACHINE(obj);
-
- return pcms->acpi_nvdimm_state.is_enabled;
-}
-
-static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
-{
- PCMachineState *pcms = PC_MACHINE(obj);
-
- pcms->acpi_nvdimm_state.is_enabled = value;
-}
-
-static char *pc_machine_get_nvdimm_persistence(Object *obj, Error **errp)
-{
- PCMachineState *pcms = PC_MACHINE(obj);
-
- return g_strdup(pcms->acpi_nvdimm_state.persistence_string);
-}
-
-static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
- Error **errp)
-{
- PCMachineState *pcms = PC_MACHINE(obj);
- NVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
-
- if (strcmp(value, "cpu") == 0)
- nvdimm_state->persistence = 3;
- else if (strcmp(value, "mem-ctrl") == 0)
- nvdimm_state->persistence = 2;
- else {
- error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
- value);
- return;
- }
-
- g_free(nvdimm_state->persistence_string);
- nvdimm_state->persistence_string = g_strdup(value);
-}
-
static bool pc_machine_get_smbus(Object *obj, Error **errp)
{
PCMachineState *pcms = PC_MACHINE(obj);
@@ -2636,8 +2597,6 @@ static void pc_machine_initfn(Object *obj)
pcms->max_ram_below_4g = 0; /* use default */
pcms->smm = ON_OFF_AUTO_AUTO;
pcms->vmport = ON_OFF_AUTO_AUTO;
- /* nvdimm is disabled on default. */
- pcms->acpi_nvdimm_state.is_enabled = false;
/* acpi build is enabled by default if machine supports it */
pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
pcms->smbus_enabled = true;
@@ -2774,6 +2733,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
hc->unplug = pc_machine_device_unplug_cb;
nc->nmi_monitor_handler = x86_nmi;
mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
+ mc->nvdimm_supported = true;
object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
pc_machine_get_device_memory_region_size, NULL,
@@ -2798,13 +2758,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
object_class_property_set_description(oc, PC_MACHINE_VMPORT,
"Enable vmport (pc & q35)", &error_abort);
- object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
- pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
-
- object_class_property_add_str(oc, PC_MACHINE_NVDIMM_PERSIST,
- pc_machine_get_nvdimm_persistence,
- pc_machine_set_nvdimm_persistence, &error_abort);
-
object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8770ecada9..8ad8e885c6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -297,8 +297,8 @@ static void pc_init1(MachineState *machine,
PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
}
- if (pcms->acpi_nvdimm_state.is_enabled) {
- nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
+ if (machine->nvdimms_state->is_enabled) {
+ nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
pcms->fw_cfg, OBJECT(pcms));
}
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index cfb9043e12..372c6b73be 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -329,8 +329,8 @@ static void pc_q35_init(MachineState *machine)
pc_vga_init(isa_bus, host_bus);
pc_nic_init(pcmc, isa_bus, host_bus);
- if (pcms->acpi_nvdimm_state.is_enabled) {
- nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
+ if (machine->nvdimms_state->is_enabled) {
+ nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
pcms->fw_cfg, OBJECT(pcms));
}
}
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9690c71a6d..e231860666 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -210,6 +210,7 @@ struct MachineClass {
int nb_nodes, ram_addr_t size);
bool ignore_boot_device_suffixes;
bool smbus_no_migration_support;
+ bool nvdimm_supported;
HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
DeviceState *dev);
@@ -272,6 +273,7 @@ struct MachineState {
const char *cpu_type;
AccelState *accelerator;
CPUArchIdList *possible_cpus;
+ struct NVDIMMState *nvdimms_state;
};
#define DEFINE_MACHINE(namestr, machine_initfn) \
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 94fb620d65..263a6343ff 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -45,8 +45,6 @@ struct PCMachineState {
OnOffAuto vmport;
OnOffAuto smm;
- NVDIMMState acpi_nvdimm_state;
-
bool acpi_build_enabled;
bool smbus_enabled;
bool sata_enabled;
@@ -74,8 +72,6 @@ struct PCMachineState {
#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
#define PC_MACHINE_VMPORT "vmport"
#define PC_MACHINE_SMM "smm"
-#define PC_MACHINE_NVDIMM "nvdimm"
-#define PC_MACHINE_NVDIMM_PERSIST "nvdimm-persistence"
#define PC_MACHINE_SMBUS "smbus"
#define PC_MACHINE_SATA "sata"
#define PC_MACHINE_PIT "pit"
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState Eric Auger
@ 2019-03-08 17:42 ` Eduardo Habkost
2019-03-08 17:45 ` Eduardo Habkost
2019-03-08 18:02 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 9+ messages in thread
From: Eduardo Habkost @ 2019-03-08 17:42 UTC (permalink / raw)
To: Eric Auger
Cc: eric.auger.pro, qemu-devel, qemu-arm, peter.maydell,
shameerali.kolothum.thodi, imammedo, david, pbonzini,
richard.henderson, sbhat, philmd
On Fri, Mar 08, 2019 at 04:25:11PM +0100, Eric Auger wrote:
[...]
> @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
> ms->mem_merge = true;
> ms->enable_graphics = true;
>
> + if (mc->nvdimm_supported) {
> + ObjectClass *oc = OBJECT_CLASS(mc);
> +
> + ms->nvdimms_state = g_new0(NVDIMMState, 1);
Initializing ms->nvdimms_state inside machine_initfn()
only if mc->nvdimm_support makes sense.
But:
> + object_class_property_add_bool(oc, "nvdimm",
> + machine_get_nvdimm, machine_set_nvdimm,
> + &error_abort);
> + object_class_property_set_description(oc, "nvdimm",
> + "Set on/off to enable/disable "
> + "NVDIMM instantiation", NULL);
> +
> + object_class_property_add_str(oc, "nvdimm-persistence",
> + machine_get_nvdimm_persistence,
> + machine_set_nvdimm_persistence,
> + &error_abort);
> + object_class_property_set_description(oc, "nvdimm-persistence",
> + "Set NVDIMM persistence"
> + "Valid values are cpu, mem-ctrl",
> + NULL);
Calling object_class_property_add_*() inside instance_init
defeats the purpose of class properties. I suggest either using
object_property_add_*(), or registering these class properties in
machine_class_base_init().
> [...]
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState
2019-03-08 17:42 ` Eduardo Habkost
@ 2019-03-08 17:45 ` Eduardo Habkost
0 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2019-03-08 17:45 UTC (permalink / raw)
To: Eric Auger
Cc: eric.auger.pro, qemu-devel, qemu-arm, peter.maydell,
shameerali.kolothum.thodi, imammedo, david, pbonzini,
richard.henderson, sbhat, philmd
On Fri, Mar 08, 2019 at 02:42:14PM -0300, Eduardo Habkost wrote:
> On Fri, Mar 08, 2019 at 04:25:11PM +0100, Eric Auger wrote:
> [...]
> > @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
> > ms->mem_merge = true;
> > ms->enable_graphics = true;
> >
> > + if (mc->nvdimm_supported) {
> > + ObjectClass *oc = OBJECT_CLASS(mc);
> > +
> > + ms->nvdimms_state = g_new0(NVDIMMState, 1);
>
> Initializing ms->nvdimms_state inside machine_initfn()
> only if mc->nvdimm_support makes sense.
>
> But:
>
> > + object_class_property_add_bool(oc, "nvdimm",
> > + machine_get_nvdimm, machine_set_nvdimm,
> > + &error_abort);
> > + object_class_property_set_description(oc, "nvdimm",
> > + "Set on/off to enable/disable "
> > + "NVDIMM instantiation", NULL);
> > +
> > + object_class_property_add_str(oc, "nvdimm-persistence",
> > + machine_get_nvdimm_persistence,
> > + machine_set_nvdimm_persistence,
> > + &error_abort);
> > + object_class_property_set_description(oc, "nvdimm-persistence",
> > + "Set NVDIMM persistence"
> > + "Valid values are cpu, mem-ctrl",
> > + NULL);
>
> Calling object_class_property_add_*() inside instance_init
> defeats the purpose of class properties. I suggest either using
> object_property_add_*(), or registering these class properties in
> machine_class_base_init().
I just realized that machine_class_base_init() won't work, as
it is called before the subclasses' class_init functions.
The only solutions I see here are using object_property_add*() or
creating a nvdimm_machine_class_init(MachineClass*) helper that
will set mc->nvdimm_supported=true and register the class properties.
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] nvdimm: Rename AcpiNVDIMMState into NVDIMMState
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 1/2] nvdimm: Rename AcpiNVDIMMState into NVDIMMState Eric Auger
@ 2019-03-08 17:59 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-08 17:59 UTC (permalink / raw)
To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell,
shameerali.kolothum.thodi, imammedo, david, pbonzini, ehabkost,
richard.henderson, sbhat
On 3/8/19 4:25 PM, Eric Auger wrote:
> As we intend to migrate the acpi_nvdimm_state into
> the base machine with a new dimms_state name, let's
> also rename the datatype.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/acpi/nvdimm.c | 18 +++++++++---------
> hw/i386/pc.c | 2 +-
> include/hw/i386/pc.h | 2 +-
> include/hw/mem/nvdimm.h | 10 +++++-----
> 4 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index e53b2cb681..f73cfb9d90 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -382,7 +382,7 @@ nvdimm_build_structure_caps(GArray *structures, uint32_t capabilities)
> nfit_caps->capabilities = cpu_to_le32(capabilities);
> }
>
> -static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
> +static GArray *nvdimm_build_device_structure(NVDIMMState *state)
> {
> GSList *device_list = nvdimm_get_device_list();
> GArray *structures = g_array_new(false, true /* clear */, 1);
> @@ -416,7 +416,7 @@ static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
> fit_buf->fit = g_array_new(false, true /* clear */, 1);
> }
>
> -static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
> +static void nvdimm_build_fit_buffer(NVDIMMState *state)
> {
> NvdimmFitBuffer *fit_buf = &state->fit_buf;
>
> @@ -425,12 +425,12 @@ static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
> fit_buf->dirty = true;
> }
>
> -void nvdimm_plug(AcpiNVDIMMState *state)
> +void nvdimm_plug(NVDIMMState *state)
> {
> nvdimm_build_fit_buffer(state);
> }
>
> -static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
> +static void nvdimm_build_nfit(NVDIMMState *state, GArray *table_offsets,
> GArray *table_data, BIOSLinker *linker)
> {
> NvdimmFitBuffer *fit_buf = &state->fit_buf;
> @@ -570,7 +570,7 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
> #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,
> +static void nvdimm_dsm_func_read_fit(NVDIMMState *state, NvdimmDsmIn *in,
> hwaddr dsm_mem_addr)
> {
> NvdimmFitBuffer *fit_buf = &state->fit_buf;
> @@ -619,7 +619,7 @@ exit:
> }
>
> static void
> -nvdimm_dsm_handle_reserved_root_method(AcpiNVDIMMState *state,
> +nvdimm_dsm_handle_reserved_root_method(NVDIMMState *state,
> NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> {
> switch (in->function) {
> @@ -863,7 +863,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;
> + NVDIMMState *state = opaque;
> NvdimmDsmIn *in;
> hwaddr dsm_mem_addr = val;
>
> @@ -925,7 +925,7 @@ void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
> }
> }
>
> -void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
> +void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
> FWCfgState *fw_cfg, Object *owner)
> {
> memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
> @@ -1319,7 +1319,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> }
>
> void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> - BIOSLinker *linker, AcpiNVDIMMState *state,
> + BIOSLinker *linker, NVDIMMState *state,
> uint32_t ram_slots)
> {
> GSList *device_list;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 42128183e9..0338dbe9da 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2571,7 +2571,7 @@ static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
> Error **errp)
> {
> PCMachineState *pcms = PC_MACHINE(obj);
> - AcpiNVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
> + NVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
>
> if (strcmp(value, "cpu") == 0)
> nvdimm_state->persistence = 3;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 54222a202d..94fb620d65 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -45,7 +45,7 @@ struct PCMachineState {
> OnOffAuto vmport;
> OnOffAuto smm;
>
> - AcpiNVDIMMState acpi_nvdimm_state;
> + NVDIMMState acpi_nvdimm_state;
>
> bool acpi_build_enabled;
> bool smbus_enabled;
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index c5c9b3c7f8..523a9b3d4a 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -123,7 +123,7 @@ struct NvdimmFitBuffer {
> };
> typedef struct NvdimmFitBuffer NvdimmFitBuffer;
>
> -struct AcpiNVDIMMState {
> +struct NVDIMMState {
> /* detect if NVDIMM support is enabled. */
> bool is_enabled;
>
> @@ -141,13 +141,13 @@ struct AcpiNVDIMMState {
> int32_t persistence;
> char *persistence_string;
> };
> -typedef struct AcpiNVDIMMState AcpiNVDIMMState;
> +typedef struct NVDIMMState NVDIMMState;
>
> -void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
> +void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
> FWCfgState *fw_cfg, Object *owner);
> void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> - BIOSLinker *linker, AcpiNVDIMMState *state,
> + BIOSLinker *linker, NVDIMMState *state,
> uint32_t ram_slots);
> -void nvdimm_plug(AcpiNVDIMMState *state);
> +void nvdimm_plug(NVDIMMState *state);
> void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
> #endif
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState Eric Auger
2019-03-08 17:42 ` Eduardo Habkost
@ 2019-03-08 18:02 ` Philippe Mathieu-Daudé
2019-03-08 18:16 ` Auger Eric
1 sibling, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-08 18:02 UTC (permalink / raw)
To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell,
shameerali.kolothum.thodi, imammedo, david, pbonzini, ehabkost,
richard.henderson, sbhat
hi Eric,
On 3/8/19 4:25 PM, Eric Auger wrote:
> As NVDIMM support is looming for ARM and SPAPR, let's
> move the acpi_nvdimm_state to the generic machine struct
> instead of duplicating the same code in several machines.
> It is also renamed into nvdimms_state and becomes a pointer.
>
> nvdimm and nvdimm-persistence become generic machine options.
> They become guarded by a nvdimm_supported machine class member.
> We also add a description for those options.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>
> ---
> v2 -> v3
> - nvdimms_state becomes a pointer
> - add machine class nvdimm_supported
>
> v1 -> v2:
> - s/acpi_nvdimm_state/nvdimms_state
> - remove ms->nvdimms_state.is_enabled initialization to false.
> ---
> hw/core/machine.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
> hw/i386/acpi-build.c | 6 ++--
> hw/i386/pc.c | 57 ++++----------------------------------
> hw/i386/pc_piix.c | 4 +--
> hw/i386/pc_q35.c | 4 +--
> include/hw/boards.h | 2 ++
> include/hw/i386/pc.h | 4 ---
> 7 files changed, 79 insertions(+), 63 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 766ca5899d..29a33194a9 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -22,6 +22,7 @@
> #include "qemu/error-report.h"
> #include "sysemu/qtest.h"
> #include "hw/pci/pci.h"
> +#include "hw/mem/nvdimm.h"
>
> GlobalProperty hw_compat_3_1[] = {
> { "pcie-root-port", "x-speed", "2_5" },
> @@ -481,6 +482,47 @@ static void machine_set_memory_encryption(Object *obj, const char *value,
> ms->memory_encryption = g_strdup(value);
> }
>
> +static bool machine_get_nvdimm(Object *obj, Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + return ms->nvdimms_state->is_enabled;
> +}
> +
> +static void machine_set_nvdimm(Object *obj, bool value, Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + ms->nvdimms_state->is_enabled = value;
> +}
> +
> +static char *machine_get_nvdimm_persistence(Object *obj, Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + return g_strdup(ms->nvdimms_state->persistence_string);
> +}
> +
> +static void machine_set_nvdimm_persistence(Object *obj, const char *value,
> + Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> + NVDIMMState *nvdimms_state = ms->nvdimms_state;
> +
> + if (strcmp(value, "cpu") == 0) {
> + nvdimms_state->persistence = 3;
> + } else if (strcmp(value, "mem-ctrl") == 0) {
> + nvdimms_state->persistence = 2;
> + } else {
> + error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
> + value);
> + return;
> + }
> +
> + g_free(nvdimms_state->persistence_string);
> + nvdimms_state->persistence_string = g_strdup(value);
> +}
> +
> void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
> {
> strList *item = g_new0(strList, 1);
> @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
> ms->mem_merge = true;
> ms->enable_graphics = true;
>
> + if (mc->nvdimm_supported) {
> + ObjectClass *oc = OBJECT_CLASS(mc);
> +
> + ms->nvdimms_state = g_new0(NVDIMMState, 1);
> + object_class_property_add_bool(oc, "nvdimm",
> + machine_get_nvdimm, machine_set_nvdimm,
> + &error_abort);
> + object_class_property_set_description(oc, "nvdimm",
> + "Set on/off to enable/disable "
> + "NVDIMM instantiation", NULL);
> +
> + object_class_property_add_str(oc, "nvdimm-persistence",
> + machine_get_nvdimm_persistence,
> + machine_set_nvdimm_persistence,
> + &error_abort);
> + object_class_property_set_description(oc, "nvdimm-persistence",
> + "Set NVDIMM persistence"
> + "Valid values are cpu, mem-ctrl",
> + NULL);
> + }
> +
> +
> /* Register notifier when init is done for sysbus sanity checks */
> ms->sysbus_notifier.notify = machine_init_notify;
> qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
> @@ -809,6 +873,7 @@ static void machine_finalize(Object *obj)
> g_free(ms->dt_compatible);
> g_free(ms->firmware);
> g_free(ms->device_memory);
> + g_free(ms->nvdimms_state);
> }
>
> bool machine_usb(MachineState *machine)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9ecc96dcc7..416da318ae 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1867,7 +1867,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> aml_append(scope, method);
> }
>
> - if (pcms->acpi_nvdimm_state.is_enabled) {
> + if (machine->nvdimms_state->is_enabled) {
> method = aml_method("_E04", 0, AML_NOTSERIALIZED);
> aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
> aml_int(0x80)));
> @@ -2704,9 +2704,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> build_dmar_q35(tables_blob, tables->linker);
> }
> }
> - if (pcms->acpi_nvdimm_state.is_enabled) {
> + if (machine->nvdimms_state->is_enabled) {
> nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> - &pcms->acpi_nvdimm_state, machine->ram_slots);
> + machine->nvdimms_state, machine->ram_slots);
> }
>
> /* Add tables supplied by user (if any) */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0338dbe9da..71385cfac9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2069,6 +2069,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> {
> const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> + const MachineState *ms = MACHINE(hotplug_dev);
> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> const uint64_t legacy_align = TARGET_PAGE_SIZE;
>
> @@ -2083,7 +2084,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> return;
> }
>
> - if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
> + if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> return;
> }
> @@ -2097,6 +2098,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
> {
> Error *local_err = NULL;
> PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> + MachineState *ms = MACHINE(hotplug_dev);
> bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>
> pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
> @@ -2105,7 +2107,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
> }
>
> if (is_nvdimm) {
> - nvdimm_plug(&pcms->acpi_nvdimm_state);
> + nvdimm_plug(ms->nvdimms_state);
> }
>
> hotplug_handler_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
> @@ -2546,47 +2548,6 @@ static void pc_machine_set_smm(Object *obj, Visitor *v, const char *name,
> visit_type_OnOffAuto(v, name, &pcms->smm, errp);
> }
>
> -static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
> -{
> - PCMachineState *pcms = PC_MACHINE(obj);
> -
> - return pcms->acpi_nvdimm_state.is_enabled;
> -}
> -
> -static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
> -{
> - PCMachineState *pcms = PC_MACHINE(obj);
> -
> - pcms->acpi_nvdimm_state.is_enabled = value;
> -}
> -
> -static char *pc_machine_get_nvdimm_persistence(Object *obj, Error **errp)
> -{
> - PCMachineState *pcms = PC_MACHINE(obj);
> -
> - return g_strdup(pcms->acpi_nvdimm_state.persistence_string);
> -}
> -
> -static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
> - Error **errp)
> -{
> - PCMachineState *pcms = PC_MACHINE(obj);
> - NVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
> -
> - if (strcmp(value, "cpu") == 0)
> - nvdimm_state->persistence = 3;
> - else if (strcmp(value, "mem-ctrl") == 0)
> - nvdimm_state->persistence = 2;
> - else {
> - error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
> - value);
> - return;
> - }
> -
> - g_free(nvdimm_state->persistence_string);
> - nvdimm_state->persistence_string = g_strdup(value);
> -}
> -
> static bool pc_machine_get_smbus(Object *obj, Error **errp)
> {
> PCMachineState *pcms = PC_MACHINE(obj);
> @@ -2636,8 +2597,6 @@ static void pc_machine_initfn(Object *obj)
> pcms->max_ram_below_4g = 0; /* use default */
> pcms->smm = ON_OFF_AUTO_AUTO;
> pcms->vmport = ON_OFF_AUTO_AUTO;
> - /* nvdimm is disabled on default. */
> - pcms->acpi_nvdimm_state.is_enabled = false;
> /* acpi build is enabled by default if machine supports it */
> pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
> pcms->smbus_enabled = true;
> @@ -2774,6 +2733,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> hc->unplug = pc_machine_device_unplug_cb;
> nc->nmi_monitor_handler = x86_nmi;
> mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> + mc->nvdimm_supported = true;
>
> object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
> pc_machine_get_device_memory_region_size, NULL,
> @@ -2798,13 +2758,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> object_class_property_set_description(oc, PC_MACHINE_VMPORT,
> "Enable vmport (pc & q35)", &error_abort);
>
> - object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
> - pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
> -
> - object_class_property_add_str(oc, PC_MACHINE_NVDIMM_PERSIST,
> - pc_machine_get_nvdimm_persistence,
> - pc_machine_set_nvdimm_persistence, &error_abort);
> -
> object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
> pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 8770ecada9..8ad8e885c6 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -297,8 +297,8 @@ static void pc_init1(MachineState *machine,
> PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
> }
>
> - if (pcms->acpi_nvdimm_state.is_enabled) {
> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> + if (machine->nvdimms_state->is_enabled) {
> + nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> pcms->fw_cfg, OBJECT(pcms));
> }
> }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index cfb9043e12..372c6b73be 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -329,8 +329,8 @@ static void pc_q35_init(MachineState *machine)
> pc_vga_init(isa_bus, host_bus);
> pc_nic_init(pcmc, isa_bus, host_bus);
>
> - if (pcms->acpi_nvdimm_state.is_enabled) {
> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> + if (machine->nvdimms_state->is_enabled) {
> + nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> pcms->fw_cfg, OBJECT(pcms));
> }
> }
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 9690c71a6d..e231860666 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -210,6 +210,7 @@ struct MachineClass {
> int nb_nodes, ram_addr_t size);
> bool ignore_boot_device_suffixes;
> bool smbus_no_migration_support;
> + bool nvdimm_supported;
>
> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> DeviceState *dev);
> @@ -272,6 +273,7 @@ struct MachineState {
> const char *cpu_type;
> AccelState *accelerator;
> CPUArchIdList *possible_cpus;
> + struct NVDIMMState *nvdimms_state;
What about simply 'nvdimm'?
- it is already part of a state
- no plural, there is only 1 state
> };
>
> #define DEFINE_MACHINE(namestr, machine_initfn) \
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 94fb620d65..263a6343ff 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -45,8 +45,6 @@ struct PCMachineState {
> OnOffAuto vmport;
> OnOffAuto smm;
>
> - NVDIMMState acpi_nvdimm_state;
> -
> bool acpi_build_enabled;
> bool smbus_enabled;
> bool sata_enabled;
> @@ -74,8 +72,6 @@ struct PCMachineState {
> #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
> #define PC_MACHINE_VMPORT "vmport"
> #define PC_MACHINE_SMM "smm"
> -#define PC_MACHINE_NVDIMM "nvdimm"
> -#define PC_MACHINE_NVDIMM_PERSIST "nvdimm-persistence"
> #define PC_MACHINE_SMBUS "smbus"
> #define PC_MACHINE_SATA "sata"
> #define PC_MACHINE_PIT "pit"
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState
2019-03-08 18:02 ` Philippe Mathieu-Daudé
@ 2019-03-08 18:16 ` Auger Eric
2019-03-08 21:04 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: Auger Eric @ 2019-03-08 18:16 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, eric.auger.pro, qemu-devel, qemu-arm,
peter.maydell, shameerali.kolothum.thodi, imammedo, david,
pbonzini, ehabkost, richard.henderson, sbhat
Hi Philippe,
On 3/8/19 7:02 PM, Philippe Mathieu-Daudé wrote:
> hi Eric,
>
> On 3/8/19 4:25 PM, Eric Auger wrote:
>> As NVDIMM support is looming for ARM and SPAPR, let's
>> move the acpi_nvdimm_state to the generic machine struct
>> instead of duplicating the same code in several machines.
>> It is also renamed into nvdimms_state and becomes a pointer.
>>
>> nvdimm and nvdimm-persistence become generic machine options.
>> They become guarded by a nvdimm_supported machine class member.
>> We also add a description for those options.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>
>> ---
>> v2 -> v3
>> - nvdimms_state becomes a pointer
>> - add machine class nvdimm_supported
>>
>> v1 -> v2:
>> - s/acpi_nvdimm_state/nvdimms_state
>> - remove ms->nvdimms_state.is_enabled initialization to false.
>> ---
>> hw/core/machine.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
>> hw/i386/acpi-build.c | 6 ++--
>> hw/i386/pc.c | 57 ++++----------------------------------
>> hw/i386/pc_piix.c | 4 +--
>> hw/i386/pc_q35.c | 4 +--
>> include/hw/boards.h | 2 ++
>> include/hw/i386/pc.h | 4 ---
>> 7 files changed, 79 insertions(+), 63 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 766ca5899d..29a33194a9 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -22,6 +22,7 @@
>> #include "qemu/error-report.h"
>> #include "sysemu/qtest.h"
>> #include "hw/pci/pci.h"
>> +#include "hw/mem/nvdimm.h"
>>
>> GlobalProperty hw_compat_3_1[] = {
>> { "pcie-root-port", "x-speed", "2_5" },
>> @@ -481,6 +482,47 @@ static void machine_set_memory_encryption(Object *obj, const char *value,
>> ms->memory_encryption = g_strdup(value);
>> }
>>
>> +static bool machine_get_nvdimm(Object *obj, Error **errp)
>> +{
>> + MachineState *ms = MACHINE(obj);
>> +
>> + return ms->nvdimms_state->is_enabled;
>> +}
>> +
>> +static void machine_set_nvdimm(Object *obj, bool value, Error **errp)
>> +{
>> + MachineState *ms = MACHINE(obj);
>> +
>> + ms->nvdimms_state->is_enabled = value;
>> +}
>> +
>> +static char *machine_get_nvdimm_persistence(Object *obj, Error **errp)
>> +{
>> + MachineState *ms = MACHINE(obj);
>> +
>> + return g_strdup(ms->nvdimms_state->persistence_string);
>> +}
>> +
>> +static void machine_set_nvdimm_persistence(Object *obj, const char *value,
>> + Error **errp)
>> +{
>> + MachineState *ms = MACHINE(obj);
>> + NVDIMMState *nvdimms_state = ms->nvdimms_state;
>> +
>> + if (strcmp(value, "cpu") == 0) {
>> + nvdimms_state->persistence = 3;
>> + } else if (strcmp(value, "mem-ctrl") == 0) {
>> + nvdimms_state->persistence = 2;
>> + } else {
>> + error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
>> + value);
>> + return;
>> + }
>> +
>> + g_free(nvdimms_state->persistence_string);
>> + nvdimms_state->persistence_string = g_strdup(value);
>> +}
>> +
>> void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>> {
>> strList *item = g_new0(strList, 1);
>> @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
>> ms->mem_merge = true;
>> ms->enable_graphics = true;
>>
>> + if (mc->nvdimm_supported) {
>> + ObjectClass *oc = OBJECT_CLASS(mc);
>> +
>> + ms->nvdimms_state = g_new0(NVDIMMState, 1);
>> + object_class_property_add_bool(oc, "nvdimm",
>> + machine_get_nvdimm, machine_set_nvdimm,
>> + &error_abort);
>> + object_class_property_set_description(oc, "nvdimm",
>> + "Set on/off to enable/disable "
>> + "NVDIMM instantiation", NULL);
>> +
>> + object_class_property_add_str(oc, "nvdimm-persistence",
>> + machine_get_nvdimm_persistence,
>> + machine_set_nvdimm_persistence,
>> + &error_abort);
>> + object_class_property_set_description(oc, "nvdimm-persistence",
>> + "Set NVDIMM persistence"
>> + "Valid values are cpu, mem-ctrl",
>> + NULL);
>> + }
>> +
>> +
>> /* Register notifier when init is done for sysbus sanity checks */
>> ms->sysbus_notifier.notify = machine_init_notify;
>> qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
>> @@ -809,6 +873,7 @@ static void machine_finalize(Object *obj)
>> g_free(ms->dt_compatible);
>> g_free(ms->firmware);
>> g_free(ms->device_memory);
>> + g_free(ms->nvdimms_state);
>> }
>>
>> bool machine_usb(MachineState *machine)
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 9ecc96dcc7..416da318ae 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1867,7 +1867,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> aml_append(scope, method);
>> }
>>
>> - if (pcms->acpi_nvdimm_state.is_enabled) {
>> + if (machine->nvdimms_state->is_enabled) {
>> method = aml_method("_E04", 0, AML_NOTSERIALIZED);
>> aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
>> aml_int(0x80)));
>> @@ -2704,9 +2704,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>> build_dmar_q35(tables_blob, tables->linker);
>> }
>> }
>> - if (pcms->acpi_nvdimm_state.is_enabled) {
>> + if (machine->nvdimms_state->is_enabled) {
>> nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
>> - &pcms->acpi_nvdimm_state, machine->ram_slots);
>> + machine->nvdimms_state, machine->ram_slots);
>> }
>>
>> /* Add tables supplied by user (if any) */
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 0338dbe9da..71385cfac9 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2069,6 +2069,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> {
>> const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> + const MachineState *ms = MACHINE(hotplug_dev);
>> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>> const uint64_t legacy_align = TARGET_PAGE_SIZE;
>>
>> @@ -2083,7 +2084,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> return;
>> }
>>
>> - if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
>> + if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> return;
>> }
>> @@ -2097,6 +2098,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>> {
>> Error *local_err = NULL;
>> PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> + MachineState *ms = MACHINE(hotplug_dev);
>> bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>
>> pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
>> @@ -2105,7 +2107,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>> }
>>
>> if (is_nvdimm) {
>> - nvdimm_plug(&pcms->acpi_nvdimm_state);
>> + nvdimm_plug(ms->nvdimms_state);
>> }
>>
>> hotplug_handler_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
>> @@ -2546,47 +2548,6 @@ static void pc_machine_set_smm(Object *obj, Visitor *v, const char *name,
>> visit_type_OnOffAuto(v, name, &pcms->smm, errp);
>> }
>>
>> -static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
>> -{
>> - PCMachineState *pcms = PC_MACHINE(obj);
>> -
>> - return pcms->acpi_nvdimm_state.is_enabled;
>> -}
>> -
>> -static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
>> -{
>> - PCMachineState *pcms = PC_MACHINE(obj);
>> -
>> - pcms->acpi_nvdimm_state.is_enabled = value;
>> -}
>> -
>> -static char *pc_machine_get_nvdimm_persistence(Object *obj, Error **errp)
>> -{
>> - PCMachineState *pcms = PC_MACHINE(obj);
>> -
>> - return g_strdup(pcms->acpi_nvdimm_state.persistence_string);
>> -}
>> -
>> -static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
>> - Error **errp)
>> -{
>> - PCMachineState *pcms = PC_MACHINE(obj);
>> - NVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
>> -
>> - if (strcmp(value, "cpu") == 0)
>> - nvdimm_state->persistence = 3;
>> - else if (strcmp(value, "mem-ctrl") == 0)
>> - nvdimm_state->persistence = 2;
>> - else {
>> - error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
>> - value);
>> - return;
>> - }
>> -
>> - g_free(nvdimm_state->persistence_string);
>> - nvdimm_state->persistence_string = g_strdup(value);
>> -}
>> -
>> static bool pc_machine_get_smbus(Object *obj, Error **errp)
>> {
>> PCMachineState *pcms = PC_MACHINE(obj);
>> @@ -2636,8 +2597,6 @@ static void pc_machine_initfn(Object *obj)
>> pcms->max_ram_below_4g = 0; /* use default */
>> pcms->smm = ON_OFF_AUTO_AUTO;
>> pcms->vmport = ON_OFF_AUTO_AUTO;
>> - /* nvdimm is disabled on default. */
>> - pcms->acpi_nvdimm_state.is_enabled = false;
>> /* acpi build is enabled by default if machine supports it */
>> pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
>> pcms->smbus_enabled = true;
>> @@ -2774,6 +2733,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>> hc->unplug = pc_machine_device_unplug_cb;
>> nc->nmi_monitor_handler = x86_nmi;
>> mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>> + mc->nvdimm_supported = true;
>>
>> object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
>> pc_machine_get_device_memory_region_size, NULL,
>> @@ -2798,13 +2758,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>> object_class_property_set_description(oc, PC_MACHINE_VMPORT,
>> "Enable vmport (pc & q35)", &error_abort);
>>
>> - object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
>> - pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
>> -
>> - object_class_property_add_str(oc, PC_MACHINE_NVDIMM_PERSIST,
>> - pc_machine_get_nvdimm_persistence,
>> - pc_machine_set_nvdimm_persistence, &error_abort);
>> -
>> object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
>> pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 8770ecada9..8ad8e885c6 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -297,8 +297,8 @@ static void pc_init1(MachineState *machine,
>> PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
>> }
>>
>> - if (pcms->acpi_nvdimm_state.is_enabled) {
>> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
>> + if (machine->nvdimms_state->is_enabled) {
>> + nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
>> pcms->fw_cfg, OBJECT(pcms));
>> }
>> }
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index cfb9043e12..372c6b73be 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -329,8 +329,8 @@ static void pc_q35_init(MachineState *machine)
>> pc_vga_init(isa_bus, host_bus);
>> pc_nic_init(pcmc, isa_bus, host_bus);
>>
>> - if (pcms->acpi_nvdimm_state.is_enabled) {
>> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
>> + if (machine->nvdimms_state->is_enabled) {
>> + nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
>> pcms->fw_cfg, OBJECT(pcms));
>> }
>> }
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 9690c71a6d..e231860666 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -210,6 +210,7 @@ struct MachineClass {
>> int nb_nodes, ram_addr_t size);
>> bool ignore_boot_device_suffixes;
>> bool smbus_no_migration_support;
>> + bool nvdimm_supported;
>>
>> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>> DeviceState *dev);
>> @@ -272,6 +273,7 @@ struct MachineState {
>> const char *cpu_type;
>> AccelState *accelerator;
>> CPUArchIdList *possible_cpus;
>> + struct NVDIMMState *nvdimms_state;
>
> What about simply 'nvdimm'?
> - it is already part of a state
> - no plural, there is only 1 state
Well actually I followed Igor's suggestion
(https://www.mail-archive.com/qemu-devel@nongnu.org/msg601980.html).
The state contains information about all present NVDIMMs (See
NvdimmFitBuffer comment and "fit: FIT structures for present NVDIMMs").
No strong opinion though.
Thanks
Eric
>
>> };
>>
>> #define DEFINE_MACHINE(namestr, machine_initfn) \
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 94fb620d65..263a6343ff 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -45,8 +45,6 @@ struct PCMachineState {
>> OnOffAuto vmport;
>> OnOffAuto smm;
>>
>> - NVDIMMState acpi_nvdimm_state;
>> -
>> bool acpi_build_enabled;
>> bool smbus_enabled;
>> bool sata_enabled;
>> @@ -74,8 +72,6 @@ struct PCMachineState {
>> #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
>> #define PC_MACHINE_VMPORT "vmport"
>> #define PC_MACHINE_SMM "smm"
>> -#define PC_MACHINE_NVDIMM "nvdimm"
>> -#define PC_MACHINE_NVDIMM_PERSIST "nvdimm-persistence"
>> #define PC_MACHINE_SMBUS "smbus"
>> #define PC_MACHINE_SATA "sata"
>> #define PC_MACHINE_PIT "pit"
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState
2019-03-08 18:16 ` Auger Eric
@ 2019-03-08 21:04 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-08 21:04 UTC (permalink / raw)
To: Auger Eric
Cc: eric.auger.pro, QEMU Developers, qemu-arm, Peter Maydell,
shameerali.kolothum.thodi, Igor Mammedov, David Hildenbrand,
Paolo Bonzini, Eduardo Habkost, Richard Henderson, sbhat
On Fri, Mar 8, 2019 at 7:16 PM Auger Eric <eric.auger@redhat.com> wrote:
> Hi Philippe,
> On 3/8/19 7:02 PM, Philippe Mathieu-Daudé wrote:
> > hi Eric,
> >
> > On 3/8/19 4:25 PM, Eric Auger wrote:
> >> As NVDIMM support is looming for ARM and SPAPR, let's
> >> move the acpi_nvdimm_state to the generic machine struct
> >> instead of duplicating the same code in several machines.
> >> It is also renamed into nvdimms_state and becomes a pointer.
> >>
> >> nvdimm and nvdimm-persistence become generic machine options.
> >> They become guarded by a nvdimm_supported machine class member.
> >> We also add a description for those options.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> >>
> >> ---
> >> v2 -> v3
> >> - nvdimms_state becomes a pointer
> >> - add machine class nvdimm_supported
> >>
> >> v1 -> v2:
> >> - s/acpi_nvdimm_state/nvdimms_state
> >> - remove ms->nvdimms_state.is_enabled initialization to false.
> >> ---
> >> hw/core/machine.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
> >> hw/i386/acpi-build.c | 6 ++--
> >> hw/i386/pc.c | 57 ++++----------------------------------
> >> hw/i386/pc_piix.c | 4 +--
> >> hw/i386/pc_q35.c | 4 +--
> >> include/hw/boards.h | 2 ++
> >> include/hw/i386/pc.h | 4 ---
> >> 7 files changed, 79 insertions(+), 63 deletions(-)
> >>
> >> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >> index 766ca5899d..29a33194a9 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -22,6 +22,7 @@
> >> #include "qemu/error-report.h"
> >> #include "sysemu/qtest.h"
> >> #include "hw/pci/pci.h"
> >> +#include "hw/mem/nvdimm.h"
> >>
> >> GlobalProperty hw_compat_3_1[] = {
> >> { "pcie-root-port", "x-speed", "2_5" },
> >> @@ -481,6 +482,47 @@ static void machine_set_memory_encryption(Object *obj, const char *value,
> >> ms->memory_encryption = g_strdup(value);
> >> }
> >>
> >> +static bool machine_get_nvdimm(Object *obj, Error **errp)
> >> +{
> >> + MachineState *ms = MACHINE(obj);
> >> +
> >> + return ms->nvdimms_state->is_enabled;
> >> +}
> >> +
> >> +static void machine_set_nvdimm(Object *obj, bool value, Error **errp)
> >> +{
> >> + MachineState *ms = MACHINE(obj);
> >> +
> >> + ms->nvdimms_state->is_enabled = value;
> >> +}
> >> +
> >> +static char *machine_get_nvdimm_persistence(Object *obj, Error **errp)
> >> +{
> >> + MachineState *ms = MACHINE(obj);
> >> +
> >> + return g_strdup(ms->nvdimms_state->persistence_string);
> >> +}
> >> +
> >> +static void machine_set_nvdimm_persistence(Object *obj, const char *value,
> >> + Error **errp)
> >> +{
> >> + MachineState *ms = MACHINE(obj);
> >> + NVDIMMState *nvdimms_state = ms->nvdimms_state;
> >> +
> >> + if (strcmp(value, "cpu") == 0) {
> >> + nvdimms_state->persistence = 3;
> >> + } else if (strcmp(value, "mem-ctrl") == 0) {
> >> + nvdimms_state->persistence = 2;
> >> + } else {
> >> + error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
> >> + value);
> >> + return;
> >> + }
> >> +
> >> + g_free(nvdimms_state->persistence_string);
> >> + nvdimms_state->persistence_string = g_strdup(value);
> >> +}
> >> +
> >> void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
> >> {
> >> strList *item = g_new0(strList, 1);
> >> @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
> >> ms->mem_merge = true;
> >> ms->enable_graphics = true;
> >>
> >> + if (mc->nvdimm_supported) {
> >> + ObjectClass *oc = OBJECT_CLASS(mc);
> >> +
> >> + ms->nvdimms_state = g_new0(NVDIMMState, 1);
> >> + object_class_property_add_bool(oc, "nvdimm",
> >> + machine_get_nvdimm, machine_set_nvdimm,
> >> + &error_abort);
> >> + object_class_property_set_description(oc, "nvdimm",
> >> + "Set on/off to enable/disable "
> >> + "NVDIMM instantiation", NULL);
> >> +
> >> + object_class_property_add_str(oc, "nvdimm-persistence",
> >> + machine_get_nvdimm_persistence,
> >> + machine_set_nvdimm_persistence,
> >> + &error_abort);
> >> + object_class_property_set_description(oc, "nvdimm-persistence",
> >> + "Set NVDIMM persistence"
> >> + "Valid values are cpu, mem-ctrl",
> >> + NULL);
> >> + }
> >> +
> >> +
> >> /* Register notifier when init is done for sysbus sanity checks */
> >> ms->sysbus_notifier.notify = machine_init_notify;
> >> qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
> >> @@ -809,6 +873,7 @@ static void machine_finalize(Object *obj)
> >> g_free(ms->dt_compatible);
> >> g_free(ms->firmware);
> >> g_free(ms->device_memory);
> >> + g_free(ms->nvdimms_state);
> >> }
> >>
> >> bool machine_usb(MachineState *machine)
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 9ecc96dcc7..416da318ae 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -1867,7 +1867,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >> aml_append(scope, method);
> >> }
> >>
> >> - if (pcms->acpi_nvdimm_state.is_enabled) {
> >> + if (machine->nvdimms_state->is_enabled) {
> >> method = aml_method("_E04", 0, AML_NOTSERIALIZED);
> >> aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
> >> aml_int(0x80)));
> >> @@ -2704,9 +2704,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> >> build_dmar_q35(tables_blob, tables->linker);
> >> }
> >> }
> >> - if (pcms->acpi_nvdimm_state.is_enabled) {
> >> + if (machine->nvdimms_state->is_enabled) {
> >> nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> >> - &pcms->acpi_nvdimm_state, machine->ram_slots);
> >> + machine->nvdimms_state, machine->ram_slots);
> >> }
> >>
> >> /* Add tables supplied by user (if any) */
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 0338dbe9da..71385cfac9 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -2069,6 +2069,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> {
> >> const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >> const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >> + const MachineState *ms = MACHINE(hotplug_dev);
> >> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >> const uint64_t legacy_align = TARGET_PAGE_SIZE;
> >>
> >> @@ -2083,7 +2084,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> return;
> >> }
> >>
> >> - if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
> >> + if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> >> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> >> return;
> >> }
> >> @@ -2097,6 +2098,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
> >> {
> >> Error *local_err = NULL;
> >> PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >> + MachineState *ms = MACHINE(hotplug_dev);
> >> bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >>
> >> pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
> >> @@ -2105,7 +2107,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
> >> }
> >>
> >> if (is_nvdimm) {
> >> - nvdimm_plug(&pcms->acpi_nvdimm_state);
> >> + nvdimm_plug(ms->nvdimms_state);
> >> }
> >>
> >> hotplug_handler_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
> >> @@ -2546,47 +2548,6 @@ static void pc_machine_set_smm(Object *obj, Visitor *v, const char *name,
> >> visit_type_OnOffAuto(v, name, &pcms->smm, errp);
> >> }
> >>
> >> -static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
> >> -{
> >> - PCMachineState *pcms = PC_MACHINE(obj);
> >> -
> >> - return pcms->acpi_nvdimm_state.is_enabled;
> >> -}
> >> -
> >> -static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
> >> -{
> >> - PCMachineState *pcms = PC_MACHINE(obj);
> >> -
> >> - pcms->acpi_nvdimm_state.is_enabled = value;
> >> -}
> >> -
> >> -static char *pc_machine_get_nvdimm_persistence(Object *obj, Error **errp)
> >> -{
> >> - PCMachineState *pcms = PC_MACHINE(obj);
> >> -
> >> - return g_strdup(pcms->acpi_nvdimm_state.persistence_string);
> >> -}
> >> -
> >> -static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
> >> - Error **errp)
> >> -{
> >> - PCMachineState *pcms = PC_MACHINE(obj);
> >> - NVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
> >> -
> >> - if (strcmp(value, "cpu") == 0)
> >> - nvdimm_state->persistence = 3;
> >> - else if (strcmp(value, "mem-ctrl") == 0)
> >> - nvdimm_state->persistence = 2;
> >> - else {
> >> - error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
> >> - value);
> >> - return;
> >> - }
> >> -
> >> - g_free(nvdimm_state->persistence_string);
> >> - nvdimm_state->persistence_string = g_strdup(value);
> >> -}
> >> -
> >> static bool pc_machine_get_smbus(Object *obj, Error **errp)
> >> {
> >> PCMachineState *pcms = PC_MACHINE(obj);
> >> @@ -2636,8 +2597,6 @@ static void pc_machine_initfn(Object *obj)
> >> pcms->max_ram_below_4g = 0; /* use default */
> >> pcms->smm = ON_OFF_AUTO_AUTO;
> >> pcms->vmport = ON_OFF_AUTO_AUTO;
> >> - /* nvdimm is disabled on default. */
> >> - pcms->acpi_nvdimm_state.is_enabled = false;
> >> /* acpi build is enabled by default if machine supports it */
> >> pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
> >> pcms->smbus_enabled = true;
> >> @@ -2774,6 +2733,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >> hc->unplug = pc_machine_device_unplug_cb;
> >> nc->nmi_monitor_handler = x86_nmi;
> >> mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> >> + mc->nvdimm_supported = true;
> >>
> >> object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
> >> pc_machine_get_device_memory_region_size, NULL,
> >> @@ -2798,13 +2758,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >> object_class_property_set_description(oc, PC_MACHINE_VMPORT,
> >> "Enable vmport (pc & q35)", &error_abort);
> >>
> >> - object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
> >> - pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
> >> -
> >> - object_class_property_add_str(oc, PC_MACHINE_NVDIMM_PERSIST,
> >> - pc_machine_get_nvdimm_persistence,
> >> - pc_machine_set_nvdimm_persistence, &error_abort);
> >> -
> >> object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
> >> pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
> >>
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 8770ecada9..8ad8e885c6 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -297,8 +297,8 @@ static void pc_init1(MachineState *machine,
> >> PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
> >> }
> >>
> >> - if (pcms->acpi_nvdimm_state.is_enabled) {
> >> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> >> + if (machine->nvdimms_state->is_enabled) {
> >> + nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> >> pcms->fw_cfg, OBJECT(pcms));
> >> }
> >> }
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index cfb9043e12..372c6b73be 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -329,8 +329,8 @@ static void pc_q35_init(MachineState *machine)
> >> pc_vga_init(isa_bus, host_bus);
> >> pc_nic_init(pcmc, isa_bus, host_bus);
> >>
> >> - if (pcms->acpi_nvdimm_state.is_enabled) {
> >> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> >> + if (machine->nvdimms_state->is_enabled) {
> >> + nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> >> pcms->fw_cfg, OBJECT(pcms));
> >> }
> >> }
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index 9690c71a6d..e231860666 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -210,6 +210,7 @@ struct MachineClass {
> >> int nb_nodes, ram_addr_t size);
> >> bool ignore_boot_device_suffixes;
> >> bool smbus_no_migration_support;
> >> + bool nvdimm_supported;
> >>
> >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >> DeviceState *dev);
> >> @@ -272,6 +273,7 @@ struct MachineState {
> >> const char *cpu_type;
> >> AccelState *accelerator;
> >> CPUArchIdList *possible_cpus;
> >> + struct NVDIMMState *nvdimms_state;
> >
> > What about simply 'nvdimm'?
> > - it is already part of a state
> > - no plural, there is only 1 state
> Well actually I followed Igor's suggestion
> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg601980.html).
>
> The state contains information about all present NVDIMMs (See
> NvdimmFitBuffer comment and "fit: FIT structures for present NVDIMMs").
So 'nvdimms' then :)
Anyway, Igor's call.
> No strong opinion though.
>
> Thanks
>
> Eric
>
>
>
> >
> >> };
> >>
> >> #define DEFINE_MACHINE(namestr, machine_initfn) \
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 94fb620d65..263a6343ff 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -45,8 +45,6 @@ struct PCMachineState {
> >> OnOffAuto vmport;
> >> OnOffAuto smm;
> >>
> >> - NVDIMMState acpi_nvdimm_state;
> >> -
> >> bool acpi_build_enabled;
> >> bool smbus_enabled;
> >> bool sata_enabled;
> >> @@ -74,8 +72,6 @@ struct PCMachineState {
> >> #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
> >> #define PC_MACHINE_VMPORT "vmport"
> >> #define PC_MACHINE_SMM "smm"
> >> -#define PC_MACHINE_NVDIMM "nvdimm"
> >> -#define PC_MACHINE_NVDIMM_PERSIST "nvdimm-persistence"
> >> #define PC_MACHINE_SMBUS "smbus"
> >> #define PC_MACHINE_SATA "sata"
> >> #define PC_MACHINE_PIT "pit"
> >>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-03-08 21:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-08 15:25 [Qemu-devel] [PATCH v3 0/2] machine: Move nvdimms state into struct MachineState Eric Auger
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 1/2] nvdimm: Rename AcpiNVDIMMState into NVDIMMState Eric Auger
2019-03-08 17:59 ` Philippe Mathieu-Daudé
2019-03-08 15:25 ` [Qemu-devel] [PATCH v3 2/2] machine: Move nvdimms state into struct MachineState Eric Auger
2019-03-08 17:42 ` Eduardo Habkost
2019-03-08 17:45 ` Eduardo Habkost
2019-03-08 18:02 ` Philippe Mathieu-Daudé
2019-03-08 18:16 ` Auger Eric
2019-03-08 21:04 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).