Re: [PATCH v2 07/18] nvdimm: reserve address range for NVDIMM

[Xiao Guangrong]

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 

[...]

@@ -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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 07/18] nvdimm: reserve address range for NVDIMM

[Eduardo Habkost]

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.x...@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 = 0x1ULL + 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(0x1ULL + 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 */

-- 
Eduardo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 07/18] nvdimm: reserve address range for NVDIMM

[Stefan Hajnoczi]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 07/18] nvdimm: reserve address range for NVDIMM

[Stefan Hajnoczi]

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.x...@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 = 0x1ULL + 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(0x1ULL + 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 majord...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html