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

[Auger Eric <eric.auger@redhat.com>]

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.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
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.
>