[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] machine: Move acpi_nvdimm_state into struct
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH v2] machine: Move acpi_nvdimm_state into struct MachineState |
Date: |
Thu, 7 Mar 2019 11:56:35 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 |
Hi Eric, Eduardo,
On 3/7/19 10:06 AM, Eric Auger wrote:
> As NVDIMM support is looming for ARM and SPAPR, let's
> move the acpi_nvdimm_state to the generic machine struct
> instead of duplicating the same code in several machines.
> It is also renamed into nvdimms_state.
>
> nvdimm and nvdimm-persistence become generic machine options.
> We also add a description for those options.
>
> We also remove the nvdimms_state.is_enabled initialization to
> false as objects are guaranteed to be zero initialized.
>
> Signed-off-by: Eric Auger <address@hidden>
> Suggested-by: Igor Mammedov <address@hidden>
>
> ---
>
> v1 -> v2:
> - s/acpi_nvdimm_state/nvdimms_state
> - remove ms->nvdimms_state.is_enabled initialization to false.
> ---
> hw/core/machine.c | 55 +++++++++++++++++++++++++++++++++++++++
> hw/i386/acpi-build.c | 6 ++---
> hw/i386/pc.c | 56 +++-------------------------------------
> hw/i386/pc_piix.c | 4 +--
> hw/i386/pc_q35.c | 4 +--
> include/hw/boards.h | 2 ++
> include/hw/i386/pc.h | 4 ---
> include/hw/mem/pc-dimm.h | 1 -
> 8 files changed, 68 insertions(+), 64 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 766ca5899d..21a7209246 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -481,6 +481,47 @@ static void machine_set_memory_encryption(Object *obj,
> const char *value,
> ms->memory_encryption = g_strdup(value);
> }
>
> +static bool machine_get_nvdimm(Object *obj, Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + return ms->nvdimms_state.is_enabled;
> +}
> +
> +static void machine_set_nvdimm(Object *obj, bool value, Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + ms->nvdimms_state.is_enabled = value;
> +}
> +
> +static char *machine_get_nvdimm_persistence(Object *obj, Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + return g_strdup(ms->nvdimms_state.persistence_string);
> +}
> +
> +static void machine_set_nvdimm_persistence(Object *obj, const char *value,
> + Error **errp)
> +{
> + MachineState *ms = MACHINE(obj);
> + AcpiNVDIMMState *nvdimms_state = &ms->nvdimms_state;
> +
> + if (strcmp(value, "cpu") == 0)
> + nvdimms_state->persistence = 3;
> + else if (strcmp(value, "mem-ctrl") == 0)
> + nvdimms_state->persistence = 2;
> + else {
> + error_setg(errp, "-machine nvdimm-persistence=%s: unsupported
> option",
> + value);
> + return;
> + }
> +
> + g_free(nvdimms_state->persistence_string);
> + nvdimms_state->persistence_string = g_strdup(value);
> +}
> +
> void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char
> *type)
> {
> strList *item = g_new0(strList, 1);
> @@ -765,6 +806,20 @@ static void machine_class_init(ObjectClass *oc, void
> *data)
> &error_abort);
> object_class_property_set_description(oc, "memory-encryption",
> "Set memory encryption object to use", &error_abort);
> +
> + object_class_property_add_bool(oc, "nvdimm",
> + machine_get_nvdimm, machine_set_nvdimm, &error_abort);
> + object_class_property_set_description(oc, "nvdimm",
> + "Set on/off to enable/disable
> NVDIMM "
> + "instantiation", NULL);
> +
> + object_class_property_add_str(oc, "nvdimm-persistence",
> + machine_get_nvdimm_persistence,
> + machine_set_nvdimm_persistence,
> &error_abort);
> + object_class_property_set_description(oc, "nvdimm-persistence",
> + "Set NVDIMM persistence"
> + "Valid values are cpu and
> mem-ctrl",
> + NULL);
> }
>
> static void machine_class_base_init(ObjectClass *oc, void *data)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9ecc96dcc7..2d7d46fe50 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1867,7 +1867,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> aml_append(scope, method);
> }
>
> - if (pcms->acpi_nvdimm_state.is_enabled) {
> + if (machine->nvdimms_state.is_enabled) {
> method = aml_method("_E04", 0, AML_NOTSERIALIZED);
> aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
> aml_int(0x80)));
> @@ -2704,9 +2704,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState
> *machine)
> build_dmar_q35(tables_blob, tables->linker);
> }
> }
> - if (pcms->acpi_nvdimm_state.is_enabled) {
> + if (machine->nvdimms_state.is_enabled) {
> nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> - &pcms->acpi_nvdimm_state, machine->ram_slots);
> + &machine->nvdimms_state, machine->ram_slots);
> }
>
> /* Add tables supplied by user (if any) */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 42128183e9..cacc4068cf 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2069,6 +2069,7 @@ static void pc_memory_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> {
> const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> + const MachineState *ms = MACHINE(hotplug_dev);
> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> const uint64_t legacy_align = TARGET_PAGE_SIZE;
>
> @@ -2083,7 +2084,7 @@ static void pc_memory_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> return;
> }
>
> - if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
> + if (is_nvdimm && !ms->nvdimms_state.is_enabled) {
> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> return;
> }
> @@ -2097,6 +2098,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
> {
> Error *local_err = NULL;
> PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> + MachineState *ms = MACHINE(hotplug_dev);
> bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>
> pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
> @@ -2105,7 +2107,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
> }
>
> if (is_nvdimm) {
> - nvdimm_plug(&pcms->acpi_nvdimm_state);
> + nvdimm_plug(&ms->nvdimms_state);
> }
>
> hotplug_handler_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
> @@ -2546,47 +2548,6 @@ static void pc_machine_set_smm(Object *obj, Visitor
> *v, const char *name,
> visit_type_OnOffAuto(v, name, &pcms->smm, errp);
> }
>
> -static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
> -{
> - PCMachineState *pcms = PC_MACHINE(obj);
> -
> - return pcms->acpi_nvdimm_state.is_enabled;
> -}
> -
> -static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
> -{
> - PCMachineState *pcms = PC_MACHINE(obj);
> -
> - pcms->acpi_nvdimm_state.is_enabled = value;
> -}
> -
> -static char *pc_machine_get_nvdimm_persistence(Object *obj, Error **errp)
> -{
> - PCMachineState *pcms = PC_MACHINE(obj);
> -
> - return g_strdup(pcms->acpi_nvdimm_state.persistence_string);
> -}
> -
> -static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
> - Error **errp)
> -{
> - PCMachineState *pcms = PC_MACHINE(obj);
> - AcpiNVDIMMState *nvdimm_state = &pcms->acpi_nvdimm_state;
> -
> - if (strcmp(value, "cpu") == 0)
> - nvdimm_state->persistence = 3;
> - else if (strcmp(value, "mem-ctrl") == 0)
> - nvdimm_state->persistence = 2;
> - else {
> - error_setg(errp, "-machine nvdimm-persistence=%s: unsupported
> option",
> - value);
> - return;
> - }
> -
> - g_free(nvdimm_state->persistence_string);
> - nvdimm_state->persistence_string = g_strdup(value);
> -}
> -
> static bool pc_machine_get_smbus(Object *obj, Error **errp)
> {
> PCMachineState *pcms = PC_MACHINE(obj);
> @@ -2636,8 +2597,6 @@ static void pc_machine_initfn(Object *obj)
> pcms->max_ram_below_4g = 0; /* use default */
> pcms->smm = ON_OFF_AUTO_AUTO;
> pcms->vmport = ON_OFF_AUTO_AUTO;
> - /* nvdimm is disabled on default. */
> - pcms->acpi_nvdimm_state.is_enabled = false;
> /* acpi build is enabled by default if machine supports it */
> pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
> pcms->smbus_enabled = true;
> @@ -2798,13 +2757,6 @@ static void pc_machine_class_init(ObjectClass *oc,
> void *data)
> object_class_property_set_description(oc, PC_MACHINE_VMPORT,
> "Enable vmport (pc & q35)", &error_abort);
>
> - object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
> - pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
> -
> - object_class_property_add_str(oc, PC_MACHINE_NVDIMM_PERSIST,
> - pc_machine_get_nvdimm_persistence,
> - pc_machine_set_nvdimm_persistence, &error_abort);
> -
> object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
> pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 8770ecada9..16ebfc5a5a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -297,8 +297,8 @@ static void pc_init1(MachineState *machine,
> PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
> }
>
> - if (pcms->acpi_nvdimm_state.is_enabled) {
> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> + if (machine->nvdimms_state.is_enabled) {
> + nvdimm_init_acpi_state(&machine->nvdimms_state, system_io,
> pcms->fw_cfg, OBJECT(pcms));
> }
> }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index cfb9043e12..dacaa90611 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -329,8 +329,8 @@ static void pc_q35_init(MachineState *machine)
> pc_vga_init(isa_bus, host_bus);
> pc_nic_init(pcmc, isa_bus, host_bus);
>
> - if (pcms->acpi_nvdimm_state.is_enabled) {
> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> + if (machine->nvdimms_state.is_enabled) {
> + nvdimm_init_acpi_state(&machine->nvdimms_state, system_io,
> pcms->fw_cfg, OBJECT(pcms));
> }
> }
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 9690c71a6d..ccf0c5a69d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -8,6 +8,7 @@
> #include "hw/qdev.h"
> #include "qom/object.h"
> #include "qom/cpu.h"
> +#include "hw/mem/nvdimm.h"
>
> /**
> * memory_region_allocate_system_memory - Allocate a board's main memory
> @@ -272,6 +273,7 @@ struct MachineState {
> const char *cpu_type;
> AccelState *accelerator;
> CPUArchIdList *possible_cpus;
> + AcpiNVDIMMState nvdimms_state;
It looks we are breaking the generic/abstract machine design.
You introduce 2 specific concepts here, ACPI and NVDIMM.
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) \
>
Re: [Qemu-devel] [PATCH v2] machine: Move acpi_nvdimm_state into struct MachineState, Eduardo Habkost, 2019/03/07