Re: [Qemu-devel] [PATCH v3 22/32] nvdimm: init the address region used by NVDIMM ACPI

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Oct 11, 2015 at 11:52:54AM +0800, Xiao Guangrong wrote:
> We reserve the memory region 0xFF00000 ~ 0xFFF00000 for NVDIMM ACPI
> which is used as:
> - the first page is mapped as MMIO, ACPI write data to this page to
>   transfer the control to QEMU
> 
> - the second page is RAM-based which used to save the input info of
>   _DSM method and QEMU reuse it store output info
> 
> - the left is mapped as RAM, it's the buffer returned by _FIT method,
>   this is needed by NVDIMM hotplug
> 

Isn't there some way to document this in code, e.g. with
macros?

Adding text under docs/specs would also be a good idea.


> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/i386/pc.c            |   3 ++
>  hw/mem/Makefile.objs    |   2 +-
>  hw/mem/nvdimm/acpi.c    | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h    |   2 +
>  include/hw/mem/nvdimm.h |  19 ++++++++
>  5 files changed, 145 insertions(+), 1 deletion(-)
>  create mode 100644 hw/mem/nvdimm/acpi.c
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6694b18..8fea4c3 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1360,6 +1360,9 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
>              exit(EXIT_FAILURE);
>          }
>  
> +        nvdimm_init_memory_state(&pcms->nvdimm_memory, system_memory, machine,
> +                                 TARGET_PAGE_SIZE);
> +

Shouldn't this be conditional on presence of the nvdimm device?


>          pcms->hotplug_memory.base =
>              ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 30);
>  
> diff --git a/hw/mem/Makefile.objs b/hw/mem/Makefile.objs
> index e0ff328..7310bac 100644
> --- a/hw/mem/Makefile.objs
> +++ b/hw/mem/Makefile.objs
> @@ -1,3 +1,3 @@
>  common-obj-$(CONFIG_DIMM) += dimm.o
>  common-obj-$(CONFIG_MEM_HOTPLUG) += pc-dimm.o
> -common-obj-$(CONFIG_NVDIMM) += nvdimm/nvdimm.o
> +common-obj-$(CONFIG_NVDIMM) += nvdimm/nvdimm.o nvdimm/acpi.o
> diff --git a/hw/mem/nvdimm/acpi.c b/hw/mem/nvdimm/acpi.c
> new file mode 100644
> index 0000000..b640874
> --- /dev/null
> +++ b/hw/mem/nvdimm/acpi.c
> @@ -0,0 +1,120 @@
> +/*
> + * NVDIMM ACPI Implementation
> + *
> + * Copyright(C) 2015 Intel Corporation.
> + *
> + * Author:
> + *  Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + *
> + * NFIT is defined in ACPI 6.0: 5.2.25 NVDIMM Firmware Interface Table (NFIT)
> + * and the DSM specfication can be found at:
> + *       http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
> + *
> + * Currently, it only supports PMEM Virtualization.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu-common.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/mem/nvdimm.h"
> +#include "internal.h"
> +
> +/* System Physical Address Range Structure */
> +struct nfit_spa {
> +    uint16_t type;
> +    uint16_t length;
> +    uint16_t spa_index;
> +    uint16_t flags;
> +    uint32_t reserved;
> +    uint32_t proximity_domain;
> +    uint8_t type_guid[16];
> +    uint64_t spa_base;
> +    uint64_t spa_length;
> +    uint64_t mem_attr;
> +} QEMU_PACKED;
> +typedef struct nfit_spa nfit_spa;
> +
> +/* Memory Device to System Physical Address Range Mapping Structure */
> +struct nfit_memdev {
> +    uint16_t type;
> +    uint16_t length;
> +    uint32_t nfit_handle;
> +    uint16_t phys_id;
> +    uint16_t region_id;
> +    uint16_t spa_index;
> +    uint16_t dcr_index;
> +    uint64_t region_len;
> +    uint64_t region_offset;
> +    uint64_t region_dpa;
> +    uint16_t interleave_index;
> +    uint16_t interleave_ways;
> +    uint16_t flags;
> +    uint16_t reserved;
> +} QEMU_PACKED;
> +typedef struct nfit_memdev nfit_memdev;
> +
> +/* NVDIMM Control Region Structure */
> +struct nfit_dcr {
> +    uint16_t type;
> +    uint16_t length;
> +    uint16_t dcr_index;
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +    uint16_t revision_id;
> +    uint16_t sub_vendor_id;
> +    uint16_t sub_device_id;
> +    uint16_t sub_revision_id;
> +    uint8_t reserved[6];
> +    uint32_t serial_number;
> +    uint16_t fic;
> +    uint16_t num_bcw;
> +    uint64_t bcw_size;
> +    uint64_t cmd_offset;
> +    uint64_t cmd_size;
> +    uint64_t status_offset;
> +    uint64_t status_size;
> +    uint16_t flags;
> +    uint8_t reserved2[6];
> +} QEMU_PACKED;
> +typedef struct nfit_dcr nfit_dcr;

Struct naming violates the QEMU coding style.
Pls fix it.

> +
> +static uint64_t nvdimm_device_structure_size(uint64_t slots)
> +{
> +    /* each nvdimm has three structures. */
> +    return slots * (sizeof(nfit_spa) + sizeof(nfit_memdev) + sizeof(nfit_dcr));
> +}
> +
> +static uint64_t nvdimm_acpi_memory_size(uint64_t slots, uint64_t page_size)
> +{
> +    uint64_t size = nvdimm_device_structure_size(slots);
> +
> +    /* two pages for nvdimm _DSM method. */
> +    return size + page_size * 2;
> +}
> +
> +void nvdimm_init_memory_state(NVDIMMState *state, MemoryRegion*system_memory,
> +                              MachineState *machine , uint64_t page_size)
> +{
> +    QEMU_BUILD_BUG_ON(nvdimm_acpi_memory_size(ACPI_MAX_RAM_SLOTS,
> +                                   page_size) >= NVDIMM_ACPI_MEM_SIZE);
> +
> +    state->base = NVDIMM_ACPI_MEM_BASE;
> +    state->page_size = page_size;
> +
> +    memory_region_init(&state->mr, OBJECT(machine), "nvdimm-acpi",
> +                       NVDIMM_ACPI_MEM_SIZE);
> +    memory_region_add_subregion(system_memory, state->base, &state->mr);
> +}
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 693b6c5..fd65c27 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/dimm.h"
> +#include "hw/mem/nvdimm.h"
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
> @@ -32,6 +33,7 @@ struct PCMachineState {
>  
>      /* <public> */
>      MemoryHotplugState hotplug_memory;
> +    NVDIMMState nvdimm_memory;
>  
>      HotplugHandler *acpi_dev;
>      ISADevice *rtc;
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index f6bd2c4..aa95961 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -15,6 +15,10 @@
>  
>  #include "hw/mem/dimm.h"
>  
> +/* Memory region 0xFF00000 ~ 0xFFF00000 is reserved for NVDIMM ACPI. */
> +#define NVDIMM_ACPI_MEM_BASE   0xFF000000ULL
> +#define NVDIMM_ACPI_MEM_SIZE   0xF00000ULL
> +
>  #define TYPE_NVDIMM "nvdimm"
>  #define NVDIMM(obj) \
>      OBJECT_CHECK(NVDIMMDevice, (obj), TYPE_NVDIMM)
> @@ -30,4 +34,19 @@ struct NVDIMMDevice {
>  };
>  typedef struct NVDIMMDevice NVDIMMDevice;
>  
> +/*
> + * NVDIMMState:
> + * @base: address in guest address space where NVDIMM ACPI memory begins.
> + * @page_size: the page size of target platform.
> + * @mr: NVDIMM ACPI memory address space container.
> + */
> +struct NVDIMMState {
> +    ram_addr_t base;
> +    uint64_t page_size;
> +    MemoryRegion mr;
> +};
> +typedef struct NVDIMMState NVDIMMState;
> +
> +void nvdimm_init_memory_state(NVDIMMState *state, MemoryRegion*system_memory,
> +                              MachineState *machine , uint64_t page_size);
>  #endif
> -- 
> 1.8.3.1