From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:38156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h1wmu-0007gB-I1 for qemu-devel@nongnu.org; Thu, 07 Mar 2019 12:26:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1h1wmt-000568-Lv for qemu-devel@nongnu.org; Thu, 07 Mar 2019 12:26:32 -0500 Date: Thu, 7 Mar 2019 14:26:12 -0300 From: Eduardo Habkost Message-ID: <20190307172612.GC8899@habkost.net> References: <20190307090639.8261-1-eric.auger@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190307090639.8261-1-eric.auger@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2] machine: Move acpi_nvdimm_state into struct MachineState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Auger Cc: eric.auger.pro@gmail.com, qemu-devel@nongnu.org, qemu-arm@nongnu.org, peter.maydell@linaro.org, shameerali.kolothum.thodi@huawei.com, imammedo@redhat.com, david@redhat.com, pbonzini@redhat.com, richard.henderson@linaro.org, sbhat@linux.ibm.com 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 > Suggested-by: Igor Mammedov > > --- > > 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 field, or we could add a static void nvdimm_machine_class_init(MachineClass *mc); helper that would enable nvdimm support on the machine type. -- Eduardo