| Message ID | 1444535584-18220-23-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: > >>+ 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. No, I don't think we should add stuff unconditionally. If not nvdimm, some other flag should indicate user intends to hotplug things.
On 10/19/2015 03:39 PM, Michael S. Tsirkin wrote: > On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: >>>> + 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. > > No, I don't think we should add stuff unconditionally. If not nvdimm, > some other flag should indicate user intends to hotplug things. > Actually, it is not unconditionally which is called if parameter "-m aaa, maxmem=bbb" (aaa < bbb) is used. It is on the some path of memoy-hotplug initiation. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 03:44:13PM +0800, Xiao Guangrong wrote: > > > On 10/19/2015 03:39 PM, Michael S. Tsirkin wrote: > >On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: > >>>>+ 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. > > > >No, I don't think we should add stuff unconditionally. If not nvdimm, > >some other flag should indicate user intends to hotplug things. > > > > Actually, it is not unconditionally which is called if parameter "-m aaa, maxmem=bbb" > (aaa < bbb) is used. It is on the some path of memoy-hotplug initiation. > Right, but that's not the same as nvdimm. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 19 Oct 2015 09:56:12 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Sun, Oct 11, 2015 at 11:52:54AM +0800, Xiao Guangrong wrote: [...] > > 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 Michael, If it's ok to map control RAM region directly from QEMU at arbitrary location let's do the same for VMGENID too (i.e. use v16 implementation which does exactly the same thing as this series). > > +#define NVDIMM_ACPI_MEM_SIZE 0xF00000ULL [...] -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 19 Oct 2015 12:17:22 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Oct 19, 2015 at 03:44:13PM +0800, Xiao Guangrong wrote: > > > > > > On 10/19/2015 03:39 PM, Michael S. Tsirkin wrote: > > >On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: > > >>>>+ 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. > > > > > >No, I don't think we should add stuff unconditionally. If not > > >nvdimm, some other flag should indicate user intends to hotplug > > >things. > > > > > > > Actually, it is not unconditionally which is called if parameter > > "-m aaa, maxmem=bbb" (aaa < bbb) is used. It is on the some path of > > memoy-hotplug initiation. > > > > Right, but that's not the same as nvdimm. > it could be pc-machine property, then it could be turned on like this: -machine nvdimm_support=on -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/19/2015 05:46 PM, Igor Mammedov wrote: > On Mon, 19 Oct 2015 12:17:22 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> On Mon, Oct 19, 2015 at 03:44:13PM +0800, Xiao Guangrong wrote: >>> >>> >>> On 10/19/2015 03:39 PM, Michael S. Tsirkin wrote: >>>> On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: >>>>>>> + 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. >>>> >>>> No, I don't think we should add stuff unconditionally. If not >>>> nvdimm, some other flag should indicate user intends to hotplug >>>> things. >>>> >>> >>> Actually, it is not unconditionally which is called if parameter >>> "-m aaa, maxmem=bbb" (aaa < bbb) is used. It is on the some path of >>> memoy-hotplug initiation. >>> >> >> Right, but that's not the same as nvdimm. >> > > it could be pc-machine property, then it could be turned on like this: > -machine nvdimm_support=on Er, I do not understand why this separate switch is needed and why nvdimm and pc-dimm is different. :( NVDIMM reuses memory-hotplug's framework, such as maxmem, slot, and dimm device, even some of ACPI logic to do hotplug things, etc. Both nvdimm and pc-dimm are built on the same infrastructure. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 11:18:36AM +0200, Igor Mammedov wrote: > On Mon, 19 Oct 2015 09:56:12 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Sun, Oct 11, 2015 at 11:52:54AM +0800, Xiao Guangrong wrote: > [...] > > > 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 > Michael, > > If it's ok to map control RAM region directly from QEMU at arbitrary > location It's a fair question. Where is it reserved? In which spec?
On Mon, Oct 19, 2015 at 06:01:17PM +0800, Xiao Guangrong wrote: > > > On 10/19/2015 05:46 PM, Igor Mammedov wrote: > >On Mon, 19 Oct 2015 12:17:22 +0300 > >"Michael S. Tsirkin" <mst@redhat.com> wrote: > > > >>On Mon, Oct 19, 2015 at 03:44:13PM +0800, Xiao Guangrong wrote: > >>> > >>> > >>>On 10/19/2015 03:39 PM, Michael S. Tsirkin wrote: > >>>>On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: > >>>>>>>+ 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. > >>>> > >>>>No, I don't think we should add stuff unconditionally. If not > >>>>nvdimm, some other flag should indicate user intends to hotplug > >>>>things. > >>>> > >>> > >>>Actually, it is not unconditionally which is called if parameter > >>>"-m aaa, maxmem=bbb" (aaa < bbb) is used. It is on the some path of > >>>memoy-hotplug initiation. > >>> > >> > >>Right, but that's not the same as nvdimm. > >> > > > >it could be pc-machine property, then it could be turned on like this: > > -machine nvdimm_support=on > > Er, I do not understand why this separate switch is needed and why nvdimm > and pc-dimm is different. :( > > NVDIMM reuses memory-hotplug's framework, such as maxmem, slot, and dimm > device, even some of ACPI logic to do hotplug things, etc. Both nvdimm > and pc-dimm are built on the same infrastructure. > > > > It does seem to add a bunch of devices in ACPI and memory regions in memory space. Whatever your device is, it's generally safe to assume that most people don't need it.
On Mon, 19 Oct 2015 18:01:17 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > On 10/19/2015 05:46 PM, Igor Mammedov wrote: > > On Mon, 19 Oct 2015 12:17:22 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > >> On Mon, Oct 19, 2015 at 03:44:13PM +0800, Xiao Guangrong wrote: > >>> > >>> > >>> On 10/19/2015 03:39 PM, Michael S. Tsirkin wrote: > >>>> On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: > >>>>>>> + 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. > >>>> > >>>> No, I don't think we should add stuff unconditionally. If not > >>>> nvdimm, some other flag should indicate user intends to hotplug > >>>> things. > >>>> > >>> > >>> Actually, it is not unconditionally which is called if parameter > >>> "-m aaa, maxmem=bbb" (aaa < bbb) is used. It is on the some path > >>> of memoy-hotplug initiation. > >>> > >> > >> Right, but that's not the same as nvdimm. > >> > > > > it could be pc-machine property, then it could be turned on like > > this: -machine nvdimm_support=on > > Er, I do not understand why this separate switch is needed and why > nvdimm and pc-dimm is different. :( > > NVDIMM reuses memory-hotplug's framework, such as maxmem, slot, and > dimm device, even some of ACPI logic to do hotplug things, etc. Both > nvdimm and pc-dimm are built on the same infrastructure. NVDIMM support consumes precious low RAM and MMIO resources and not small amount at that. So turning it on unconditionally with memory hotplug even if NVDIMM wouldn't ever be used isn't nice. However that concern could be dropped if instead of allocating it's own control MMIO/RAM regions, NVDIMM would reuse memory hotplug's MMIO region and replace RAM region with serializing/marshaling label data over the same MMIO interface (yes, it's slower but it's not performance critical path). > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/19/2015 06:25 PM, Michael S. Tsirkin wrote: > On Mon, Oct 19, 2015 at 11:18:36AM +0200, Igor Mammedov wrote: >> On Mon, 19 Oct 2015 09:56:12 +0300 >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >>> On Sun, Oct 11, 2015 at 11:52:54AM +0800, Xiao Guangrong wrote: >> [...] >>>> 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 >> Michael, >> >> If it's ok to map control RAM region directly from QEMU at arbitrary >> location > > It's a fair question. Where is it reserved? In which spec? > The region 0xFF00000 ~ 0xFFF00000 is just reserved for vDSM implementation, it is not required by spec. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/19/2015 06:42 PM, Igor Mammedov wrote: > On Mon, 19 Oct 2015 18:01:17 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> >> >> On 10/19/2015 05:46 PM, Igor Mammedov wrote: >>> On Mon, 19 Oct 2015 12:17:22 +0300 >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>> >>>> On Mon, Oct 19, 2015 at 03:44:13PM +0800, Xiao Guangrong wrote: >>>>> >>>>> >>>>> On 10/19/2015 03:39 PM, Michael S. Tsirkin wrote: >>>>>> On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: >>>>>>>>> + 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. >>>>>> >>>>>> No, I don't think we should add stuff unconditionally. If not >>>>>> nvdimm, some other flag should indicate user intends to hotplug >>>>>> things. >>>>>> >>>>> >>>>> Actually, it is not unconditionally which is called if parameter >>>>> "-m aaa, maxmem=bbb" (aaa < bbb) is used. It is on the some path >>>>> of memoy-hotplug initiation. >>>>> >>>> >>>> Right, but that's not the same as nvdimm. >>>> >>> >>> it could be pc-machine property, then it could be turned on like >>> this: -machine nvdimm_support=on >> >> Er, I do not understand why this separate switch is needed and why >> nvdimm and pc-dimm is different. :( >> >> NVDIMM reuses memory-hotplug's framework, such as maxmem, slot, and >> dimm device, even some of ACPI logic to do hotplug things, etc. Both >> nvdimm and pc-dimm are built on the same infrastructure. > NVDIMM support consumes precious low RAM and MMIO resources and > not small amount at that. So turning it on unconditionally with > memory hotplug even if NVDIMM wouldn't ever be used isn't nice. Okay, understand... will introduce nvdimm_support as your suggestion. Thank you, Igor and Michael! -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 20, 2015 at 01:54:12AM +0800, Xiao Guangrong wrote: > > > On 10/19/2015 06:25 PM, Michael S. Tsirkin wrote: > >On Mon, Oct 19, 2015 at 11:18:36AM +0200, Igor Mammedov wrote: > >>On Mon, 19 Oct 2015 09:56:12 +0300 > >>"Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >>>On Sun, Oct 11, 2015 at 11:52:54AM +0800, Xiao Guangrong wrote: > >>[...] > >>>>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 > >>Michael, > >> > >>If it's ok to map control RAM region directly from QEMU at arbitrary > >>location > > > >It's a fair question. Where is it reserved? In which spec? > > > > The region 0xFF00000 ~ 0xFFF00000 is just reserved for vDSM implementation, > it is not required by spec. See Igor's comment then. You can't just steal it like that.
On 10/19/2015 06:42 PM, Igor Mammedov wrote: > On Mon, 19 Oct 2015 18:01:17 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> >> >> On 10/19/2015 05:46 PM, Igor Mammedov wrote: >>> On Mon, 19 Oct 2015 12:17:22 +0300 >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>> >>>> On Mon, Oct 19, 2015 at 03:44:13PM +0800, Xiao Guangrong wrote: >>>>> >>>>> >>>>> On 10/19/2015 03:39 PM, Michael S. Tsirkin wrote: >>>>>> On Mon, Oct 19, 2015 at 03:27:21PM +0800, Xiao Guangrong wrote: >>>>>>>>> + 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. >>>>>> >>>>>> No, I don't think we should add stuff unconditionally. If not >>>>>> nvdimm, some other flag should indicate user intends to hotplug >>>>>> things. >>>>>> >>>>> >>>>> Actually, it is not unconditionally which is called if parameter >>>>> "-m aaa, maxmem=bbb" (aaa < bbb) is used. It is on the some path >>>>> of memoy-hotplug initiation. >>>>> >>>> >>>> Right, but that's not the same as nvdimm. >>>> >>> >>> it could be pc-machine property, then it could be turned on like >>> this: -machine nvdimm_support=on >> >> Er, I do not understand why this separate switch is needed and why >> nvdimm and pc-dimm is different. :( >> >> NVDIMM reuses memory-hotplug's framework, such as maxmem, slot, and >> dimm device, even some of ACPI logic to do hotplug things, etc. Both >> nvdimm and pc-dimm are built on the same infrastructure. > NVDIMM support consumes precious low RAM and MMIO resources and > not small amount at that. So turning it on unconditionally with > memory hotplug even if NVDIMM wouldn't ever be used isn't nice. > > However that concern could be dropped if instead of allocating it's > own control MMIO/RAM regions, NVDIMM would reuse memory hotplug's MMIO > region and replace RAM region with serializing/marshaling label data > over the same MMIO interface (yes, it's slower but it's not > performance critical path).12 I really do not want to reuse all memory-hotplug's resource, NVDIMM and memory-hotplug do not have the same ACPI logic, that makes the AML code really complex. Another point is, Microsoft does use label data area oon its own way - label data area will not be used as namespace area at all, too slow access for _DSM is not acceptable for vNVDIMM usage. Most important point is, we do not want to slow down system boot with NVDIMM attached, (imagine accessing 128K data with single 8 bytes MMIO access, crazy slowly.), NVDIMM will be use as boot device and it will be used for light-way virtualization,?such as Clear Container and Hyper, which require boot the system up as fast as possible. I understand your concern that reserve big resource is not so acceptable - okay, then how about just reserve 4 bit IO port and 1 RAM? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + 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; + +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
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 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