Re: [PATCH v2 06/11] nvdimm acpi: initialize the resource used by NVDIMM ACPI

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Cc: pbonzini@redhat.com, imammedo@redhat.com, gleb@kernel.org,
	mtosatti@redhat.com, stefanha@redhat.com, rth@twiddle.net,
	ehabkost@redhat.com, dan.j.williams@intel.com,
	kvm@vger.kernel.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 06/11] nvdimm acpi: initialize the resource used by NVDIMM ACPI
Date: Thu, 4 Feb 2016 18:22:44 +0200	[thread overview]
Message-ID: <20160204182217-mutt-send-email-mst@redhat.com> (raw)
In-Reply-To: <1452624610-46945-7-git-send-email-guangrong.xiao@linux.intel.com>

On Wed, Jan 13, 2016 at 02:50:05AM +0800, Xiao Guangrong wrote:
> 32 bits IO port starting from 0x0a18 in guest is reserved for NVDIMM
> ACPI emulation. The table, NVDIMM_DSM_MEM_FILE, will be patched into
> NVDIMM ACPI binary code
> 
> OSPM uses this port to tell QEMU the final address of the DSM memory
> and notify QEMU to emulate the DSM method
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>

This will have to be rebased now that guest info is gone.

> ---
>  hw/acpi/Makefile.objs   |  2 +-
>  hw/acpi/nvdimm.c        | 35 +++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c    | 10 +---------
>  hw/i386/pc.c            |  8 +++++---
>  hw/i386/pc_piix.c       |  5 +++++
>  hw/i386/pc_q35.c        |  8 +++++++-
>  include/hw/i386/pc.h    |  5 ++++-
>  include/hw/mem/nvdimm.h | 28 +++++++++++++++++++++++++++-
>  8 files changed, 85 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index f3ade9a..faee86c 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -2,7 +2,7 @@ common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
>  common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
>  common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o cpu_hotplug_acpi_table.o
>  common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o memory_hotplug_acpi_table.o
> -common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
> +obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>  common-obj-$(CONFIG_ACPI) += acpi_interface.o
>  common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
>  common-obj-$(CONFIG_ACPI) += aml-build.o
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index df1b176..256cedd 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -28,6 +28,7 @@
>  
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/aml-build.h"
> +#include "hw/nvram/fw_cfg.h"
>  #include "hw/mem/nvdimm.h"
>  
>  static int nvdimm_plugged_device_list(Object *obj, void *opaque)
> @@ -369,6 +370,40 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
>      g_array_free(structures, true);
>  }
>  
> +static uint64_t
> +nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return 0;
> +}
> +
> +static void
> +nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +}
> +
> +static const MemoryRegionOps nvdimm_dsm_ops = {
> +    .read = nvdimm_dsm_read,
> +    .write = nvdimm_dsm_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
> +                            FWCfgState *fw_cfg, Object *owner)
> +{
> +    memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
> +                          "nvdimm-acpi-io", NVDIMM_ACPI_IO_LEN);
> +    memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr);
> +
> +    state->dsm_mem = g_array_new(false, true /* clear */, 1);
> +    acpi_data_push(state->dsm_mem, TARGET_PAGE_SIZE);
> +    fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
> +                    state->dsm_mem->len);
> +}
> +
>  #define NVDIMM_COMMON_DSM      "NCAL"
>  
>  static void nvdimm_build_common_dsm(Aml *dev)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1ca044f..c68cfb8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -39,7 +39,6 @@
>  #include "hw/loader.h"
>  #include "hw/isa/isa.h"
>  #include "hw/acpi/memory_hotplug.h"
> -#include "hw/mem/nvdimm.h"
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
>  #include "sysemu/tpm_backend.h"
> @@ -2602,13 +2601,6 @@ static bool acpi_has_iommu(void)
>      return intel_iommu && !ambiguous;
>  }
>  
> -static bool acpi_has_nvdimm(void)
> -{
> -    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> -
> -    return pcms->nvdimm;
> -}
> -
>  static
>  void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>  {
> @@ -2692,7 +2684,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>          build_dmar_q35(tables_blob, tables->linker);
>      }
>  
> -    if (acpi_has_nvdimm()) {
> +    if (guest_info->has_nvdimm) {
>          nvdimm_build_acpi(table_offsets, tables_blob, tables->linker);
>      }
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c36b8cf..397de28 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1228,6 +1228,8 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>          }
>      }
>  
> +    guest_info->has_nvdimm = pcms->acpi_nvdimm_state.is_enabled;
> +
>      guest_info_state->machine_done.notify = pc_guest_info_machine_done;
>      qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
>      return guest_info;
> @@ -1877,14 +1879,14 @@ static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
>  
> -    return pcms->nvdimm;
> +    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->nvdimm = value;
> +    pcms->acpi_nvdimm_state.is_enabled = value;
>  }
>  
>  static void pc_machine_initfn(Object *obj)
> @@ -1923,7 +1925,7 @@ static void pc_machine_initfn(Object *obj)
>                                      &error_abort);
>  
>      /* nvdimm is disabled on default. */
> -    pcms->nvdimm = false;
> +    pcms->acpi_nvdimm_state.is_enabled = false;
>      object_property_add_bool(obj, PC_MACHINE_NVDIMM, pc_machine_get_nvdimm,
>                               pc_machine_set_nvdimm, &error_abort);
>  }
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 438cdae..2fee478 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -281,6 +281,11 @@ static void pc_init1(MachineState *machine,
>      if (pcmc->pci_enabled) {
>          pc_pci_device_init(pci_bus);
>      }
> +
> +    if (guest_info->has_nvdimm) {
> +        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> +                               guest_info->fw_cfg, OBJECT(pcms));
> +    }
>  }
>  
>  /* Looking for a pc_compat_2_4() function? It doesn't exist.
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 412b3cd..c9334d5 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -60,6 +60,7 @@ static void pc_q35_init(MachineState *machine)
>      PCIDevice *lpc;
>      BusState *idebus[MAX_SATA_PORTS];
>      ISADevice *rtc_state;
> +    MemoryRegion *system_io = get_system_io();
>      MemoryRegion *pci_memory;
>      MemoryRegion *rom_memory;
>      MemoryRegion *ram_memory;
> @@ -176,7 +177,7 @@ static void pc_q35_init(MachineState *machine)
>      q35_host->mch.ram_memory = ram_memory;
>      q35_host->mch.pci_address_space = pci_memory;
>      q35_host->mch.system_memory = get_system_memory();
> -    q35_host->mch.address_space_io = get_system_io();
> +    q35_host->mch.address_space_io = system_io;
>      q35_host->mch.below_4g_mem_size = pcms->below_4g_mem_size;
>      q35_host->mch.above_4g_mem_size = pcms->above_4g_mem_size;
>      /* pci */
> @@ -267,6 +268,11 @@ static void pc_q35_init(MachineState *machine)
>      if (pcmc->pci_enabled) {
>          pc_pci_device_init(host_bus);
>      }
> +
> +    if (guest_info->has_nvdimm) {
> +        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> +                               guest_info->fw_cfg, OBJECT(pcms));
> +    }
>  }
>  
>  /* Looking for a pc_compat_2_4() function? It doesn't exist.
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 8122229..362ddc4 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -17,6 +17,7 @@
>  #include "hw/boards.h"
>  #include "hw/compat.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/nvdimm.h"
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
> @@ -55,7 +56,8 @@ struct PCMachineState {
>      uint64_t max_ram_below_4g;
>      OnOffAuto vmport;
>      OnOffAuto smm;
> -    bool nvdimm;
> +
> +    AcpiNVDIMMState acpi_nvdimm_state;
>  
>      /* RAM information (sizes, addresses, configuration): */
>      ram_addr_t below_4g_mem_size, above_4g_mem_size;
> @@ -161,6 +163,7 @@ struct PcGuestInfo {
>      bool has_acpi_build;
>      bool has_reserved_memory;
>      bool rsdp_in_ram;
> +    bool has_nvdimm;
>  };
>  
>  /* parallel.c */
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 49183c1..634c60b 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -25,8 +25,34 @@
>  
>  #include "hw/mem/pc-dimm.h"
>  
> -#define TYPE_NVDIMM      "nvdimm"
> +#define TYPE_NVDIMM             "nvdimm"
>  
> +#define NVDIMM_DSM_MEM_FILE     "etc/acpi/nvdimm-mem"
> +
> +/*
> + * 32 bits IO port starting from 0x0a18 in guest is reserved for
> + * NVDIMM ACPI emulation.
> + */
> +#define NVDIMM_ACPI_IO_BASE     0x0a18
> +#define NVDIMM_ACPI_IO_LEN      4
> +
> +/*
> + * AcpiNVDIMMState:
> + * @is_enabled: detect if NVDIMM support is enabled.
> + *
> + * @dsm_mem: the data of the fw_cfg file NVDIMM_DSM_MEM_FILE.
> + * @io_mr: the IO region used by OSPM to transfer control to QEMU.
> + */
> +struct AcpiNVDIMMState {
> +    bool is_enabled;
> +
> +    GArray *dsm_mem;
> +    MemoryRegion io_mr;
> +};
> +typedef struct AcpiNVDIMMState AcpiNVDIMMState;
> +
> +void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
> +                            FWCfgState *fw_cfg, Object *owner);
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         GArray *linker);
>  #endif
> -- 
> 1.8.3.1