Re: [Qemu-devel] [RFC PATCH v1 13/13] spapr: Memory hotplug support

2015-02-24 Thread Bharata B Rao 
On Tue, Feb 24, 2015 at 05:26:12PM +1100, David Gibson wrote:
 On Thu, Jan 08, 2015 at 11:40:20AM +0530, Bharata B Rao wrote:
  Make use of pc-dimm infrastructure to support memory hotplug
  for PowerPC.
  
  Modelled on i386 memory hotplug.
  
  Signed-off-by: Bharata B Rao bhar...@linux.vnet.ibm.com
  ---
   hw/ppc/spapr.c| 107 
  +-
   hw/ppc/spapr_events.c |   3 ++
   2 files changed, 108 insertions(+), 2 deletions(-)
  
  diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
  index 6964b06..139 100644
  --- a/hw/ppc/spapr.c
  +++ b/hw/ppc/spapr.c
  @@ -58,7 +58,8 @@
   #include hw/nmi.h
   
   #include hw/compat.h
  -
  +#include hw/mem/pc-dimm.h
  +#include qapi/qmp/qerror.h
   #include libfdt.h
   
   /* SLOF memory layout:
  @@ -2165,6 +2166,103 @@ static void spapr_cpu_unplug(HotplugHandler 
  *hotplug_dev, DeviceState *dev,
   return;
   }
   
  +static int spapr_dimms_capacity(Object *obj, void *opaque)
  +{
  +Error *local_err = NULL;
  +uint64_t *size = opaque;
  +
  +if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
  +(*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP, 
  local_err);
  +
  +if (local_err) {
  +qerror_report_err(local_err);
  +error_free(local_err);
  +return 1;
  +}
  +}
  +
  +object_child_foreach(obj, spapr_dimms_capacity, opaque);
  +return 0;
  +}
 
 I don't see any reason you can't use pc_existing_dimms_capacity()
 rather than duplicating it.

Already done that and got the required stuff pushed upstream.
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg03589.html

 
  +static void spapr_memory_plug(HotplugHandler *hotplug_dev,
  + DeviceState *dev, Error **errp)
  +{
  +int slot;
  +Error *local_err = NULL;
  +sPAPRMachineState *ms = SPAPR_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 dimms_capacity = 0;
  +uint64_t align = TARGET_PAGE_SIZE; /* TODO: enforce alignment */
  +uint64_t addr;
  +sPAPRDRConnector *drc;
  +
  +addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, 
  local_err);
  +if (local_err) {
  +goto out;
  +}
  +
  +addr = pc_dimm_get_free_addr(ms-hotplug_memory_base,
  + memory_region_size(ms-hotplug_memory),
  + !addr ? NULL : addr, align,
  + memory_region_size(mr), local_err);
  +if (local_err) {
  +goto out;
  +}
  +
  +if (spapr_dimms_capacity(OBJECT(machine), dimms_capacity)) {
  +error_setg(local_err, failed to get total size of existing 
  DIMMs);
  +goto out;
  +}
  +
  +if (dimms_capacity  machine-maxram_size - machine-ram_size) {
  +error_setg(local_err, not enough space, proposed use of 0x% 
  PRIx64
  +from total of 0x RAM_ADDR_FMT,
  +   dimms_capacity, machine-maxram_size);
  +goto out;
  +}
  +
  +object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, 
  local_err);
  +if (local_err) {
  +goto out;
  +}
  +
  +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;
  +}
  +
  +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(ms-hotplug_memory,
  +addr - ms-hotplug_memory_base, mr);
  +vmstate_register_ram(mr, dev);
  +
  +drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
  +addr/SPAPR_MIN_MEMORY_BLOCK_SIZE);
  +g_assert(drc);
  +spapr_hotplug_req_add_event(drc);
  +
  +out:
  +error_propagate(errp, local_err);
  +}
 
 It looks like this is basically the same as pc_dimm_plug() except for
 a couple of checks and the last section which actually notifies the
 guest.  Could this be made into common code in pc-dimm.c with hooks
 for the platform specific notification?

Yes I could do that.

 
 Maybe PC and sPAPR subclasses of a common dimm object?

Will take a look and see how best this can be done.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 13/13] spapr: Memory hotplug support

2015-02-23 Thread David Gibson 
On Thu, Jan 08, 2015 at 11:40:20AM +0530, Bharata B Rao wrote:
 Make use of pc-dimm infrastructure to support memory hotplug
 for PowerPC.
 
 Modelled on i386 memory hotplug.
 
 Signed-off-by: Bharata B Rao bhar...@linux.vnet.ibm.com
 ---
  hw/ppc/spapr.c| 107 
 +-
  hw/ppc/spapr_events.c |   3 ++
  2 files changed, 108 insertions(+), 2 deletions(-)
 
 diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
 index 6964b06..139 100644
 --- a/hw/ppc/spapr.c
 +++ b/hw/ppc/spapr.c
 @@ -58,7 +58,8 @@
  #include hw/nmi.h
  
  #include hw/compat.h
 -
 +#include hw/mem/pc-dimm.h
 +#include qapi/qmp/qerror.h
  #include libfdt.h
  
  /* SLOF memory layout:
 @@ -2165,6 +2166,103 @@ static void spapr_cpu_unplug(HotplugHandler 
 *hotplug_dev, DeviceState *dev,
  return;
  }
  
 +static int spapr_dimms_capacity(Object *obj, void *opaque)
 +{
 +Error *local_err = NULL;
 +uint64_t *size = opaque;
 +
 +if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
 +(*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP, 
 local_err);
 +
 +if (local_err) {
 +qerror_report_err(local_err);
 +error_free(local_err);
 +return 1;
 +}
 +}
 +
 +object_child_foreach(obj, spapr_dimms_capacity, opaque);
 +return 0;
 +}

I don't see any reason you can't use pc_existing_dimms_capacity()
rather than duplicating it.

 +static void spapr_memory_plug(HotplugHandler *hotplug_dev,
 + DeviceState *dev, Error **errp)
 +{
 +int slot;
 +Error *local_err = NULL;
 +sPAPRMachineState *ms = SPAPR_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 dimms_capacity = 0;
 +uint64_t align = TARGET_PAGE_SIZE; /* TODO: enforce alignment */
 +uint64_t addr;
 +sPAPRDRConnector *drc;
 +
 +addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, 
 local_err);
 +if (local_err) {
 +goto out;
 +}
 +
 +addr = pc_dimm_get_free_addr(ms-hotplug_memory_base,
 + memory_region_size(ms-hotplug_memory),
 + !addr ? NULL : addr, align,
 + memory_region_size(mr), local_err);
 +if (local_err) {
 +goto out;
 +}
 +
 +if (spapr_dimms_capacity(OBJECT(machine), dimms_capacity)) {
 +error_setg(local_err, failed to get total size of existing DIMMs);
 +goto out;
 +}
 +
 +if (dimms_capacity  machine-maxram_size - machine-ram_size) {
 +error_setg(local_err, not enough space, proposed use of 0x% PRIx64
 +from total of 0x RAM_ADDR_FMT,
 +   dimms_capacity, machine-maxram_size);
 +goto out;
 +}
 +
 +object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, 
 local_err);
 +if (local_err) {
 +goto out;
 +}
 +
 +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;
 +}
 +
 +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(ms-hotplug_memory,
 +addr - ms-hotplug_memory_base, mr);
 +vmstate_register_ram(mr, dev);
 +
 +drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
 +addr/SPAPR_MIN_MEMORY_BLOCK_SIZE);
 +g_assert(drc);
 +spapr_hotplug_req_add_event(drc);
 +
 +out:
 +error_propagate(errp, local_err);
 +}

It looks like this is basically the same as pc_dimm_plug() except for
a couple of checks and the last section which actually notifies the
guest.  Could this be made into common code in pc-dimm.c with hooks
for the platform specific notification?

Maybe PC and sPAPR subclasses of a common dimm object?

-- 
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


pgp5PdxJH8Y0j.pgp
Description: PGP signature

[Qemu-devel] [RFC PATCH v1 13/13] spapr: Memory hotplug support

2015-01-07 Thread Bharata B Rao 
Make use of pc-dimm infrastructure to support memory hotplug
for PowerPC.

Modelled on i386 memory hotplug.

Signed-off-by: Bharata B Rao bhar...@linux.vnet.ibm.com
---
 hw/ppc/spapr.c| 107 +-
 hw/ppc/spapr_events.c |   3 ++
 2 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6964b06..139 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -58,7 +58,8 @@
 #include hw/nmi.h
 
 #include hw/compat.h
-
+#include hw/mem/pc-dimm.h
+#include qapi/qmp/qerror.h
 #include libfdt.h
 
 /* SLOF memory layout:
@@ -2165,6 +2166,103 @@ static void spapr_cpu_unplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 return;
 }
 
+static int spapr_dimms_capacity(Object *obj, void *opaque)
+{
+Error *local_err = NULL;
+uint64_t *size = opaque;
+
+if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
+(*size) += object_property_get_int(obj, PC_DIMM_SIZE_PROP, local_err);
+
+if (local_err) {
+qerror_report_err(local_err);
+error_free(local_err);
+return 1;
+}
+}
+
+object_child_foreach(obj, spapr_dimms_capacity, opaque);
+return 0;
+}
+
+static void spapr_memory_plug(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+int slot;
+Error *local_err = NULL;
+sPAPRMachineState *ms = SPAPR_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 dimms_capacity = 0;
+uint64_t align = TARGET_PAGE_SIZE; /* TODO: enforce alignment */
+uint64_t addr;
+sPAPRDRConnector *drc;
+
+addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, 
local_err);
+if (local_err) {
+goto out;
+}
+
+addr = pc_dimm_get_free_addr(ms-hotplug_memory_base,
+ memory_region_size(ms-hotplug_memory),
+ !addr ? NULL : addr, align,
+ memory_region_size(mr), local_err);
+if (local_err) {
+goto out;
+}
+
+if (spapr_dimms_capacity(OBJECT(machine), dimms_capacity)) {
+error_setg(local_err, failed to get total size of existing DIMMs);
+goto out;
+}
+
+if (dimms_capacity  machine-maxram_size - machine-ram_size) {
+error_setg(local_err, not enough space, proposed use of 0x% PRIx64
+from total of 0x RAM_ADDR_FMT,
+   dimms_capacity, machine-maxram_size);
+goto out;
+}
+
+object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, local_err);
+if (local_err) {
+goto out;
+}
+
+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;
+}
+
+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(ms-hotplug_memory,
+addr - ms-hotplug_memory_base, mr);
+vmstate_register_ram(mr, dev);
+
+drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
+addr/SPAPR_MIN_MEMORY_BLOCK_SIZE);
+g_assert(drc);
+spapr_hotplug_req_add_event(drc);
+
+out:
+error_propagate(errp, local_err);
+}
+
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
   DeviceState *dev, Error **errp)
 {
@@ -2172,6 +2270,8 @@ static void spapr_machine_device_plug(HotplugHandler 
*hotplug_dev,
 if (dev-hotplugged) {
 spapr_cpu_plug(hotplug_dev, dev, errp);
 }
+} else if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+spapr_memory_plug(hotplug_dev, dev, errp);
 }
 }
 
@@ -2182,13 +2282,16 @@ static void spapr_machine_device_unplug(HotplugHandler 
*hotplug_dev,
 if (dev-hotplugged) {
 spapr_cpu_unplug(hotplug_dev, dev, errp);
 }
+} else if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+error_setg(errp, Memory hot unplug is not yet supported\n);
 }
 }
 
 static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
  DeviceState *dev)
 {
-if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
+object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
 return HOTPLUG_HANDLER(machine);
 }
 return NULL;
diff