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

[Philippe Mathieu-Daudé]

On 3/7/19 5:58 PM, Eduardo Habkost wrote:
> On Thu, Mar 07, 2019 at 04:15:21PM +0100, Auger Eric wrote:
>> Hi Philippe, Eduardo,
>>
>> On 3/7/19 11:56 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Eric, Eduardo,
>>>
>>> On 3/7/19 10:06 AM, 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 <address@hidden>
>>>> Suggested-by: Igor Mammedov <address@hidden>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - s/acpi_nvdimm_state/nvdimms_state
>>>> - remove ms->nvdimms_state.is_enabled initialization to false.
>>>> ---
>>>>  hw/core/machine.c        | 55 +++++++++++++++++++++++++++++++++++++++
>>>>  hw/i386/acpi-build.c     |  6 ++---
>>>>  hw/i386/pc.c             | 56 +++-------------------------------------
>>>>  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/pc-dimm.h |  1 -
>>>>  8 files changed, 68 insertions(+), 64 deletions(-)
>>>>
>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>> index 766ca5899d..21a7209246 100644
>>>> --- a/hw/core/machine.c
>>>> +++ b/hw/core/machine.c
>>>> @@ -481,6 +481,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);
>>>> +    AcpiNVDIMMState *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);
>>>> @@ -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);
>>>>  }
>>>>  
>>>>  static void machine_class_base_init(ObjectClass *oc, void *data)
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index 9ecc96dcc7..2d7d46fe50 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 42128183e9..cacc4068cf 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);
>>>> -    AcpiNVDIMMState *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;
>>>> @@ -2798,13 +2757,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..16ebfc5a5a 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..dacaa90611 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..ccf0c5a69d 100644
>>>> --- a/include/hw/boards.h
>>>> +++ b/include/hw/boards.h
>>>> @@ -8,6 +8,7 @@
>>>>  #include "hw/qdev.h"
>>>>  #include "qom/object.h"
>>>>  #include "qom/cpu.h"
>>>> +#include "hw/mem/nvdimm.h"
>>>>  
>>>>  /**
>>>>   * memory_region_allocate_system_memory - Allocate a board's main memory
>>>> @@ -272,6 +273,7 @@ struct MachineState {
>>>>      const char *cpu_type;
>>>>      AccelState *accelerator;
>>>>      CPUArchIdList *possible_cpus;
>>>> +    AcpiNVDIMMState nvdimms_state;
>>>
>>> It looks we are breaking the generic/abstract machine design.
>>> You introduce 2 specific concepts here, ACPI and NVDIMM.
>> at least this should be renamed NVDIMMState according to last Igor's
>> comment.
>>>
>>> MachineClass tries to be generic, and and still suffer from specific
>>> fields inherited from the PC Machine.
>>>
>>> I'd use an intermediate Memory-related object between Machine and
>>> AcpiNVDIMM states.
>>>
>>> I see there already is DeviceMemoryState.
>>>
>>> Can AcpiNVDIMMState go there? That would be more acceptable IMHO.
>> DeviceMemoryState is also declared in include/hw/boards.h so does it
>> hide any details.
>>
>> Not every machine has device memory or irqchip_split either for instance
>> (ARM virt is a good example). So my understanding is we put in
>> MachineState fields that are generic enough to be used by several
>> machines and avoid duplication.
>>
>> Eduardo, do you have any objection wrt putting this state in the base
>> machine?
> 
> I am not against having code and fields in MachineState that
> apply to just a few machines.  I would even prefer that instead
> of adding even more complexity to the QOM tree and/or QOM type
> hierarchy.

Understood.

> But I would make the it a pointer to an incomplete type, to avoid
> the boards.h -> nvdimm.h header dependency.

Good idea.