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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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 <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?
>

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 <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.

I got it. Will add nvdimm_ prefix.