From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [Qemu-devel] [PATCH v2 07/18] nvdimm: reserve address range for NVDIMM Date: Sun, 6 Sep 2015 15:22:07 +0800 Message-ID: <55EBE99F.40604@linux.intel.com> References: <1439563931-12352-1-git-send-email-guangrong.xiao@linux.intel.com> <1439563931-12352-8-git-send-email-guangrong.xiao@linux.intel.com> <20150904140227.779b0687@nial.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: pbonzini@redhat.com, ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, rth@twiddle.net To: Igor Mammedov Return-path: Received: from mga14.intel.com ([192.55.52.115]:28365 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750964AbbIFH2E (ORCPT ); Sun, 6 Sep 2015 03:28:04 -0400 In-Reply-To: <20150904140227.779b0687@nial.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 09/04/2015 08:02 PM, Igor Mammedov wrote: > On Fri, 14 Aug 2015 22:52:00 +0800 > Xiao Guangrong wrote: > >> NVDIMM reserves all the free range above 4G to do: >> - Persistent Memory (PMEM) mapping >> - implement NVDIMM ACPI device _DSM method >> >> Signed-off-by: Xiao Guangrong >> --- >> hw/i386/pc.c | 12 ++++++++++-- >> hw/mem/nvdimm/pc-nvdimm.c | 13 +++++++++++++ >> include/hw/mem/pc-nvdimm.h | 1 + >> 3 files changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 7661ea9..41af6ea 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -64,6 +64,7 @@ >> #include "hw/pci/pci_host.h" >> #include "acpi-build.h" >> #include "hw/mem/pc-dimm.h" >> +#include "hw/mem/pc-nvdimm.h" >> #include "qapi/visitor.h" >> #include "qapi-visit.h" >> >> @@ -1302,6 +1303,7 @@ FWCfgState *pc_memory_init(MachineState *machine, >> MemoryRegion *ram_below_4g, *ram_above_4g; >> FWCfgState *fw_cfg; >> PCMachineState *pcms = PC_MACHINE(machine); >> + ram_addr_t offset; >> >> assert(machine->ram_size == below_4g_mem_size + above_4g_mem_size); >> >> @@ -1339,6 +1341,8 @@ FWCfgState *pc_memory_init(MachineState *machine, >> exit(EXIT_FAILURE); >> } >> >> + offset = 0x100000000ULL + above_4g_mem_size; >> + >> /* initialize hotplug memory address space */ >> if (guest_info->has_reserved_memory && >> (machine->ram_size < machine->maxram_size)) { >> @@ -1358,8 +1362,7 @@ FWCfgState *pc_memory_init(MachineState *machine, >> exit(EXIT_FAILURE); >> } >> >> - pcms->hotplug_memory.base = >> - ROUND_UP(0x100000000ULL + above_4g_mem_size, 1ULL << 30); >> + pcms->hotplug_memory.base = ROUND_UP(offset, 1ULL << 30); >> >> if (pcms->enforce_aligned_dimm) { >> /* size hotplug region assuming 1G page max alignment per slot */ >> @@ -1377,8 +1380,13 @@ FWCfgState *pc_memory_init(MachineState *machine, >> "hotplug-memory", hotplug_mem_size); >> memory_region_add_subregion(system_memory, pcms->hotplug_memory.base, >> &pcms->hotplug_memory.mr); >> + >> + offset = pcms->hotplug_memory.base + hotplug_mem_size; >> } >> >> + /* all the space left above 4G is reserved for NVDIMM. */ >> + pc_nvdimm_reserve_range(offset); > I'd drop 'offset' in this patch and just use: > foo(pcms->hotplug_memory.base + hotplug_mem_size) > That works only if hotplug is used... however we can enable nvdimm separately. >> + >> /* Initialize PC system firmware */ >> pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw); >> >> diff --git a/hw/mem/nvdimm/pc-nvdimm.c b/hw/mem/nvdimm/pc-nvdimm.c >> index a53d235..7a270a8 100644 >> --- a/hw/mem/nvdimm/pc-nvdimm.c >> +++ b/hw/mem/nvdimm/pc-nvdimm.c >> @@ -24,6 +24,19 @@ >> >> #include "hw/mem/pc-nvdimm.h" >> >> +#define PAGE_SIZE (1UL << 12) >> + >> +static struct nvdimms_info { >> + ram_addr_t current_addr; >> +} nvdimms_info; > no globals please, so far it looks like pcms->hotplug_memory > so add asimmilar nvdimm_memory field to PCMachineState > Okay, it's good to me. >> + >> +/* the address range [offset, ~0ULL) is reserved for NVDIMM. */ >> +void pc_nvdimm_reserve_range(ram_addr_t offset) > do you plan to reuse this function, if not then just inline it at call site I prefer it as a inline function and move it to the nvdimm.h file since it's easier to port it to other platforms - avoid to find the pieces of code related to nvdimm in x86 arch and only needed to implement the functions in nvdimm.h. > >> +{ >> + offset = ROUND_UP(offset, PAGE_SIZE); > I'd suggest round up to 1Gb as we do with mem hotplug Okay, good to me. Really appreciate for all your time/comment in the whole patchset, Igor!