diff mbox series

[v3,2/2] machine: Move nvdimms state into struct MachineState

Message ID 20190308152511.10882-3-eric.auger@redhat.com
State New
Headers show
Series machine: Move nvdimms state into struct MachineState | expand

Commit Message

Eric Auger March 8, 2019, 3:25 p.m. UTC 
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 and becomes a pointer.

nvdimm and nvdimm-persistence become generic machine options.
They become guarded by a nvdimm_supported machine class member.
We also add a description for those options.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>

---
v2 -> v3
- nvdimms_state becomes a pointer
- add machine class nvdimm_supported

v1 -> v2:
- s/acpi_nvdimm_state/nvdimms_state
- remove ms->nvdimms_state.is_enabled initialization to false.
---
 hw/core/machine.c    | 65 ++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c |  6 ++--
 hw/i386/pc.c         | 57 ++++----------------------------------
 hw/i386/pc_piix.c    |  4 +--
 hw/i386/pc_q35.c     |  4 +--
 include/hw/boards.h  |  2 ++
 include/hw/i386/pc.h |  4 ---
 7 files changed, 79 insertions(+), 63 deletions(-)

Comments

Eduardo Habkost March 8, 2019, 5:42 p.m. UTC | #1 
On Fri, Mar 08, 2019 at 04:25:11PM +0100, Eric Auger wrote:
[...]
> @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
>      ms->mem_merge = true;
>      ms->enable_graphics = true;
>  
> +    if (mc->nvdimm_supported) {
> +        ObjectClass *oc = OBJECT_CLASS(mc);
> +
> +        ms->nvdimms_state = g_new0(NVDIMMState, 1);

Initializing ms->nvdimms_state inside machine_initfn()
only if mc->nvdimm_support makes sense.

But:

> +        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, mem-ctrl",
> +                                              NULL);

Calling object_class_property_add_*() inside instance_init
defeats the purpose of class properties.  I suggest either using
object_property_add_*(), or registering these class properties in
machine_class_base_init().


> [...]
Eduardo Habkost March 8, 2019, 5:45 p.m. UTC | #2 
On Fri, Mar 08, 2019 at 02:42:14PM -0300, Eduardo Habkost wrote:
> On Fri, Mar 08, 2019 at 04:25:11PM +0100, Eric Auger wrote:
> [...]
> > @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
> >      ms->mem_merge = true;
> >      ms->enable_graphics = true;
> >  
> > +    if (mc->nvdimm_supported) {
> > +        ObjectClass *oc = OBJECT_CLASS(mc);
> > +
> > +        ms->nvdimms_state = g_new0(NVDIMMState, 1);
> 
> Initializing ms->nvdimms_state inside machine_initfn()
> only if mc->nvdimm_support makes sense.
> 
> But:
> 
> > +        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, mem-ctrl",
> > +                                              NULL);
> 
> Calling object_class_property_add_*() inside instance_init
> defeats the purpose of class properties.  I suggest either using
> object_property_add_*(), or registering these class properties in
> machine_class_base_init().

I just realized that machine_class_base_init() won't work, as
it is called before the subclasses' class_init functions.

The only solutions I see here are using object_property_add*() or
creating a nvdimm_machine_class_init(MachineClass*) helper that
will set mc->nvdimm_supported=true and register the class properties.
Philippe Mathieu-Daudé March 8, 2019, 6:02 p.m. UTC | #3 
hi Eric,

On 3/8/19 4:25 PM, 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 and becomes a pointer.
> 
> nvdimm and nvdimm-persistence become generic machine options.
> They become guarded by a nvdimm_supported machine class member.
> We also add a description for those options.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> 
> ---
> v2 -> v3
> - nvdimms_state becomes a pointer
> - add machine class nvdimm_supported
> 
> v1 -> v2:
> - s/acpi_nvdimm_state/nvdimms_state
> - remove ms->nvdimms_state.is_enabled initialization to false.
> ---
>  hw/core/machine.c    | 65 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c |  6 ++--
>  hw/i386/pc.c         | 57 ++++----------------------------------
>  hw/i386/pc_piix.c    |  4 +--
>  hw/i386/pc_q35.c     |  4 +--
>  include/hw/boards.h  |  2 ++
>  include/hw/i386/pc.h |  4 ---
>  7 files changed, 79 insertions(+), 63 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 766ca5899d..29a33194a9 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -22,6 +22,7 @@
>  #include "qemu/error-report.h"
>  #include "sysemu/qtest.h"
>  #include "hw/pci/pci.h"
> +#include "hw/mem/nvdimm.h"
>  
>  GlobalProperty hw_compat_3_1[] = {
>      { "pcie-root-port", "x-speed", "2_5" },
> @@ -481,6 +482,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);
> +    NVDIMMState *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);
> @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
>      ms->mem_merge = true;
>      ms->enable_graphics = true;
>  
> +    if (mc->nvdimm_supported) {
> +        ObjectClass *oc = OBJECT_CLASS(mc);
> +
> +        ms->nvdimms_state = g_new0(NVDIMMState, 1);
> +        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, mem-ctrl",
> +                                              NULL);
> +    }
> +
> +
>      /* Register notifier when init is done for sysbus sanity checks */
>      ms->sysbus_notifier.notify = machine_init_notify;
>      qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
> @@ -809,6 +873,7 @@ static void machine_finalize(Object *obj)
>      g_free(ms->dt_compatible);
>      g_free(ms->firmware);
>      g_free(ms->device_memory);
> +    g_free(ms->nvdimms_state);
>  }
>  
>  bool machine_usb(MachineState *machine)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9ecc96dcc7..416da318ae 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 0338dbe9da..71385cfac9 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);
> -    NVDIMMState *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;
> @@ -2774,6 +2733,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      hc->unplug = pc_machine_device_unplug_cb;
>      nc->nmi_monitor_handler = x86_nmi;
>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> +    mc->nvdimm_supported = true;
>  
>      object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
>          pc_machine_get_device_memory_region_size, NULL,
> @@ -2798,13 +2758,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..8ad8e885c6 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..372c6b73be 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..e231860666 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -210,6 +210,7 @@ struct MachineClass {
>                                   int nb_nodes, ram_addr_t size);
>      bool ignore_boot_device_suffixes;
>      bool smbus_no_migration_support;
> +    bool nvdimm_supported;
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> @@ -272,6 +273,7 @@ struct MachineState {
>      const char *cpu_type;
>      AccelState *accelerator;
>      CPUArchIdList *possible_cpus;
> +    struct NVDIMMState *nvdimms_state;

What about simply 'nvdimm'?
- it is already part of a state
- no plural, there is only 1 state

>  };
>  
>  #define DEFINE_MACHINE(namestr, machine_initfn) \
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 94fb620d65..263a6343ff 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -45,8 +45,6 @@ struct PCMachineState {
>      OnOffAuto vmport;
>      OnOffAuto smm;
>  
> -    NVDIMMState 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"
>
Eric Auger March 8, 2019, 6:16 p.m. UTC | #4 
Hi Philippe,
On 3/8/19 7:02 PM, Philippe Mathieu-Daudé wrote:
> hi Eric,
> 
> On 3/8/19 4:25 PM, 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 and becomes a pointer.
>>
>> nvdimm and nvdimm-persistence become generic machine options.
>> They become guarded by a nvdimm_supported machine class member.
>> We also add a description for those options.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>
>> ---
>> v2 -> v3
>> - nvdimms_state becomes a pointer
>> - add machine class nvdimm_supported
>>
>> v1 -> v2:
>> - s/acpi_nvdimm_state/nvdimms_state
>> - remove ms->nvdimms_state.is_enabled initialization to false.
>> ---
>>  hw/core/machine.c    | 65 ++++++++++++++++++++++++++++++++++++++++++++
>>  hw/i386/acpi-build.c |  6 ++--
>>  hw/i386/pc.c         | 57 ++++----------------------------------
>>  hw/i386/pc_piix.c    |  4 +--
>>  hw/i386/pc_q35.c     |  4 +--
>>  include/hw/boards.h  |  2 ++
>>  include/hw/i386/pc.h |  4 ---
>>  7 files changed, 79 insertions(+), 63 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 766ca5899d..29a33194a9 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -22,6 +22,7 @@
>>  #include "qemu/error-report.h"
>>  #include "sysemu/qtest.h"
>>  #include "hw/pci/pci.h"
>> +#include "hw/mem/nvdimm.h"
>>  
>>  GlobalProperty hw_compat_3_1[] = {
>>      { "pcie-root-port", "x-speed", "2_5" },
>> @@ -481,6 +482,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);
>> +    NVDIMMState *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);
>> @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
>>      ms->mem_merge = true;
>>      ms->enable_graphics = true;
>>  
>> +    if (mc->nvdimm_supported) {
>> +        ObjectClass *oc = OBJECT_CLASS(mc);
>> +
>> +        ms->nvdimms_state = g_new0(NVDIMMState, 1);
>> +        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, mem-ctrl",
>> +                                              NULL);
>> +    }
>> +
>> +
>>      /* Register notifier when init is done for sysbus sanity checks */
>>      ms->sysbus_notifier.notify = machine_init_notify;
>>      qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
>> @@ -809,6 +873,7 @@ static void machine_finalize(Object *obj)
>>      g_free(ms->dt_compatible);
>>      g_free(ms->firmware);
>>      g_free(ms->device_memory);
>> +    g_free(ms->nvdimms_state);
>>  }
>>  
>>  bool machine_usb(MachineState *machine)
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 9ecc96dcc7..416da318ae 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 0338dbe9da..71385cfac9 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);
>> -    NVDIMMState *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;
>> @@ -2774,6 +2733,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>      hc->unplug = pc_machine_device_unplug_cb;
>>      nc->nmi_monitor_handler = x86_nmi;
>>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>> +    mc->nvdimm_supported = true;
>>  
>>      object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
>>          pc_machine_get_device_memory_region_size, NULL,
>> @@ -2798,13 +2758,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..8ad8e885c6 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..372c6b73be 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..e231860666 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -210,6 +210,7 @@ struct MachineClass {
>>                                   int nb_nodes, ram_addr_t size);
>>      bool ignore_boot_device_suffixes;
>>      bool smbus_no_migration_support;
>> +    bool nvdimm_supported;
>>  
>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>                                             DeviceState *dev);
>> @@ -272,6 +273,7 @@ struct MachineState {
>>      const char *cpu_type;
>>      AccelState *accelerator;
>>      CPUArchIdList *possible_cpus;
>> +    struct NVDIMMState *nvdimms_state;
> 
> What about simply 'nvdimm'?
> - it is already part of a state
> - no plural, there is only 1 state
Well actually I followed Igor's suggestion
(https://www.mail-archive.com/qemu-devel@nongnu.org/msg601980.html).

The state contains information about all present NVDIMMs (See
NvdimmFitBuffer comment and "fit: FIT structures for present NVDIMMs").

No strong opinion though.

Thanks

Eric



> 
>>  };
>>  
>>  #define DEFINE_MACHINE(namestr, machine_initfn) \
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 94fb620d65..263a6343ff 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -45,8 +45,6 @@ struct PCMachineState {
>>      OnOffAuto vmport;
>>      OnOffAuto smm;
>>  
>> -    NVDIMMState 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"
>>
Philippe Mathieu-Daudé March 8, 2019, 9:04 p.m. UTC | #5 
On Fri, Mar 8, 2019 at 7:16 PM Auger Eric <eric.auger@redhat.com> wrote:
> Hi Philippe,
> On 3/8/19 7:02 PM, Philippe Mathieu-Daudé wrote:
> > hi Eric,
> >
> > On 3/8/19 4:25 PM, 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 and becomes a pointer.
> >>
> >> nvdimm and nvdimm-persistence become generic machine options.
> >> They become guarded by a nvdimm_supported machine class member.
> >> We also add a description for those options.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> >>
> >> ---
> >> v2 -> v3
> >> - nvdimms_state becomes a pointer
> >> - add machine class nvdimm_supported
> >>
> >> v1 -> v2:
> >> - s/acpi_nvdimm_state/nvdimms_state
> >> - remove ms->nvdimms_state.is_enabled initialization to false.
> >> ---
> >>  hw/core/machine.c    | 65 ++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/i386/acpi-build.c |  6 ++--
> >>  hw/i386/pc.c         | 57 ++++----------------------------------
> >>  hw/i386/pc_piix.c    |  4 +--
> >>  hw/i386/pc_q35.c     |  4 +--
> >>  include/hw/boards.h  |  2 ++
> >>  include/hw/i386/pc.h |  4 ---
> >>  7 files changed, 79 insertions(+), 63 deletions(-)
> >>
> >> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >> index 766ca5899d..29a33194a9 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -22,6 +22,7 @@
> >>  #include "qemu/error-report.h"
> >>  #include "sysemu/qtest.h"
> >>  #include "hw/pci/pci.h"
> >> +#include "hw/mem/nvdimm.h"
> >>
> >>  GlobalProperty hw_compat_3_1[] = {
> >>      { "pcie-root-port", "x-speed", "2_5" },
> >> @@ -481,6 +482,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);
> >> +    NVDIMMState *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);
> >> @@ -791,6 +833,28 @@ static void machine_initfn(Object *obj)
> >>      ms->mem_merge = true;
> >>      ms->enable_graphics = true;
> >>
> >> +    if (mc->nvdimm_supported) {
> >> +        ObjectClass *oc = OBJECT_CLASS(mc);
> >> +
> >> +        ms->nvdimms_state = g_new0(NVDIMMState, 1);
> >> +        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, mem-ctrl",
> >> +                                              NULL);
> >> +    }
> >> +
> >> +
> >>      /* Register notifier when init is done for sysbus sanity checks */
> >>      ms->sysbus_notifier.notify = machine_init_notify;
> >>      qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
> >> @@ -809,6 +873,7 @@ static void machine_finalize(Object *obj)
> >>      g_free(ms->dt_compatible);
> >>      g_free(ms->firmware);
> >>      g_free(ms->device_memory);
> >> +    g_free(ms->nvdimms_state);
> >>  }
> >>
> >>  bool machine_usb(MachineState *machine)
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 9ecc96dcc7..416da318ae 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 0338dbe9da..71385cfac9 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);
> >> -    NVDIMMState *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;
> >> @@ -2774,6 +2733,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >>      hc->unplug = pc_machine_device_unplug_cb;
> >>      nc->nmi_monitor_handler = x86_nmi;
> >>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> >> +    mc->nvdimm_supported = true;
> >>
> >>      object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
> >>          pc_machine_get_device_memory_region_size, NULL,
> >> @@ -2798,13 +2758,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..8ad8e885c6 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..372c6b73be 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..e231860666 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -210,6 +210,7 @@ struct MachineClass {
> >>                                   int nb_nodes, ram_addr_t size);
> >>      bool ignore_boot_device_suffixes;
> >>      bool smbus_no_migration_support;
> >> +    bool nvdimm_supported;
> >>
> >>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >>                                             DeviceState *dev);
> >> @@ -272,6 +273,7 @@ struct MachineState {
> >>      const char *cpu_type;
> >>      AccelState *accelerator;
> >>      CPUArchIdList *possible_cpus;
> >> +    struct NVDIMMState *nvdimms_state;
> >
> > What about simply 'nvdimm'?
> > - it is already part of a state
> > - no plural, there is only 1 state
> Well actually I followed Igor's suggestion
> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg601980.html).
>
> The state contains information about all present NVDIMMs (See
> NvdimmFitBuffer comment and "fit: FIT structures for present NVDIMMs").

So 'nvdimms' then :)

Anyway, Igor's call.

> No strong opinion though.
>
> Thanks
>
> Eric
>
>
>
> >
> >>  };
> >>
> >>  #define DEFINE_MACHINE(namestr, machine_initfn) \
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 94fb620d65..263a6343ff 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -45,8 +45,6 @@ struct PCMachineState {
> >>      OnOffAuto vmport;
> >>      OnOffAuto smm;
> >>
> >> -    NVDIMMState 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 mbox series

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 766ca5899d..29a33194a9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -22,6 +22,7 @@ 
 #include "qemu/error-report.h"
 #include "sysemu/qtest.h"
 #include "hw/pci/pci.h"
+#include "hw/mem/nvdimm.h"
 
 GlobalProperty hw_compat_3_1[] = {
     { "pcie-root-port", "x-speed", "2_5" },
@@ -481,6 +482,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);
+    NVDIMMState *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);
@@ -791,6 +833,28 @@  static void machine_initfn(Object *obj)
     ms->mem_merge = true;
     ms->enable_graphics = true;
 
+    if (mc->nvdimm_supported) {
+        ObjectClass *oc = OBJECT_CLASS(mc);
+
+        ms->nvdimms_state = g_new0(NVDIMMState, 1);
+        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, mem-ctrl",
+                                              NULL);
+    }
+
+
     /* Register notifier when init is done for sysbus sanity checks */
     ms->sysbus_notifier.notify = machine_init_notify;
     qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
@@ -809,6 +873,7 @@  static void machine_finalize(Object *obj)
     g_free(ms->dt_compatible);
     g_free(ms->firmware);
     g_free(ms->device_memory);
+    g_free(ms->nvdimms_state);
 }
 
 bool machine_usb(MachineState *machine)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9ecc96dcc7..416da318ae 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 0338dbe9da..71385cfac9 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);
-    NVDIMMState *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;
@@ -2774,6 +2733,7 @@  static void pc_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = pc_machine_device_unplug_cb;
     nc->nmi_monitor_handler = x86_nmi;
     mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
+    mc->nvdimm_supported = true;
 
     object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
         pc_machine_get_device_memory_region_size, NULL,
@@ -2798,13 +2758,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..8ad8e885c6 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..372c6b73be 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..e231860666 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -210,6 +210,7 @@  struct MachineClass {
                                  int nb_nodes, ram_addr_t size);
     bool ignore_boot_device_suffixes;
     bool smbus_no_migration_support;
+    bool nvdimm_supported;
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
@@ -272,6 +273,7 @@  struct MachineState {
     const char *cpu_type;
     AccelState *accelerator;
     CPUArchIdList *possible_cpus;
+    struct NVDIMMState *nvdimms_state;
 };
 
 #define DEFINE_MACHINE(namestr, machine_initfn) \
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 94fb620d65..263a6343ff 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -45,8 +45,6 @@  struct PCMachineState {
     OnOffAuto vmport;
     OnOffAuto smm;
 
-    NVDIMMState 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"