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.au...@redhat.com> >>>> Suggested-by: Igor Mammedov <imamm...@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.