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

[Auger Eric]

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?

Thanks

Eric
> 
> Eduardo, what do you think (about this patch refactor in general)?
> 
> Thanks,
> 
> Phil.
> 
>>  };
>>  
>>  #define DEFINE_MACHINE(namestr, machine_initfn) \
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 54222a202d..263a6343ff 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -45,8 +45,6 @@ struct PCMachineState {
>>      OnOffAuto vmport;
>>      OnOffAuto smm;
>>  
>> -    AcpiNVDIMMState acpi_nvdimm_state;
>> -
>>      bool acpi_build_enabled;
>>      bool smbus_enabled;
>>      bool sata_enabled;
>> @@ -74,8 +72,6 @@ struct PCMachineState {
>>  #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
>>  #define PC_MACHINE_VMPORT           "vmport"
>>  #define PC_MACHINE_SMM              "smm"
>> -#define PC_MACHINE_NVDIMM           "nvdimm"
>> -#define PC_MACHINE_NVDIMM_PERSIST   "nvdimm-persistence"
>>  #define PC_MACHINE_SMBUS            "smbus"
>>  #define PC_MACHINE_SATA             "sata"
>>  #define PC_MACHINE_PIT              "pit"
>> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
>> index 01436b9f50..3e5489d3a1 100644
>> --- a/include/hw/mem/pc-dimm.h
>> +++ b/include/hw/mem/pc-dimm.h
>> @@ -19,7 +19,6 @@
>>  #include "exec/memory.h"
>>  #include "sysemu/hostmem.h"
>>  #include "hw/qdev.h"
>> -#include "hw/boards.h"
>>  
>>  #define TYPE_PC_DIMM "pc-dimm"
>>  #define PC_DIMM(obj) \
>>
>