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

[Xiao Guangrong]

On 10/19/2015 02:56 PM, Michael S. Tsirkin wrote:
> 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?

Yes. It's also documented when DSM memory is created, see
nvdimm_build_dsm_memory() introduced in
[PATCH v4 25/33] nvdimm acpi: init the address region used by DSM

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

Yes. A doc has been added in v4.

> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >   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?

We will enable hotplug on nvdimm devices in the near future once Linux driver is
ready. I'd keep it here for future development.

> >           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 <address@hidden>
> > + *
> > + * 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.

I got it. Will add nvdimm_ prefix.