Hi Eduardo, On 3/7/19 6:26 PM, Eduardo Habkost wrote: > On Thu, Mar 07, 2019 at 10:06:39AM +0100, 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. >> >> nvdimm and nvdimm-persistence become generic machine options. >> We also add a description for those options. >> >> We also remove the nvdimms_state.is_enabled initialization to >> false as objects are guaranteed to be zero initialized. >> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> Suggested-by: Igor Mammedov <imamm...@redhat.com> >> >> --- >> >> v1 -> v2: >> - s/acpi_nvdimm_state/nvdimms_state >> - remove ms->nvdimms_state.is_enabled initialization to false. > [...] >> @@ -765,6 +806,20 @@ static void machine_class_init(ObjectClass *oc, void >> *data) >> &error_abort); >> object_class_property_set_description(oc, "memory-encryption", >> "Set memory encryption object to use", &error_abort); >> + >> + 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 and >> mem-ctrl", >> + NULL); > > As noted in another reply, I don't mind adding new MachineState > fields, but now I noticed you are adding new user-visible > options, which requires more care. > > This patch seems to make all machines except PC silently ignore > the new nvdimm options. If the current machine doesn't support > nvdimm, "nvdimm-persistence=..." and "nvdimm=on" should be > rejected by QEMU instead of silently ignored. > > Probably the simplest way to do that is by making the > registration of those QOM properties conditional. > > We could add a simple > bool MachineClass::nvdimm_supported Makes sense to me too. I will go that way.
Thanks Eric > field, or we could add a > static void nvdimm_machine_class_init(MachineClass *mc); > helper that would enable nvdimm support on the machine type. >
