Re: [Qemu-devel] [PATCH v2] machine: Move acpi_nvdimm_state into struct MachineState

[Eduardo Habkost <ehabkost@redhat.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 <eric.auger@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@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
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