| Message ID | 1439563931-12352-8-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 14, 2015 at 10:52:00PM +0800, Xiao Guangrong wrote: > 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) This macro name is likely to collide with system headers or other code. Could you use the existing TARGET_PAGE_SIZE constant instead? -- 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 Fri, Aug 14, 2015 at 10:52:00PM +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 <guangrong.xiao@linux.intel.com> > --- > 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(-) CCing Igor for memory hotplug-related changes. > 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); > + > /* 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; > + > +/* the address range [offset, ~0ULL) is reserved for NVDIMM. */ > +void pc_nvdimm_reserve_range(ram_addr_t offset) > +{ > + offset = ROUND_UP(offset, PAGE_SIZE); > + nvdimms_info.current_addr = offset; > +} > + > static char *get_file(Object *obj, Error **errp) > { > PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj); > diff --git a/include/hw/mem/pc-nvdimm.h b/include/hw/mem/pc-nvdimm.h > index 51152b8..8601e9b 100644 > --- a/include/hw/mem/pc-nvdimm.h > +++ b/include/hw/mem/pc-nvdimm.h > @@ -28,4 +28,5 @@ typedef struct PCNVDIMMDevice { > #define PC_NVDIMM(obj) \ > OBJECT_CHECK(PCNVDIMMDevice, (obj), TYPE_PC_NVDIMM) > > +void pc_nvdimm_reserve_range(ram_addr_t offset); > #endif > -- > 2.4.3 > > -- > 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 08/25/2015 11:12 PM, Stefan Hajnoczi wrote: > On Fri, Aug 14, 2015 at 10:52:00PM +0800, Xiao Guangrong wrote: >> 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) > > This macro name is likely to collide with system headers or other code. > > Could you use the existing TARGET_PAGE_SIZE constant instead? > Will follow your way in the next version. Thank you for pointing it out. -- 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 Fri, Aug 14, 2015 at 10:52:00PM +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 <guangrong.xiao@linux.intel.com> [...] > @@ -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; "offset" is a very generic name. I suggest naming it "nvdimm_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 */
Hi Eduardo, Thank you for reviewing my patches. On 08/29/2015 01:25 AM, Eduardo Habkost wrote: > On Fri, Aug 14, 2015 at 10:52:00PM +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 <guangrong.xiao@linux.intel.com> > [...] >> @@ -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; > > "offset" is a very generic name. I suggest naming it "nvdimm_offset". 'offset' is used for generic proposal as it is not only used for nvdimm but also for calculating hotplug_mem_base: pcms->hotplug_memory.base = ROUND_UP(offset, 1ULL << 30); -- 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 Fri, 14 Aug 2015 22:52:00 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 <guangrong.xiao@linux.intel.com> > --- > 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) > + > /* 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 > + > +/* 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 > +{ > + offset = ROUND_UP(offset, PAGE_SIZE); I'd suggest round up to 1Gb as we do with mem hotplug > + nvdimms_info.current_addr = offset; > +} > + > static char *get_file(Object *obj, Error **errp) > { > PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj); > diff --git a/include/hw/mem/pc-nvdimm.h b/include/hw/mem/pc-nvdimm.h > index 51152b8..8601e9b 100644 > --- a/include/hw/mem/pc-nvdimm.h > +++ b/include/hw/mem/pc-nvdimm.h > @@ -28,4 +28,5 @@ typedef struct PCNVDIMMDevice { > #define PC_NVDIMM(obj) \ > OBJECT_CHECK(PCNVDIMMDevice, (obj), TYPE_PC_NVDIMM) > > +void pc_nvdimm_reserve_range(ram_addr_t offset); > #endif -- 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 09/04/2015 08:02 PM, Igor Mammedov wrote: > On Fri, 14 Aug 2015 22:52:00 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 <guangrong.xiao@linux.intel.com> >> --- >> 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! -- 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 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); + /* 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; + +/* the address range [offset, ~0ULL) is reserved for NVDIMM. */ +void pc_nvdimm_reserve_range(ram_addr_t offset) +{ + offset = ROUND_UP(offset, PAGE_SIZE); + nvdimms_info.current_addr = offset; +} + static char *get_file(Object *obj, Error **errp) { PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj); diff --git a/include/hw/mem/pc-nvdimm.h b/include/hw/mem/pc-nvdimm.h index 51152b8..8601e9b 100644 --- a/include/hw/mem/pc-nvdimm.h +++ b/include/hw/mem/pc-nvdimm.h @@ -28,4 +28,5 @@ typedef struct PCNVDIMMDevice { #define PC_NVDIMM(obj) \ OBJECT_CHECK(PCNVDIMMDevice, (obj), TYPE_PC_NVDIMM) +void pc_nvdimm_reserve_range(ram_addr_t offset); #endif
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 <guangrong.xiao@linux.intel.com> --- 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(-)