[v2] machine: Move acpi_nvdimm_state into struct MachineState

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

On Thu,  7 Mar 2019 10:06:39 +0100
Eric Auger <eric.auger@redhat.com> 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.
> ---
>  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)

broken indent

> +{
> +    MachineState *ms = MACHINE(obj);
> +    AcpiNVDIMMState *nvdimms_state = &ms->nvdimms_state;
> +
> +    if (strcmp(value, "cpu") == 0)
> +        nvdimms_state->persistence = 3;
we probably should use QAPI enum magic to handle description to value conversion
but I don't know how to (CCed Markus).
But since it's just moving existing code, it do not insist and it could be
done on top later on.


> +    else if (strcmp(value, "mem-ctrl") == 0)
> +        nvdimms_state->persistence = 2;

As opposed to kernel if/else in QEMU always should use {} even if they affect only one line

> +    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's still dubbed after Acpi in type name ...

>  };
>  
>  #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) \

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

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.

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) \
>

On Thu, Mar 07, 2019 at 10:48:59AM +0100, Igor Mammedov wrote:
[...]
> > +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;
> we probably should use QAPI enum magic to handle description to value conversion
> but I don't know how to (CCed Markus).

QEMU is not very consistent in this.  I have found at least 3
different methods for registering enum properties:

1) qdev PropertyInfo using set_enum/get_enum.  Examples:
   DEFINE_PROP_ON_OFF_AUTO, DEFINE_PROP_LOSTTICKPOLICY.
2) object_property_add() + visit_type_...().  Examples:
   machine_set_kernel_irqchip(), pc_machine_set_vmport(),
   pc_machine_set_smm().
3) object_property_add_add_enum() and object_class_property_add_enum().  Examples:
   qauthz_list_prop_set_policy(), host_memory_backend_set_policy(),
   qcrypto_secret_prop_set_format(), netfilter_set_direction().

> But since it's just moving existing code, it do not insist and it could be
> done on top later on.

Agreed, but I think we should at least add a TODO comment
indicating the code is not a good example to be followed.

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

On 07.03.19 16:15, 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 <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.
>>> ---
>>>  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

device_memory is an abstracted concept that can be theoretically
implemented by any machine. nvdimm is not.

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

On 3/7/19 4:21 PM, David Hildenbrand wrote:
> On 07.03.19 16:15, 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 <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.
>>>> ---
>>>>  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
> 
> device_memory is an abstracted concept that can be theoretically
> implemented by any machine. nvdimm is not.

Then maybe the DeviceMemoryState is misleading, NVDIMM might not be
related to QEMU's DeviceMemory concept, but it is a 'device memory' :)

The between-object I'm referring to would be a container of device
memory types. It doesn't have to be a single instance but rather a list
of 'hw memory' interfaces.

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

On 07.03.19 16:36, Philippe Mathieu-Daudé wrote:
> On 3/7/19 4:21 PM, David Hildenbrand wrote:
>> On 07.03.19 16:15, 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 <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.
>>>>> ---
>>>>>  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
>>
>> device_memory is an abstracted concept that can be theoretically
>> implemented by any machine. nvdimm is not.
> 
> Then maybe the DeviceMemoryState is misleading, NVDIMM might not be
> related to QEMU's DeviceMemory concept, but it is a 'device memory' :)

We should have/should name it MemoryDeviceState/memory_device_state.
That is source of confusion (memory device vs. device memory) :) Yes, I
am to blame.

> 
> The between-object I'm referring to would be a container of device
> memory types. It doesn't have to be a single instance but rather a list
> of 'hw memory' interfaces.
>
>>

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

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

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

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.

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