[Qemu-devel] [PATCH v3 0/5] Refactoring pc_dimm_plug and NUMA node lookup API

[Qemu-devel] [PATCH v3 0/5] Refactoring pc_dimm_plug and NUMA node lookup API

[Bharata B Rao]

Hi,

Here is the v3 of the patchset that refactors pc_dimm_plug and adds
an API to lookup NUMA node by address.

- Refactoring pc_dimm_plug() helps other architectures like PowerPC
  to make use of common code.
- API to lookup NUMA node id by address is required to support memory
  hotplug on PowerPC sPAPR guests.

The patchset that adds memory hotplug support to PowerPC sPAPR that
was posted at
http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg06574.html
depends on this patchset.

Changes in v3:
-------------
- Not calling pc_dimm_memory_unplug() when pc_dimm_memory_plug() fails
  because when pc_dimm_memory_plug() fails, it wouldn't have done
  memory_region_add_subregion() and vmstate_register_ram().
- As per Igor's suggestion, moving of hotplug related fields from
  PCMachineState is now in a separate patch.

v2: https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05157.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg03212.html
v0: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01078.html

Bharata B Rao (5):
  pc,pc-dimm: Extract hotplug related fields in PCMachineState to a
    structure
  pc,pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate
    routine
  numa,pc-dimm: Store pc-dimm memory information in numa_info
  numa: Store boot memory address range in node_info
  numa: API to lookup NUMA node by address

 hw/i386/acpi-build.c     |  2 +-
 hw/i386/pc.c             | 84 +++++++++---------------------------------------
 hw/mem/pc-dimm.c         | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h     |  7 ++--
 include/hw/mem/pc-dimm.h | 15 +++++++++
 include/sysemu/numa.h    | 11 +++++++
 numa.c                   | 77 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 205 insertions(+), 75 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v3 1/5] pc, pc-dimm: Extract hotplug related fields in PCMachineState to a structure

[Bharata B Rao]

Move hotplug_memory_base and hotplug_memory fields of PCMachineState
into a separate structure so that the same can be made use of from
other architectures supporing memory hotplug.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/i386/acpi-build.c     |  2 +-
 hw/i386/pc.c             | 26 +++++++++++++-------------
 include/hw/i386/pc.h     |  7 ++-----
 include/hw/mem/pc-dimm.h | 11 +++++++++++
 4 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b71e942..5f6fa95 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1512,7 +1512,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
      */
     if (hotplugabble_address_space_size) {
         numamem = acpi_data_push(table_data, sizeof *numamem);
-        acpi_build_srat_memory(numamem, pcms->hotplug_memory_base,
+        acpi_build_srat_memory(numamem, pcms->hotplug_memory.base,
                                hotplugabble_address_space_size, 0,
                                MEM_AFFINITY_HOTPLUGGABLE |
                                MEM_AFFINITY_ENABLED);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7072930..e51c3be 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1297,7 +1297,7 @@ FWCfgState *pc_memory_init(MachineState *machine,
             exit(EXIT_FAILURE);
         }
 
-        pcms->hotplug_memory_base =
+        pcms->hotplug_memory.base =
             ROUND_UP(0x100000000ULL + above_4g_mem_size, 1ULL << 30);
 
         if (pcms->enforce_aligned_dimm) {
@@ -1305,17 +1305,17 @@ FWCfgState *pc_memory_init(MachineState *machine,
             hotplug_mem_size += (1ULL << 30) * machine->ram_slots;
         }
 
-        if ((pcms->hotplug_memory_base + hotplug_mem_size) <
+        if ((pcms->hotplug_memory.base + hotplug_mem_size) <
             hotplug_mem_size) {
             error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
                          machine->maxram_size);
             exit(EXIT_FAILURE);
         }
 
-        memory_region_init(&pcms->hotplug_memory, OBJECT(pcms),
+        memory_region_init(&pcms->hotplug_memory.mr, OBJECT(pcms),
                            "hotplug-memory", hotplug_mem_size);
-        memory_region_add_subregion(system_memory, pcms->hotplug_memory_base,
-                                    &pcms->hotplug_memory);
+        memory_region_add_subregion(system_memory, pcms->hotplug_memory.base,
+                                    &pcms->hotplug_memory.mr);
     }
 
     /* Initialize PC system firmware */
@@ -1333,9 +1333,9 @@ FWCfgState *pc_memory_init(MachineState *machine,
     fw_cfg = bochs_bios_init();
     rom_set_fw(fw_cfg);
 
-    if (guest_info->has_reserved_memory && pcms->hotplug_memory_base) {
+    if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
         uint64_t *val = g_malloc(sizeof(*val));
-        *val = cpu_to_le64(ROUND_UP(pcms->hotplug_memory_base, 0x1ULL << 30));
+        *val = cpu_to_le64(ROUND_UP(pcms->hotplug_memory.base, 0x1ULL << 30));
         fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, sizeof(*val));
     }
 
@@ -1575,8 +1575,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         align = memory_region_get_alignment(mr);
     }
 
-    addr = pc_dimm_get_free_addr(pcms->hotplug_memory_base,
-                                 memory_region_size(&pcms->hotplug_memory),
+    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
+                                 memory_region_size(&pcms->hotplug_memory.mr),
                                  !addr ? NULL : &addr, align,
                                  memory_region_size(mr), &local_err);
     if (local_err) {
@@ -1630,8 +1630,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    memory_region_add_subregion(&pcms->hotplug_memory,
-                                addr - pcms->hotplug_memory_base, mr);
+    memory_region_add_subregion(&pcms->hotplug_memory.mr,
+                                addr - pcms->hotplug_memory.base, mr);
     vmstate_register_ram(mr, dev);
 
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
@@ -1677,7 +1677,7 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    memory_region_del_subregion(&pcms->hotplug_memory, mr);
+    memory_region_del_subregion(&pcms->hotplug_memory.mr, mr);
     vmstate_unregister_ram(mr, dev);
 
     object_unparent(OBJECT(dev));
@@ -1766,7 +1766,7 @@ pc_machine_get_hotplug_memory_region_size(Object *obj, Visitor *v, void *opaque,
                                           const char *name, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
-    int64_t value = memory_region_size(&pcms->hotplug_memory);
+    int64_t value = memory_region_size(&pcms->hotplug_memory.mr);
 
     visit_type_int(v, &value, name, errp);
 }
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 86c5651..328c8f7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -15,14 +15,12 @@
 #include "hw/pci/pci.h"
 #include "hw/boards.h"
 #include "hw/compat.h"
+#include "hw/mem/pc-dimm.h"
 
 #define HPET_INTCAP "hpet-intcap"
 
 /**
  * PCMachineState:
- * @hotplug_memory_base: address in guest RAM address space where hotplug memory
- * address space begins.
- * @hotplug_memory: hotplug memory addess space container
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
  * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
  *                        backend's alignment value if provided
@@ -32,8 +30,7 @@ struct PCMachineState {
     MachineState parent_obj;
 
     /* <public> */
-    ram_addr_t hotplug_memory_base;
-    MemoryRegion hotplug_memory;
+    MemoryHotplugState hotplug_memory;
 
     HotplugHandler *acpi_dev;
     ISADevice *rtc;
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index f7b80b4..d528a3f 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -70,6 +70,17 @@ typedef struct PCDIMMDeviceClass {
     MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
 } PCDIMMDeviceClass;
 
+/**
+ * MemoryHotplugState:
+ * @base: address in guest RAM address space where hotplug memory
+ * address space begins.
+ * @mr: hotplug memory addess space container
+ */
+typedef struct MemoryHotplugState {
+    ram_addr_t base;
+    MemoryRegion mr;
+} MemoryHotplugState;
+
 uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
                                uint64_t address_space_size,
                                uint64_t *hint, uint64_t align, uint64_t size,
-- 
2.1.0

[Qemu-devel] [PATCH v3 2/5] pc, pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate routine

[Bharata B Rao]

pc_dimm_plug() has code that will be needed for memory plug handlers
in other archs too. Extract code from pc_dimm_plug() into a generic
routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
correspondingly refactor re-usable unplug code into pc_dimm_memory_unplug().

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/i386/pc.c             | 66 ++++-----------------------------------
 hw/mem/pc-dimm.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/mem/pc-dimm.h |  4 +++
 3 files changed, 90 insertions(+), 60 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e51c3be..cb57612 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -64,7 +64,6 @@
 #include "hw/pci/pci_host.h"
 #include "acpi-build.h"
 #include "hw/mem/pc-dimm.h"
-#include "trace.h"
 #include "qapi/visitor.h"
 #include "qapi-visit.h"
 
@@ -1554,88 +1553,35 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
 static void pc_dimm_plug(HotplugHandler *hotplug_dev,
                          DeviceState *dev, Error **errp)
 {
-    int slot;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    MachineState *machine = MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr = ddc->get_memory_region(dimm);
-    uint64_t existing_dimms_capacity = 0;
     uint64_t align = TARGET_PAGE_SIZE;
-    uint64_t addr;
-
-    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
 
     if (memory_region_get_alignment(mr) && pcms->enforce_aligned_dimm) {
         align = memory_region_get_alignment(mr);
     }
 
-    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
-                                 memory_region_size(&pcms->hotplug_memory.mr),
-                                 !addr ? NULL : &addr, align,
-                                 memory_region_size(mr), &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    if (existing_dimms_capacity + memory_region_size(mr) >
-        machine->maxram_size - machine->ram_size) {
-        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
-                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
-                   existing_dimms_capacity,
-                   machine->maxram_size - machine->ram_size);
-        goto out;
-    }
-
-    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    trace_mhp_pc_dimm_assigned_address(addr);
-
-    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
-                                 machine->ram_slots, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    trace_mhp_pc_dimm_assigned_slot(slot);
-
     if (!pcms->acpi_dev) {
         error_setg(&local_err,
                    "memory hotplug is not enabled: missing acpi device");
         goto out;
     }
 
-    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
-        error_setg(&local_err, "hypervisor has no free memory slots left");
+    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
+    if (local_err) {
         goto out;
     }
 
-    memory_region_add_subregion(&pcms->hotplug_memory.mr,
-                                addr - pcms->hotplug_memory.base, mr);
-    vmstate_register_ram(mr, dev);
-
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+    if (local_err) {
+        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
+    }
+
 out:
     error_propagate(errp, local_err);
 }
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index e70633d..98971b7 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -23,12 +23,92 @@
 #include "qapi/visitor.h"
 #include "qemu/range.h"
 #include "sysemu/numa.h"
+#include "sysemu/kvm.h"
+#include "trace.h"
 
 typedef struct pc_dimms_capacity {
      uint64_t size;
      Error    **errp;
 } pc_dimms_capacity;
 
+void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
+                         MemoryRegion *mr, uint64_t align, Error **errp)
+{
+    int slot;
+    MachineState *machine = MACHINE(qdev_get_machine());
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    Error *local_err = NULL;
+    uint64_t existing_dimms_capacity = 0;
+    uint64_t addr;
+
+    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    addr = pc_dimm_get_free_addr(hpms->base,
+                                 memory_region_size(&hpms->mr),
+                                 !addr ? NULL : &addr, align,
+                                 memory_region_size(mr), &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    if (existing_dimms_capacity + memory_region_size(mr) >
+        machine->maxram_size - machine->ram_size) {
+        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
+                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
+                   existing_dimms_capacity,
+                   machine->maxram_size - machine->ram_size);
+        goto out;
+    }
+
+    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    trace_mhp_pc_dimm_assigned_address(addr);
+
+    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
+                                 machine->ram_slots, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    trace_mhp_pc_dimm_assigned_slot(slot);
+
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(&local_err, "hypervisor has no free memory slots left");
+        goto out;
+    }
+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
+    vmstate_register_ram(mr, dev);
+
+out:
+    error_propagate(errp, local_err);
+}
+
+void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
+                           MemoryRegion *mr)
+{
+    memory_region_del_subregion(&hpms->mr, mr);
+    vmstate_unregister_ram(mr, dev);
+}
+
 static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
 {
     pc_dimms_capacity *cap = opaque;
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index d528a3f..3eabef4 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -90,4 +90,8 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
 int qmp_pc_dimm_device_list(Object *obj, void *opaque);
 uint64_t pc_existing_dimms_capacity(Error **errp);
+void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
+                         MemoryRegion *mr, uint64_t align, Error **errp);
+void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
+                           MemoryRegion *mr);
 #endif
-- 
2.1.0

[Qemu-devel] [PATCH v3 3/5] numa, pc-dimm: Store pc-dimm memory information in numa_info

[Bharata B Rao]

Start storing the (start_addr, size, nodeid) of the pc-dimm memory
in numa_info so that this information can be used to lookup
node by address.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/mem/pc-dimm.c      |  4 ++++
 include/sysemu/numa.h | 10 ++++++++++
 numa.c                | 26 ++++++++++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 98971b7..bb04862 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -97,6 +97,7 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
 
     memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
     vmstate_register_ram(mr, dev);
+    numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
 
 out:
     error_propagate(errp, local_err);
@@ -105,6 +106,9 @@ out:
 void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
                            MemoryRegion *mr)
 {
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+
+    numa_unset_mem_node_id(dimm->addr, memory_region_size(mr), dimm->node);
     memory_region_del_subregion(&hpms->mr, mr);
     vmstate_unregister_ram(mr, dev);
 }
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 6523b4d..7176364 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -10,16 +10,26 @@
 
 extern int nb_numa_nodes;   /* Number of NUMA nodes */
 
+struct numa_addr_range {
+    ram_addr_t mem_start;
+    ram_addr_t mem_end;
+    QLIST_ENTRY(numa_addr_range) entry;
+};
+
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
     bool present;
+    QLIST_HEAD(, numa_addr_range) addr; /* List to store address ranges */
 } NodeInfo;
+
 extern NodeInfo numa_info[MAX_NODES];
 void parse_numa_opts(MachineClass *mc);
 void numa_post_machine_init(void);
 void query_numa_node_mem(uint64_t node_mem[]);
 extern QemuOptsList qemu_numa_opts;
+void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
+void numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
 
 #endif
diff --git a/numa.c b/numa.c
index 91fc6c1..1f58231 100644
--- a/numa.c
+++ b/numa.c
@@ -52,6 +52,28 @@ static int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
 int nb_numa_nodes;
 NodeInfo numa_info[MAX_NODES];
 
+void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node)
+{
+    struct numa_addr_range *range = g_malloc0(sizeof(*range));
+
+    range->mem_start = addr;
+    range->mem_end = addr + size;
+    QLIST_INSERT_HEAD(&numa_info[node].addr, range, entry);
+}
+
+void numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node)
+{
+    struct numa_addr_range *range, *next;
+
+    QLIST_FOREACH_SAFE(range, &numa_info[node].addr, entry, next) {
+        if (addr == range->mem_start && (addr + size) == range->mem_end) {
+            QLIST_REMOVE(range, entry);
+            g_free(range);
+            return;
+        }
+    }
+}
+
 static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
 {
     uint16_t nodenr;
@@ -274,6 +296,10 @@ void parse_numa_opts(MachineClass *mc)
         }
 
         for (i = 0; i < nb_numa_nodes; i++) {
+            QLIST_INIT(&numa_info[i].addr);
+        }
+
+        for (i = 0; i < nb_numa_nodes; i++) {
             if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
                 break;
             }
-- 
2.1.0

[Qemu-devel] [PATCH v3 4/5] numa: Store boot memory address range in node_info

[Bharata B Rao]

Store memory address range information of boot memory  in address
range list of numa_info.

This helps to have a common NUMA node lookup by address function that
works for both boot time memory and hotplugged memory.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 numa.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/numa.c b/numa.c
index 1f58231..5d37bf9 100644
--- a/numa.c
+++ b/numa.c
@@ -74,6 +74,21 @@ void numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node)
     }
 }
 
+static void numa_set_mem_ranges(void)
+{
+    int i;
+    ram_addr_t mem_start = 0;
+
+    /*
+     * Deduce start address of each node and use it to store
+     * the address range info in numa_info address range list
+     */
+    for (i = 0; i < nb_numa_nodes; i++) {
+        numa_set_mem_node_id(mem_start, numa_info[i].node_mem, i);
+        mem_start += numa_info[i].node_mem;
+    }
+}
+
 static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
 {
     uint16_t nodenr;
@@ -299,6 +314,8 @@ void parse_numa_opts(MachineClass *mc)
             QLIST_INIT(&numa_info[i].addr);
         }
 
+        numa_set_mem_ranges();
+
         for (i = 0; i < nb_numa_nodes; i++) {
             if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
                 break;
-- 
2.1.0

[Qemu-devel] [PATCH v3 5/5] numa: API to lookup NUMA node by address

[Bharata B Rao]

Introduce an API numa_get_node(ram_addr_t addr, Error **errp) that
returns the NUMA node to which the given address belongs to. This
API works uniformly for both boot time as well as hotplugged memory.

This API is needed by sPAPR PowerPC to support
ibm,dynamic-reconfiguration-memory device tree node which is needed for
memory hotplug.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 include/sysemu/numa.h |  1 +
 numa.c                | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 7176364..a6392bc 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -31,5 +31,6 @@ void query_numa_node_mem(uint64_t node_mem[]);
 extern QemuOptsList qemu_numa_opts;
 void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
 void numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
+uint32_t numa_get_node(ram_addr_t addr, Error **errp);
 
 #endif
diff --git a/numa.c b/numa.c
index 5d37bf9..bcf00a5 100644
--- a/numa.c
+++ b/numa.c
@@ -89,6 +89,40 @@ static void numa_set_mem_ranges(void)
     }
 }
 
+/*
+ * Check if @addr falls under NUMA @node.
+ */
+static bool numa_addr_belongs_to_node(ram_addr_t addr, uint32_t node)
+{
+    struct numa_addr_range *range;
+
+    QLIST_FOREACH(range, &numa_info[node].addr, entry) {
+        if (addr >= range->mem_start && addr < range->mem_end) {
+            return true;
+        }
+    }
+    return false;
+}
+
+/*
+ * Given an address, return the index of the NUMA node to which the
+ * address belongs to.
+ */
+uint32_t numa_get_node(ram_addr_t addr, Error **errp)
+{
+    uint32_t i;
+
+    for (i = 0; i < nb_numa_nodes; i++) {
+        if (numa_addr_belongs_to_node(addr, i)) {
+            return i;
+        }
+    }
+
+    error_setg(errp, "Address 0x" RAM_ADDR_FMT " doesn't belong to any "
+                "NUMA node", addr);
+    return -1;
+}
+
 static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
 {
     uint16_t nodenr;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 4/5] numa: Store boot memory address range in node_info

[Bharata B Rao]

On Fri, Jun 26, 2015 at 09:36:03AM +0530, Bharata B Rao wrote:
> Store memory address range information of boot memory  in address
> range list of numa_info.
> 
> This helps to have a common NUMA node lookup by address function that
> works for both boot time memory and hotplugged memory.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Missed including that.

Re: [Qemu-devel] [PATCH v3 1/5] pc, pc-dimm: Extract hotplug related fields in PCMachineState to a structure

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On Fri, Jun 26, 2015 at 09:36:00AM +0530, Bharata B Rao wrote:
> Move hotplug_memory_base and hotplug_memory fields of PCMachineState
> into a separate structure so that the same can be made use of from
> other architectures supporing memory hotplug.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] pc, pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate routine

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

On Fri, Jun 26, 2015 at 09:36:01AM +0530, Bharata B Rao wrote:
> pc_dimm_plug() has code that will be needed for memory plug handlers
> in other archs too. Extract code from pc_dimm_plug() into a generic
> routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
> correspondingly refactor re-usable unplug code into pc_dimm_memory_unplug().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 4/5] numa: Store boot memory address range in node_info

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Fri, Jun 26, 2015 at 09:36:03AM +0530, Bharata B Rao wrote:
> Store memory address range information of boot memory  in address
> range list of numa_info.
> 
> This helps to have a common NUMA node lookup by address function that
> works for both boot time memory and hotplugged memory.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] Refactoring pc_dimm_plug and NUMA node lookup API

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]

On Fri, Jun 26, 2015 at 09:35:59AM +0530, Bharata B Rao wrote:
> Hi,
> 
> Here is the v3 of the patchset that refactors pc_dimm_plug and adds
> an API to lookup NUMA node by address.
> 
> - Refactoring pc_dimm_plug() helps other architectures like PowerPC
>   to make use of common code.
> - API to lookup NUMA node id by address is required to support memory
>   hotplug on PowerPC sPAPR guests.
> 
> The patchset that adds memory hotplug support to PowerPC sPAPR that
> was posted at
> http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg06574.html
> depends on this patchset.
> 
> Changes in v3:
> -------------
> - Not calling pc_dimm_memory_unplug() when pc_dimm_memory_plug() fails
>   because when pc_dimm_memory_plug() fails, it wouldn't have done
>   memory_region_add_subregion() and vmstate_register_ram().
> - As per Igor's suggestion, moving of hotplug related fields from
>   PCMachineState is now in a separate patch.
> 
> v2: https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05157.html
> v1: https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg03212.html
> v0: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01078.html
> 
> Bharata B Rao (5):
>   pc,pc-dimm: Extract hotplug related fields in PCMachineState to a
>     structure
>   pc,pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate
>     routine
>   numa,pc-dimm: Store pc-dimm memory information in numa_info
>   numa: Store boot memory address range in node_info
>   numa: API to lookup NUMA node by address
> 
>  hw/i386/acpi-build.c     |  2 +-
>  hw/i386/pc.c             | 84 +++++++++---------------------------------------
>  hw/mem/pc-dimm.c         | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h     |  7 ++--
>  include/hw/mem/pc-dimm.h | 15 +++++++++
>  include/sysemu/numa.h    | 11 +++++++
>  numa.c                   | 77 ++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 205 insertions(+), 75 deletions(-)

Looks good to me.  I'm hoping this can be merged pretty soon, so I can
then pull the actual pseries hotplug memory stuff into spapr-next.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] pc, pc-dimm: Extract hotplug related fields in PCMachineState to a structure

[Igor Mammedov]

On Fri, 26 Jun 2015 09:36:00 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Move hotplug_memory_base and hotplug_memory fields of PCMachineState
> into a separate structure so that the same can be made use of from
> other architectures supporing memory hotplug.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/i386/acpi-build.c     |  2 +-
>  hw/i386/pc.c             | 26 +++++++++++++-------------
>  include/hw/i386/pc.h     |  7 ++-----
>  include/hw/mem/pc-dimm.h | 11 +++++++++++
>  4 files changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b71e942..5f6fa95 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1512,7 +1512,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
>       */
>      if (hotplugabble_address_space_size) {
>          numamem = acpi_data_push(table_data, sizeof *numamem);
> -        acpi_build_srat_memory(numamem, pcms->hotplug_memory_base,
> +        acpi_build_srat_memory(numamem, pcms->hotplug_memory.base,
>                                 hotplugabble_address_space_size, 0,
>                                 MEM_AFFINITY_HOTPLUGGABLE |
>                                 MEM_AFFINITY_ENABLED);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7072930..e51c3be 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1297,7 +1297,7 @@ FWCfgState *pc_memory_init(MachineState *machine,
>              exit(EXIT_FAILURE);
>          }
>  
> -        pcms->hotplug_memory_base =
> +        pcms->hotplug_memory.base =
>              ROUND_UP(0x100000000ULL + above_4g_mem_size, 1ULL << 30);
>  
>          if (pcms->enforce_aligned_dimm) {
> @@ -1305,17 +1305,17 @@ FWCfgState *pc_memory_init(MachineState *machine,
>              hotplug_mem_size += (1ULL << 30) * machine->ram_slots;
>          }
>  
> -        if ((pcms->hotplug_memory_base + hotplug_mem_size) <
> +        if ((pcms->hotplug_memory.base + hotplug_mem_size) <
>              hotplug_mem_size) {
>              error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
>                           machine->maxram_size);
>              exit(EXIT_FAILURE);
>          }
>  
> -        memory_region_init(&pcms->hotplug_memory, OBJECT(pcms),
> +        memory_region_init(&pcms->hotplug_memory.mr, OBJECT(pcms),
>                             "hotplug-memory", hotplug_mem_size);
> -        memory_region_add_subregion(system_memory, pcms->hotplug_memory_base,
> -                                    &pcms->hotplug_memory);
> +        memory_region_add_subregion(system_memory, pcms->hotplug_memory.base,
> +                                    &pcms->hotplug_memory.mr);
>      }
>  
>      /* Initialize PC system firmware */
> @@ -1333,9 +1333,9 @@ FWCfgState *pc_memory_init(MachineState *machine,
>      fw_cfg = bochs_bios_init();
>      rom_set_fw(fw_cfg);
>  
> -    if (guest_info->has_reserved_memory && pcms->hotplug_memory_base) {
> +    if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
>          uint64_t *val = g_malloc(sizeof(*val));
> -        *val = cpu_to_le64(ROUND_UP(pcms->hotplug_memory_base, 0x1ULL << 30));
> +        *val = cpu_to_le64(ROUND_UP(pcms->hotplug_memory.base, 0x1ULL << 30));
>          fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, sizeof(*val));
>      }
>  
> @@ -1575,8 +1575,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          align = memory_region_get_alignment(mr);
>      }
>  
> -    addr = pc_dimm_get_free_addr(pcms->hotplug_memory_base,
> -                                 memory_region_size(&pcms->hotplug_memory),
> +    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
> +                                 memory_region_size(&pcms->hotplug_memory.mr),
>                                   !addr ? NULL : &addr, align,
>                                   memory_region_size(mr), &local_err);
>      if (local_err) {
> @@ -1630,8 +1630,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> -    memory_region_add_subregion(&pcms->hotplug_memory,
> -                                addr - pcms->hotplug_memory_base, mr);
> +    memory_region_add_subregion(&pcms->hotplug_memory.mr,
> +                                addr - pcms->hotplug_memory.base, mr);
>      vmstate_register_ram(mr, dev);
>  
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> @@ -1677,7 +1677,7 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> -    memory_region_del_subregion(&pcms->hotplug_memory, mr);
> +    memory_region_del_subregion(&pcms->hotplug_memory.mr, mr);
>      vmstate_unregister_ram(mr, dev);
>  
>      object_unparent(OBJECT(dev));
> @@ -1766,7 +1766,7 @@ pc_machine_get_hotplug_memory_region_size(Object *obj, Visitor *v, void *opaque,
>                                            const char *name, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> -    int64_t value = memory_region_size(&pcms->hotplug_memory);
> +    int64_t value = memory_region_size(&pcms->hotplug_memory.mr);
>  
>      visit_type_int(v, &value, name, errp);
>  }
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 86c5651..328c8f7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -15,14 +15,12 @@
>  #include "hw/pci/pci.h"
>  #include "hw/boards.h"
>  #include "hw/compat.h"
> +#include "hw/mem/pc-dimm.h"
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
>  /**
>   * PCMachineState:
> - * @hotplug_memory_base: address in guest RAM address space where hotplug memory
> - * address space begins.
> - * @hotplug_memory: hotplug memory addess space container
>   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
>   * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
>   *                        backend's alignment value if provided
> @@ -32,8 +30,7 @@ struct PCMachineState {
>      MachineState parent_obj;
>  
>      /* <public> */
> -    ram_addr_t hotplug_memory_base;
> -    MemoryRegion hotplug_memory;
> +    MemoryHotplugState hotplug_memory;
>  
>      HotplugHandler *acpi_dev;
>      ISADevice *rtc;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index f7b80b4..d528a3f 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -70,6 +70,17 @@ typedef struct PCDIMMDeviceClass {
>      MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>  
> +/**
> + * MemoryHotplugState:
> + * @base: address in guest RAM address space where hotplug memory
> + * address space begins.
> + * @mr: hotplug memory addess space container
granted typo was in original code, but in case of respin pls fix it up

s/addess/address/

> + */
> +typedef struct MemoryHotplugState {
> +    ram_addr_t base;
> +    MemoryRegion mr;
> +} MemoryHotplugState;
> +
>  uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
>                                 uint64_t address_space_size,
>                                 uint64_t *hint, uint64_t align, uint64_t size,

Re: [Qemu-devel] [PATCH v3 2/5] pc, pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate routine

[Igor Mammedov]

On Fri, 26 Jun 2015 09:36:01 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> pc_dimm_plug() has code that will be needed for memory plug handlers
> in other archs too. Extract code from pc_dimm_plug() into a generic
> routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
> correspondingly refactor re-usable unplug code into pc_dimm_memory_unplug().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
thanks for splitting, much easier to review.


> ---
>  hw/i386/pc.c             | 66 ++++-----------------------------------
>  hw/mem/pc-dimm.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/mem/pc-dimm.h |  4 +++
>  3 files changed, 90 insertions(+), 60 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e51c3be..cb57612 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -64,7 +64,6 @@
>  #include "hw/pci/pci_host.h"
>  #include "acpi-build.h"
>  #include "hw/mem/pc-dimm.h"
> -#include "trace.h"
>  #include "qapi/visitor.h"
>  #include "qapi-visit.h"
>  
> @@ -1554,88 +1553,35 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>                           DeviceState *dev, Error **errp)
>  {
> -    int slot;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -    MachineState *machine = MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t existing_dimms_capacity = 0;
>      uint64_t align = TARGET_PAGE_SIZE;
> -    uint64_t addr;
> -
> -    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
>  
>      if (memory_region_get_alignment(mr) && pcms->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
>  
> -    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
> -                                 memory_region_size(&pcms->hotplug_memory.mr),
> -                                 !addr ? NULL : &addr, align,
> -                                 memory_region_size(mr), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    if (existing_dimms_capacity + memory_region_size(mr) >
> -        machine->maxram_size - machine->ram_size) {
> -        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> -                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> -                   existing_dimms_capacity,
> -                   machine->maxram_size - machine->ram_size);
> -        goto out;
> -    }
> -
> -    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    trace_mhp_pc_dimm_assigned_address(addr);
> -
> -    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> -                                 machine->ram_slots, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    trace_mhp_pc_dimm_assigned_slot(slot);
> -
>      if (!pcms->acpi_dev) {
>          error_setg(&local_err,
>                     "memory hotplug is not enabled: missing acpi device");
>          goto out;
>      }
>  
> -    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> -        error_setg(&local_err, "hypervisor has no free memory slots left");
> +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
> +    if (local_err) {
>          goto out;
>      }
>  
> -    memory_region_add_subregion(&pcms->hotplug_memory.mr,
> -                                addr - pcms->hotplug_memory.base, mr);
> -    vmstate_register_ram(mr, dev);
> -
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> +    if (local_err) {
> +        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
> +    }
You are adding something new while patch claims to be just moving code.
I'm not sure it's save to call unplug here if hhc->plug() failed
since it might have triggered guest visible side effects.

I'd just drop this hunk and leave original code,
on top of that extra patch for hhc->plug(s/local_err/error_abort/)
might make a sense since hhc->plug() should never fail and if it's
then we should fix error in code instead of trying to recover.

>  out:
>      error_propagate(errp, local_err);
>  }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index e70633d..98971b7 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -23,12 +23,92 @@
>  #include "qapi/visitor.h"
>  #include "qemu/range.h"
>  #include "sysemu/numa.h"
> +#include "sysemu/kvm.h"
> +#include "trace.h"
>  
>  typedef struct pc_dimms_capacity {
>       uint64_t size;
>       Error    **errp;
>  } pc_dimms_capacity;
>  
> +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> +                         MemoryRegion *mr, uint64_t align, Error **errp)
> +{
> +    int slot;
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    Error *local_err = NULL;
> +    uint64_t existing_dimms_capacity = 0;
> +    uint64_t addr;
> +
> +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    addr = pc_dimm_get_free_addr(hpms->base,
> +                                 memory_region_size(&hpms->mr),
> +                                 !addr ? NULL : &addr, align,
> +                                 memory_region_size(mr), &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (existing_dimms_capacity + memory_region_size(mr) >
> +        machine->maxram_size - machine->ram_size) {
> +        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> +                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> +                   existing_dimms_capacity,
> +                   machine->maxram_size - machine->ram_size);
> +        goto out;
> +    }
> +
> +    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    trace_mhp_pc_dimm_assigned_address(addr);
> +
> +    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> +                                 machine->ram_slots, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    trace_mhp_pc_dimm_assigned_slot(slot);
> +
> +    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> +        error_setg(&local_err, "hypervisor has no free memory slots left");
> +        goto out;
> +    }
> +
> +    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> +    vmstate_register_ram(mr, dev);
> +
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
> +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> +                           MemoryRegion *mr)
> +{
> +    memory_region_del_subregion(&hpms->mr, mr);
> +    vmstate_unregister_ram(mr, dev);
> +}
if you adding this here then you probably should be removing/replacing
related hunks from pc.c, I don't see it in this patch.

> +
>  static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
>  {
>      pc_dimms_capacity *cap = opaque;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index d528a3f..3eabef4 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -90,4 +90,8 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
> +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> +                         MemoryRegion *mr, uint64_t align, Error **errp);
> +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> +                           MemoryRegion *mr);
>  #endif

Re: [Qemu-devel] [PATCH v3 3/5] numa, pc-dimm: Store pc-dimm memory information in numa_info

[Igor Mammedov]

On Fri, 26 Jun 2015 09:36:02 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Start storing the (start_addr, size, nodeid) of the pc-dimm memory
> in numa_info so that this information can be used to lookup
> node by address.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/mem/pc-dimm.c      |  4 ++++
>  include/sysemu/numa.h | 10 ++++++++++
>  numa.c                | 26 ++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 98971b7..bb04862 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -97,6 +97,7 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
>  
>      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
>      vmstate_register_ram(mr, dev);
> +    numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
>  
>  out:
>      error_propagate(errp, local_err);
> @@ -105,6 +106,9 @@ out:
>  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
>                             MemoryRegion *mr)
>  {
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +
> +    numa_unset_mem_node_id(dimm->addr, memory_region_size(mr), dimm->node);
>      memory_region_del_subregion(&hpms->mr, mr);
>      vmstate_unregister_ram(mr, dev);
>  }
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 6523b4d..7176364 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -10,16 +10,26 @@
>  
>  extern int nb_numa_nodes;   /* Number of NUMA nodes */
>  
> +struct numa_addr_range {
> +    ram_addr_t mem_start;
> +    ram_addr_t mem_end;
> +    QLIST_ENTRY(numa_addr_range) entry;
> +};
> +
>  typedef struct node_info {
>      uint64_t node_mem;
>      DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
>      struct HostMemoryBackend *node_memdev;
>      bool present;
> +    QLIST_HEAD(, numa_addr_range) addr; /* List to store address ranges */
>  } NodeInfo;
> +
>  extern NodeInfo numa_info[MAX_NODES];
>  void parse_numa_opts(MachineClass *mc);
>  void numa_post_machine_init(void);
>  void query_numa_node_mem(uint64_t node_mem[]);
>  extern QemuOptsList qemu_numa_opts;
> +void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
> +void numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
>  
>  #endif
> diff --git a/numa.c b/numa.c
> index 91fc6c1..1f58231 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -52,6 +52,28 @@ static int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
>  int nb_numa_nodes;
>  NodeInfo numa_info[MAX_NODES];
>  
> +void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node)
> +{
> +    struct numa_addr_range *range = g_malloc0(sizeof(*range));
> +
> +    range->mem_start = addr;
> +    range->mem_end = addr + size;
off by one?
for me mem_end mean last address in range and not an address past it.
so suggest:

assert(size);
range->mem_end = addr + size - 1;

with fixups in following code
> +    QLIST_INSERT_HEAD(&numa_info[node].addr, range, entry);
> +}
> +
> +void numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node)
> +{
> +    struct numa_addr_range *range, *next;
> +
> +    QLIST_FOREACH_SAFE(range, &numa_info[node].addr, entry, next) {
> +        if (addr == range->mem_start && (addr + size) == range->mem_end) {
> +            QLIST_REMOVE(range, entry);
> +            g_free(range);
> +            return;
> +        }
> +    }
> +}
> +
>  static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
>  {
>      uint16_t nodenr;
> @@ -274,6 +296,10 @@ void parse_numa_opts(MachineClass *mc)
>          }
>  
>          for (i = 0; i < nb_numa_nodes; i++) {
> +            QLIST_INIT(&numa_info[i].addr);
> +        }
> +
> +        for (i = 0; i < nb_numa_nodes; i++) {
>              if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
>                  break;
>              }

Re: [Qemu-devel] [PATCH v3 2/5] pc, pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate routine

[Bharata B Rao]

On Fri, Jun 26, 2015 at 10:21:54AM +0200, Igor Mammedov wrote:
> On Fri, 26 Jun 2015 09:36:01 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > pc_dimm_plug() has code that will be needed for memory plug handlers
> > in other archs too. Extract code from pc_dimm_plug() into a generic
> > routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
> > correspondingly refactor re-usable unplug code into pc_dimm_memory_unplug().
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> thanks for splitting, much easier to review.

Yes it is much easier to see the changes and thanks for the review.

> 
> 
> > ---
> >  hw/i386/pc.c             | 66 ++++-----------------------------------
> >  hw/mem/pc-dimm.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/mem/pc-dimm.h |  4 +++
> >  3 files changed, 90 insertions(+), 60 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e51c3be..cb57612 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -64,7 +64,6 @@
> >  #include "hw/pci/pci_host.h"
> >  #include "acpi-build.h"
> >  #include "hw/mem/pc-dimm.h"
> > -#include "trace.h"
> >  #include "qapi/visitor.h"
> >  #include "qapi-visit.h"
> >  
> > @@ -1554,88 +1553,35 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> >  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >                           DeviceState *dev, Error **errp)
> >  {
> > -    int slot;
> >      HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > -    MachineState *machine = MACHINE(hotplug_dev);
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >      MemoryRegion *mr = ddc->get_memory_region(dimm);
> > -    uint64_t existing_dimms_capacity = 0;
> >      uint64_t align = TARGET_PAGE_SIZE;
> > -    uint64_t addr;
> > -
> > -    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> >  
> >      if (memory_region_get_alignment(mr) && pcms->enforce_aligned_dimm) {
> >          align = memory_region_get_alignment(mr);
> >      }
> >  
> > -    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
> > -                                 memory_region_size(&pcms->hotplug_memory.mr),
> > -                                 !addr ? NULL : &addr, align,
> > -                                 memory_region_size(mr), &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -
> > -    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -
> > -    if (existing_dimms_capacity + memory_region_size(mr) >
> > -        machine->maxram_size - machine->ram_size) {
> > -        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> > -                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> > -                   existing_dimms_capacity,
> > -                   machine->maxram_size - machine->ram_size);
> > -        goto out;
> > -    }
> > -
> > -    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -    trace_mhp_pc_dimm_assigned_address(addr);
> > -
> > -    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -
> > -    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> > -                                 machine->ram_slots, &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -    trace_mhp_pc_dimm_assigned_slot(slot);
> > -
> >      if (!pcms->acpi_dev) {
> >          error_setg(&local_err,
> >                     "memory hotplug is not enabled: missing acpi device");
> >          goto out;
> >      }
> >  
> > -    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > -        error_setg(&local_err, "hypervisor has no free memory slots left");
> > +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
> > +    if (local_err) {
> >          goto out;
> >      }
> >  
> > -    memory_region_add_subregion(&pcms->hotplug_memory.mr,
> > -                                addr - pcms->hotplug_memory.base, mr);
> > -    vmstate_register_ram(mr, dev);
> > -
> >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> >      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> > +    if (local_err) {
> > +        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
> > +    }
> You are adding something new while patch claims to be just moving code.
> I'm not sure it's save to call unplug here if hhc->plug() failed
> since it might have triggered guest visible side effects.

In sPAPR, the routine equivalent to hhc->plug() could fail and
I needed pc_dimm_memory_unplug() to recover. But now I have rearranged
the code there to ensure that it never fails like in x86. However
I need pc_dimm_memory_unplug() nevertheless in sPAPR.

> 
> I'd just drop this hunk and leave original code,
> on top of that extra patch for hhc->plug(s/local_err/error_abort/)
> might make a sense since hhc->plug() should never fail and if it's
> then we should fix error in code instead of trying to recover.

I have added a small patch for this in my series.

> 
> >  out:
> >      error_propagate(errp, local_err);
> >  }
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index e70633d..98971b7 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -23,12 +23,92 @@
> >  #include "qapi/visitor.h"
> >  #include "qemu/range.h"
> >  #include "sysemu/numa.h"
> > +#include "sysemu/kvm.h"
> > +#include "trace.h"
> >  
> >  typedef struct pc_dimms_capacity {
> >       uint64_t size;
> >       Error    **errp;
> >  } pc_dimms_capacity;
> >  
> > +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > +                         MemoryRegion *mr, uint64_t align, Error **errp)
> > +{
> > +    int slot;
> > +    MachineState *machine = MACHINE(qdev_get_machine());
> > +    PCDIMMDevice *dimm = PC_DIMM(dev);
> > +    Error *local_err = NULL;
> > +    uint64_t existing_dimms_capacity = 0;
> > +    uint64_t addr;
> > +
> > +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    addr = pc_dimm_get_free_addr(hpms->base,
> > +                                 memory_region_size(&hpms->mr),
> > +                                 !addr ? NULL : &addr, align,
> > +                                 memory_region_size(mr), &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (existing_dimms_capacity + memory_region_size(mr) >
> > +        machine->maxram_size - machine->ram_size) {
> > +        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> > +                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> > +                   existing_dimms_capacity,
> > +                   machine->maxram_size - machine->ram_size);
> > +        goto out;
> > +    }
> > +
> > +    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    trace_mhp_pc_dimm_assigned_address(addr);
> > +
> > +    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> > +                                 machine->ram_slots, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    trace_mhp_pc_dimm_assigned_slot(slot);
> > +
> > +    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > +        error_setg(&local_err, "hypervisor has no free memory slots left");
> > +        goto out;
> > +    }
> > +
> > +    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > +    vmstate_register_ram(mr, dev);
> > +
> > +out:
> > +    error_propagate(errp, local_err);
> > +}
> > +
> > +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> > +                           MemoryRegion *mr)
> > +{
> > +    memory_region_del_subregion(&hpms->mr, mr);
> > +    vmstate_unregister_ram(mr, dev);
> > +}
> if you adding this here then you probably should be removing/replacing
> related hunks from pc.c, I don't see it in this patch.

In v2, I was calling this routine from pc_dimm_unplug, but somehow missed
this hunk during rebasing in v3.

I have addressed all you comments for this version and ready with next
version. However will wait till monday before respinning to see if I
am required to do more changes.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v3 2/5] pc, pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate routine

[Igor Mammedov]

On Fri, 26 Jun 2015 17:15:45 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Fri, Jun 26, 2015 at 10:21:54AM +0200, Igor Mammedov wrote:
> > On Fri, 26 Jun 2015 09:36:01 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > pc_dimm_plug() has code that will be needed for memory plug
> > > handlers in other archs too. Extract code from pc_dimm_plug()
> > > into a generic routine pc_dimm_memory_plug() that resides in
> > > pc-dimm.c. Also correspondingly refactor re-usable unplug code
> > > into pc_dimm_memory_unplug().
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > thanks for splitting, much easier to review.
> 
> Yes it is much easier to see the changes and thanks for the review.
> 
> > 
> > 
> > > ---
> > >  hw/i386/pc.c             | 66
> > > ++++----------------------------------- hw/mem/pc-dimm.c
> > > | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > include/hw/mem/pc-dimm.h |  4 +++ 3 files changed, 90
> > > insertions(+), 60 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index e51c3be..cb57612 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -64,7 +64,6 @@
> > >  #include "hw/pci/pci_host.h"
> > >  #include "acpi-build.h"
> > >  #include "hw/mem/pc-dimm.h"
> > > -#include "trace.h"
> > >  #include "qapi/visitor.h"
> > >  #include "qapi-visit.h"
> > >  
> > > @@ -1554,88 +1553,35 @@ void ioapic_init_gsi(GSIState *gsi_state,
> > > const char *parent_name) static void pc_dimm_plug(HotplugHandler
> > > *hotplug_dev, DeviceState *dev, Error **errp)
> > >  {
> > > -    int slot;
> > >      HotplugHandlerClass *hhc;
> > >      Error *local_err = NULL;
> > >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > > -    MachineState *machine = MACHINE(hotplug_dev);
> > >      PCDIMMDevice *dimm = PC_DIMM(dev);
> > >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > >      MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > -    uint64_t existing_dimms_capacity = 0;
> > >      uint64_t align = TARGET_PAGE_SIZE;
> > > -    uint64_t addr;
> > > -
> > > -    addr = object_property_get_int(OBJECT(dimm),
> > > PC_DIMM_ADDR_PROP, &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > >  
> > >      if (memory_region_get_alignment(mr) &&
> > > pcms->enforce_aligned_dimm) { align =
> > > memory_region_get_alignment(mr); }
> > >  
> > > -    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
> > > -
> > > memory_region_size(&pcms->hotplug_memory.mr),
> > > -                                 !addr ? NULL : &addr, align,
> > > -                                 memory_region_size(mr),
> > > &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -
> > > -    existing_dimms_capacity =
> > > pc_existing_dimms_capacity(&local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -
> > > -    if (existing_dimms_capacity + memory_region_size(mr) >
> > > -        machine->maxram_size - machine->ram_size) {
> > > -        error_setg(&local_err, "not enough space, currently 0x%"
> > > PRIx64
> > > -                   " in use of total hot pluggable 0x"
> > > RAM_ADDR_FMT,
> > > -                   existing_dimms_capacity,
> > > -                   machine->maxram_size - machine->ram_size);
> > > -        goto out;
> > > -    }
> > > -
> > > -    object_property_set_int(OBJECT(dev), addr,
> > > PC_DIMM_ADDR_PROP, &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -    trace_mhp_pc_dimm_assigned_address(addr);
> > > -
> > > -    slot = object_property_get_int(OBJECT(dev),
> > > PC_DIMM_SLOT_PROP, &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -
> > > -    slot = pc_dimm_get_free_slot(slot ==
> > > PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> > > -                                 machine->ram_slots, &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -    object_property_set_int(OBJECT(dev), slot,
> > > PC_DIMM_SLOT_PROP, &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -    trace_mhp_pc_dimm_assigned_slot(slot);
> > > -
> > >      if (!pcms->acpi_dev) {
> > >          error_setg(&local_err,
> > >                     "memory hotplug is not enabled: missing acpi
> > > device"); goto out;
> > >      }
> > >  
> > > -    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > > -        error_setg(&local_err, "hypervisor has no free memory
> > > slots left");
> > > +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align,
> > > &local_err);
> > > +    if (local_err) {
> > >          goto out;
> > >      }
> > >  
> > > -    memory_region_add_subregion(&pcms->hotplug_memory.mr,
> > > -                                addr -
> > > pcms->hotplug_memory.base, mr);
> > > -    vmstate_register_ram(mr, dev);
> > > -
> > >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> > >      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> > > +    if (local_err) {
> > > +        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
> > > +    }
> > You are adding something new while patch claims to be just moving
> > code. I'm not sure it's save to call unplug here if hhc->plug()
> > failed since it might have triggered guest visible side effects.
> 
> In sPAPR, the routine equivalent to hhc->plug() could fail and
> I needed pc_dimm_memory_unplug() to recover. But now I have rearranged
> the code there to ensure that it never fails like in x86. However
> I need pc_dimm_memory_unplug() nevertheless in sPAPR.
> 
> > 
> > I'd just drop this hunk and leave original code,
> > on top of that extra patch for hhc->plug(s/local_err/error_abort/)
> > might make a sense since hhc->plug() should never fail and if it's
> > then we should fix error in code instead of trying to recover.
> 
> I have added a small patch for this in my series.
> 
> > 
> > >  out:
> > >      error_propagate(errp, local_err);
> > >  }
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > index e70633d..98971b7 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -23,12 +23,92 @@
> > >  #include "qapi/visitor.h"
> > >  #include "qemu/range.h"
> > >  #include "sysemu/numa.h"
> > > +#include "sysemu/kvm.h"
> > > +#include "trace.h"
> > >  
> > >  typedef struct pc_dimms_capacity {
> > >       uint64_t size;
> > >       Error    **errp;
> > >  } pc_dimms_capacity;
> > >  
> > > +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState
> > > *hpms,
> > > +                         MemoryRegion *mr, uint64_t align, Error
> > > **errp) +{
> > > +    int slot;
> > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > +    PCDIMMDevice *dimm = PC_DIMM(dev);
> > > +    Error *local_err = NULL;
> > > +    uint64_t existing_dimms_capacity = 0;
> > > +    uint64_t addr;
> > > +
> > > +    addr = object_property_get_int(OBJECT(dimm),
> > > PC_DIMM_ADDR_PROP, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    addr = pc_dimm_get_free_addr(hpms->base,
> > > +                                 memory_region_size(&hpms->mr),
> > > +                                 !addr ? NULL : &addr, align,
> > > +                                 memory_region_size(mr),
> > > &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    existing_dimms_capacity =
> > > pc_existing_dimms_capacity(&local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (existing_dimms_capacity + memory_region_size(mr) >
> > > +        machine->maxram_size - machine->ram_size) {
> > > +        error_setg(&local_err, "not enough space, currently 0x%"
> > > PRIx64
> > > +                   " in use of total hot pluggable 0x"
> > > RAM_ADDR_FMT,
> > > +                   existing_dimms_capacity,
> > > +                   machine->maxram_size - machine->ram_size);
> > > +        goto out;
> > > +    }
> > > +
> > > +    object_property_set_int(OBJECT(dev), addr,
> > > PC_DIMM_ADDR_PROP, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +    trace_mhp_pc_dimm_assigned_address(addr);
> > > +
> > > +    slot = object_property_get_int(OBJECT(dev),
> > > PC_DIMM_SLOT_PROP, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    slot = pc_dimm_get_free_slot(slot ==
> > > PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> > > +                                 machine->ram_slots, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +    object_property_set_int(OBJECT(dev), slot,
> > > PC_DIMM_SLOT_PROP, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +    trace_mhp_pc_dimm_assigned_slot(slot);
> > > +
> > > +    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > > +        error_setg(&local_err, "hypervisor has no free memory
> > > slots left");
> > > +        goto out;
> > > +    }
> > > +
> > > +    memory_region_add_subregion(&hpms->mr, addr - hpms->base,
> > > mr);
> > > +    vmstate_register_ram(mr, dev);
> > > +
> > > +out:
> > > +    error_propagate(errp, local_err);
> > > +}
> > > +
> > > +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState
> > > *hpms,
> > > +                           MemoryRegion *mr)
> > > +{
> > > +    memory_region_del_subregion(&hpms->mr, mr);
> > > +    vmstate_unregister_ram(mr, dev);
> > > +}
> > if you adding this here then you probably should be
> > removing/replacing related hunks from pc.c, I don't see it in this
> > patch.
> 
> In v2, I was calling this routine from pc_dimm_unplug, but somehow
> missed this hunk during rebasing in v3.
> 
> I have addressed all you comments for this version and ready with next
> version. However will wait till monday before respinning to see if I
> am required to do more changes.
I've did line by line review of all patches so there shouldn't  be
more comments from my side.


> 
> Regards,
> Bharata.
>