[Qemu-devel] [RFC PATCH v0 0/3] sPAPR: Memory hot removal support

[Qemu-devel] [RFC PATCH v0 0/3] sPAPR: Memory hot removal support

[Bharata B Rao]

This patchset enables memory hot unplug for PowerPC sPAPR guests.
This applies against spapr-next branch of David Gibson's tree that
currently contains the memory hotplug code for sPAPR.

Currently with drmgr, it is not possible to attempt just the removal
of those LMBs that form the DIMM device when the DIMM device is removed.
drmgr just walks through all the available 'removable' LMBs and tries
to off-line the specified number of LMBs. Because of this, I have
introduced some additional checks in QEMU to fail the removal request of an
LMB that doesn't belong to the DIMM device which is being unplugged.
This causes some churn in the guest when the LMB that didn't belong to
the DIMM device was offlined and later brought online again by drmgr
when QEMU fails the release of the corresponding DRC object.

Bharata B Rao (3):
  pc-dimm: Add a field to PCDIMMDevice to mark device deletion state
  spapr-rtas: Enable rtas_set_indicator() to return correct error
  spapr: Memory hot-unplug support

 hw/ppc/spapr.c           | 114 ++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ppc/spapr_drc.c       |  21 +++++++++
 hw/ppc/spapr_rtas.c      |   9 ++--
 include/hw/mem/pc-dimm.h |   1 +
 include/hw/ppc/spapr.h   |   2 +
 5 files changed, 142 insertions(+), 5 deletions(-)

-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 1/3] pc-dimm: Add a field to PCDIMMDevice to mark device deletion state

[Bharata B Rao]

Add a field to PCDIMMDevice to note that the device has been marked
for removal. This will be used by PowerPC memory hotplug code to
honour the LMB removal requests of only those LMBs that belong to
PCDIMMDevice that has been marked for removal. This will be set from
-unplug() handler.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 include/hw/mem/pc-dimm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index d83bf30..4ca9316 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -56,6 +56,7 @@ typedef struct PCDIMMDevice {
     uint32_t node;
     int32_t slot;
     HostMemoryBackend *hostmem;
+    bool delete_pending;
 } PCDIMMDevice;
 
 /**
-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 2/3] spapr-rtas: Enable rtas_set_indicator() to return correct error

[Bharata B Rao]

drck->set_isolation_state() can return error. For such a case ensure
correct error is returned by rtas_set_indicator() instead of always
returning success.

TODO: rtas_st(, , uint32 val) => the return value uint32, but
drck->set_[allocation/indicator/isolation]_state() is returning int.
Should we change this return value to uint32_t to match with rtas_st()
argument ?

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr_rtas.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index e99e25f..96729b4 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -374,6 +374,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     uint32_t sensor_state;
     sPAPRDRConnector *drc;
     sPAPRDRConnectorClass *drck;
+    int ret;
 
     if (nargs != 3 || nret != 1) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -413,19 +414,19 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                 spapr_ccs_remove(spapr, ccs);
             }
         }
-        drck->set_isolation_state(drc, sensor_state);
+        ret = drck->set_isolation_state(drc, sensor_state);
         break;
     case RTAS_SENSOR_TYPE_DR:
-        drck->set_indicator_state(drc, sensor_state);
+        ret = drck->set_indicator_state(drc, sensor_state);
         break;
     case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
-        drck->set_allocation_state(drc, sensor_state);
+        ret = drck->set_allocation_state(drc, sensor_state);
         break;
     default:
         goto out_unimplemented;
     }
 
-    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    rtas_st(rets, 0, ret);
     return;
 
 out_unimplemented:
-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 3/3] spapr: Memory hot-unplug support

[Bharata B Rao]

Add support to hot remove pc-dimm memory devices.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 114 ++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ppc/spapr_drc.c     |  21 +++++++++
 include/hw/ppc/spapr.h |   2 +
 3 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 06d000d..441012d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2110,6 +2110,109 @@ out:
     error_propagate(errp, local_err);
 }
 
+typedef struct sPAPRDIMMState {
+    uint32_t nr_lmbs;
+} sPAPRDIMMState;
+
+/*
+ * Called from spapr_drc.c: set_isolation_state().
+ *
+ * If the drc is being marked as ISOLATED, ensure that the corresponding
+ * LMB is part of the DIMM device which is being deleted.
+ */
+int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc,
+                                sPAPRDRIsolationState state)
+{
+    DeviceState *dev = drc->dev;
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+
+    if (state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
+        return 0;
+    }
+
+    if (!dimm->delete_pending) {
+        return -1;
+    }
+
+    return 0;
+}
+
+static void spapr_lmb_release(DeviceState *dev, void *opaque)
+{
+    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
+    HotplugHandler *hotplug_ctrl = NULL;
+    Error *local_err = NULL;
+
+    if (--ds->nr_lmbs) {
+        return;
+    }
+
+    g_free(ds);
+
+    /*
+     * Now that all the LMBs have been removed by the guest, call the
+     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     */
+    hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
+}
+
+static void spapr_del_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
+                           Error **errp)
+{
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
+    Error *local_err = NULL;
+    int i;
+    sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState));
+
+    ds->nr_lmbs = nr_lmbs;
+    for (i = 0; i < nr_lmbs; i++) {
+        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
+                addr/SPAPR_MEMORY_BLOCK_SIZE);
+        g_assert(drc);
+
+        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+        drck->detach(drc, dev, spapr_lmb_release, ds, &local_err);
+        addr += SPAPR_MEMORY_BLOCK_SIZE;
+    }
+    spapr_hotplug_req_remove_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                Error **errp)
+{
+    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
+
+    pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
+    object_unparent(OBJECT(dev));
+}
+
+static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+    Error *local_err = NULL;
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
+    uint64_t size = memory_region_size(mr);
+    uint64_t addr;
+
+    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    dimm->delete_pending = true;
+    spapr_del_lmbs(dev, addr, size, &local_err);
+out:
+    error_propagate(errp, local_err);
+}
+
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
@@ -2157,7 +2260,15 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        error_setg(errp, "Memory hot unplug not supported by sPAPR");
+        spapr_memory_unplug(hotplug_dev, dev, errp);
+    }
+}
+
+static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
+                                                DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug_request(hotplug_dev, dev, errp);
     }
 }
 
@@ -2191,6 +2302,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_hotplug_handler = spapr_get_hotpug_handler;
     hc->plug = spapr_machine_device_plug;
     hc->unplug = spapr_machine_device_unplug;
+    hc->unplug_request = spapr_machine_device_unplug_request;
 
     smc->dr_lmb_enabled = false;
     fwc->get_dev_path = spapr_get_fw_dev_path;
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 8cbcf4d..b9d7c71 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -11,6 +11,7 @@
  */
 
 #include "hw/ppc/spapr_drc.h"
+#include "hw/ppc/spapr.h"
 #include "qom/object.h"
 #include "hw/qdev.h"
 #include "qapi/visitor.h"
@@ -63,9 +64,29 @@ static int set_isolation_state(sPAPRDRConnector *drc,
                                sPAPRDRIsolationState state)
 {
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    int ret;
 
     DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state);
 
+    /*
+     * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
+     * belong to a DIMM device that is marked for removal.
+     *
+     * Currently the guest userspace tool drmgr that drives the memory
+     * hotplug/unplug will just try to remove a set of 'removable' LMBs
+     * in response to a hot unplug request that is based on drc-count.
+     * If the LMB being removed doesn't belong to a DIMM device that is
+     * actually being unplugged, fail the isolation request here.
+     *
+     * TODO: Calling out from spapr_drc.c like this doesn't look good.
+     */
+    if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
+        ret = spapr_lmb_in_removable_dimm(drc, state);
+        if (ret) {
+            return RTAS_OUT_HW_ERROR;
+        }
+    }
+
     drc->isolation_state = state;
 
     if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index d87c6d4..2039ca1 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -594,6 +594,8 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
                                        uint32_t count);
 void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
                                           uint32_t count);
+int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc,
+                                sPAPRDRIsolationState state);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v0 2/3] spapr-rtas: Enable rtas_set_indicator() to return correct error

[Michael Roth]

Quoting Bharata B Rao (2015-08-19 01:56:10)
> drck->set_isolation_state() can return error. For such a case ensure
> correct error is returned by rtas_set_indicator() instead of always
> returning success.
> 
> TODO: rtas_st(, , uint32 val) => the return value uint32, but
> drck->set_[allocation/indicator/isolation]_state() is returning int.
> Should we change this return value to uint32_t to match with rtas_st()
> argument ?

I wouldn't bother too much aligning the types unless we go to the extent
of documenting these interfaces as returning rtas error codes. That's
not really the case currently, and there's a lot of rtas errors that
don't really need to be determined by DRC code so I think it's more
trouble than it's worth.

For now I think it's better to just check for ret != 0 and set return
values explicitly in rtas code based on what the drc errors entail.

> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_rtas.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index e99e25f..96729b4 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -374,6 +374,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      uint32_t sensor_state;
>      sPAPRDRConnector *drc;
>      sPAPRDRConnectorClass *drck;
> +    int ret;
> 
>      if (nargs != 3 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -413,19 +414,19 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                  spapr_ccs_remove(spapr, ccs);
>              }
>          }
> -        drck->set_isolation_state(drc, sensor_state);
> +        ret = drck->set_isolation_state(drc, sensor_state);
>          break;
>      case RTAS_SENSOR_TYPE_DR:
> -        drck->set_indicator_state(drc, sensor_state);
> +        ret = drck->set_indicator_state(drc, sensor_state);
>          break;
>      case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> -        drck->set_allocation_state(drc, sensor_state);
> +        ret = drck->set_allocation_state(drc, sensor_state);
>          break;
>      default:
>          goto out_unimplemented;
>      }
> 
> -    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    rtas_st(rets, 0, ret);
>      return;
> 
>  out_unimplemented:
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [RFC PATCH v0 1/3] pc-dimm: Add a field to PCDIMMDevice to mark device deletion state

[Michael Roth]

Quoting Bharata B Rao (2015-08-19 01:56:09)
> Add a field to PCDIMMDevice to note that the device has been marked
> for removal. This will be used by PowerPC memory hotplug code to
> honour the LMB removal requests of only those LMBs that belong to
> PCDIMMDevice that has been marked for removal. This will be set from
> -unplug() handler.

Why not track the delete pending state in the DRC? We have an
awaiting_release flag there for similar purpose.

> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  include/hw/mem/pc-dimm.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index d83bf30..4ca9316 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -56,6 +56,7 @@ typedef struct PCDIMMDevice {
>      uint32_t node;
>      int32_t slot;
>      HostMemoryBackend *hostmem;
> +    bool delete_pending;
>  } PCDIMMDevice;
> 
>  /**
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [RFC PATCH v0 3/3] spapr: Memory hot-unplug support

[Michael Roth]

Quoting Bharata B Rao (2015-08-19 01:56:11)
> Add support to hot remove pc-dimm memory devices.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c         | 114 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/ppc/spapr_drc.c     |  21 +++++++++
>  include/hw/ppc/spapr.h |   2 +
>  3 files changed, 136 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 06d000d..441012d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2110,6 +2110,109 @@ out:
>      error_propagate(errp, local_err);
>  }
> 
> +typedef struct sPAPRDIMMState {
> +    uint32_t nr_lmbs;
> +} sPAPRDIMMState;
> +
> +/*
> + * Called from spapr_drc.c: set_isolation_state().
> + *
> + * If the drc is being marked as ISOLATED, ensure that the corresponding
> + * LMB is part of the DIMM device which is being deleted.
> + */
> +int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc,
> +                                sPAPRDRIsolationState state)
> +{
> +    DeviceState *dev = drc->dev;
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +
> +    if (state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> +        return 0;
> +    }
> +
> +    if (!dimm->delete_pending) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void spapr_lmb_release(DeviceState *dev, void *opaque)
> +{
> +    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
> +    HotplugHandler *hotplug_ctrl = NULL;
> +    Error *local_err = NULL;
> +
> +    if (--ds->nr_lmbs) {
> +        return;
> +    }
> +
> +    g_free(ds);
> +
> +    /*
> +     * Now that all the LMBs have been removed by the guest, call the
> +     * pc-dimm unplug handler to cleanup up the pc-dimm device.
> +     */
> +    hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +    hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> +}
> +
> +static void spapr_del_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
> +                           Error **errp)
> +{
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> +    Error *local_err = NULL;
> +    int i;
> +    sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState));
> +
> +    ds->nr_lmbs = nr_lmbs;
> +    for (i = 0; i < nr_lmbs; i++) {
> +        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> +                addr/SPAPR_MEMORY_BLOCK_SIZE);
> +        g_assert(drc);
> +
> +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +        drck->detach(drc, dev, spapr_lmb_release, ds, &local_err);
> +        addr += SPAPR_MEMORY_BLOCK_SIZE;
> +    }
> +    spapr_hotplug_req_remove_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
> +}
> +
> +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                Error **errp)
> +{
> +    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +
> +    pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
> +    object_unparent(OBJECT(dev));
> +}

In the current code the unplug() and request_unplug() are mutually
exclusive. Are the plans on making the unplug() do something in the
prescence of request_unplug()? If so, I'd imagine it would've be a
forced removal, except maybe as a fallback if the request is determined
to fail somehow?

> +
> +static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> +                                        DeviceState *dev, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    uint64_t size = memory_region_size(mr);
> +    uint64_t addr;
> +
> +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    dimm->delete_pending = true;
> +    spapr_del_lmbs(dev, addr, size, &local_err);
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> @@ -2157,7 +2260,15 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        error_setg(errp, "Memory hot unplug not supported by sPAPR");
> +        spapr_memory_unplug(hotplug_dev, dev, errp);
> +    }
> +}
> +
> +static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> +                                                DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        spapr_memory_unplug_request(hotplug_dev, dev, errp);
>      }
>  }
> 
> @@ -2191,6 +2302,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->get_hotplug_handler = spapr_get_hotpug_handler;
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
> +    hc->unplug_request = spapr_machine_device_unplug_request;
> 
>      smc->dr_lmb_enabled = false;
>      fwc->get_dev_path = spapr_get_fw_dev_path;
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8cbcf4d..b9d7c71 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -11,6 +11,7 @@
>   */
> 
>  #include "hw/ppc/spapr_drc.h"
> +#include "hw/ppc/spapr.h"
>  #include "qom/object.h"
>  #include "hw/qdev.h"
>  #include "qapi/visitor.h"
> @@ -63,9 +64,29 @@ static int set_isolation_state(sPAPRDRConnector *drc,
>                                 sPAPRDRIsolationState state)
>  {
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    int ret;
> 
>      DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state);
> 
> +    /*
> +     * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> +     * belong to a DIMM device that is marked for removal.
> +     *
> +     * Currently the guest userspace tool drmgr that drives the memory
> +     * hotplug/unplug will just try to remove a set of 'removable' LMBs
> +     * in response to a hot unplug request that is based on drc-count.
> +     * If the LMB being removed doesn't belong to a DIMM device that is
> +     * actually being unplugged, fail the isolation request here.
> +     *
> +     * TODO: Calling out from spapr_drc.c like this doesn't look good.
> +     */
> +    if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> +        ret = spapr_lmb_in_removable_dimm(drc, state);
> +        if (ret) {
> +            return RTAS_OUT_HW_ERROR;
> +        }
> +    }
> +
>      drc->isolation_state = state;
> 
>      if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d87c6d4..2039ca1 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -594,6 +594,8 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
>                                         uint32_t count);
>  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>                                            uint32_t count);
> +int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc,
> +                                sPAPRDRIsolationState state);
> 
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [RFC PATCH v0 1/3] pc-dimm: Add a field to PCDIMMDevice to mark device deletion state

[Bharata B Rao]

On Mon, Aug 24, 2015 at 09:30:35PM -0500, Michael Roth wrote:
> Quoting Bharata B Rao (2015-08-19 01:56:09)
> > Add a field to PCDIMMDevice to note that the device has been marked
> > for removal. This will be used by PowerPC memory hotplug code to
> > honour the LMB removal requests of only those LMBs that belong to
> > PCDIMMDevice that has been marked for removal. This will be set from
> > -unplug() handler.
> 
> Why not track the delete pending state in the DRC? We have an
> awaiting_release flag there for similar purpose.

Ah yes, that should be possible. Will drop this patch in the
next iteration.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v0 3/3] spapr: Memory hot-unplug support

[Bharata B Rao]

On Mon, Aug 24, 2015 at 09:39:31PM -0500, Michael Roth wrote:
> Quoting Bharata B Rao (2015-08-19 01:56:11)
> > Add support to hot remove pc-dimm memory devices.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c         | 114 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  hw/ppc/spapr_drc.c     |  21 +++++++++
> >  include/hw/ppc/spapr.h |   2 +
> >  3 files changed, 136 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 06d000d..441012d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2110,6 +2110,109 @@ out:
> >      error_propagate(errp, local_err);
> >  }
> > 
> > +typedef struct sPAPRDIMMState {
> > +    uint32_t nr_lmbs;
> > +} sPAPRDIMMState;
> > +
> > +/*
> > + * Called from spapr_drc.c: set_isolation_state().
> > + *
> > + * If the drc is being marked as ISOLATED, ensure that the corresponding
> > + * LMB is part of the DIMM device which is being deleted.
> > + */
> > +int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc,
> > +                                sPAPRDRIsolationState state)
> > +{
> > +    DeviceState *dev = drc->dev;
> > +    PCDIMMDevice *dimm = PC_DIMM(dev);
> > +
> > +    if (state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > +        return 0;
> > +    }
> > +
> > +    if (!dimm->delete_pending) {
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static void spapr_lmb_release(DeviceState *dev, void *opaque)
> > +{
> > +    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
> > +    HotplugHandler *hotplug_ctrl = NULL;
> > +    Error *local_err = NULL;
> > +
> > +    if (--ds->nr_lmbs) {
> > +        return;
> > +    }
> > +
> > +    g_free(ds);
> > +
> > +    /*
> > +     * Now that all the LMBs have been removed by the guest, call the
> > +     * pc-dimm unplug handler to cleanup up the pc-dimm device.
> > +     */
> > +    hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > +    hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> > +}
> > +
> > +static void spapr_del_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
> > +                           Error **errp)
> > +{
> > +    sPAPRDRConnector *drc;
> > +    sPAPRDRConnectorClass *drck;
> > +    uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> > +    Error *local_err = NULL;
> > +    int i;
> > +    sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState));
> > +
> > +    ds->nr_lmbs = nr_lmbs;
> > +    for (i = 0; i < nr_lmbs; i++) {
> > +        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> > +                addr/SPAPR_MEMORY_BLOCK_SIZE);
> > +        g_assert(drc);
> > +
> > +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +        drck->detach(drc, dev, spapr_lmb_release, ds, &local_err);
> > +        addr += SPAPR_MEMORY_BLOCK_SIZE;
> > +    }
> > +    spapr_hotplug_req_remove_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
> > +}
> > +
> > +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > +                                Error **errp)
> > +{
> > +    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> > +    PCDIMMDevice *dimm = PC_DIMM(dev);
> > +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > +    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > +
> > +    pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
> > +    object_unparent(OBJECT(dev));
> > +}
> 
> In the current code the unplug() and request_unplug() are mutually
> exclusive. Are the plans on making the unplug() do something in the
> prescence of request_unplug()? If so, I'd imagine it would've be a
> forced removal, except maybe as a fallback if the request is determined
> to fail somehow?

Like x86 memory hotremoval, our model too fits into async type of removal
where we first send removal notification to guest in ->unplug_request() and
when the guest indeed removes the memory, we cleanup the pc-dimm device
in ->unplug().

Since we implement both ->unplug() and ->unplug_request(), and given that
the removal works like above, I don't see why we would ever end up doing a
forced removal from ->unplug().

> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 8cbcf4d..b9d7c71 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -11,6 +11,7 @@
> >   */
> > 
> >  #include "hw/ppc/spapr_drc.h"
> > +#include "hw/ppc/spapr.h"
> >  #include "qom/object.h"
> >  #include "hw/qdev.h"
> >  #include "qapi/visitor.h"
> > @@ -63,9 +64,29 @@ static int set_isolation_state(sPAPRDRConnector *drc,
> >                                 sPAPRDRIsolationState state)
> >  {
> >      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +    int ret;
> > 
> >      DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state);
> > 
> > +    /*
> > +     * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> > +     * belong to a DIMM device that is marked for removal.
> > +     *
> > +     * Currently the guest userspace tool drmgr that drives the memory
> > +     * hotplug/unplug will just try to remove a set of 'removable' LMBs
> > +     * in response to a hot unplug request that is based on drc-count.
> > +     * If the LMB being removed doesn't belong to a DIMM device that is
> > +     * actually being unplugged, fail the isolation request here.
> > +     *
> > +     * TODO: Calling out from spapr_drc.c like this doesn't look good.
> > +     */
> > +    if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > +        ret = spapr_lmb_in_removable_dimm(drc, state);
> > +        if (ret) {
> > +            return RTAS_OUT_HW_ERROR;
> > +        }
> > +    }

I am not sure if this call out is the right way to do this. Do you have
suggestions here ?

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v0 2/3] spapr-rtas: Enable rtas_set_indicator() to return correct error

[David Gibson]

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

On Wed, Aug 19, 2015 at 12:26:10PM +0530, Bharata B Rao wrote:
> drck->set_isolation_state() can return error. For such a case ensure
> correct error is returned by rtas_set_indicator() instead of always
> returning success.
> 
> TODO: rtas_st(, , uint32 val) => the return value uint32, but
> drck->set_[allocation/indicator/isolation]_state() is returning int.
> Should we change this return value to uint32_t to match with rtas_st()
> argument ?
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

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

> ---
>  hw/ppc/spapr_rtas.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index e99e25f..96729b4 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -374,6 +374,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      uint32_t sensor_state;
>      sPAPRDRConnector *drc;
>      sPAPRDRConnectorClass *drck;
> +    int ret;
>  
>      if (nargs != 3 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -413,19 +414,19 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                  spapr_ccs_remove(spapr, ccs);
>              }
>          }
> -        drck->set_isolation_state(drc, sensor_state);
> +        ret = drck->set_isolation_state(drc, sensor_state);
>          break;
>      case RTAS_SENSOR_TYPE_DR:
> -        drck->set_indicator_state(drc, sensor_state);
> +        ret = drck->set_indicator_state(drc, sensor_state);
>          break;
>      case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> -        drck->set_allocation_state(drc, sensor_state);
> +        ret = drck->set_allocation_state(drc, sensor_state);
>          break;
>      default:
>          goto out_unimplemented;
>      }
>  
> -    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    rtas_st(rets, 0, ret);
>      return;
>  
>  out_unimplemented:

-- 
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] [RFC PATCH v0 3/3] spapr: Memory hot-unplug support

[David Gibson]

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

On Wed, Aug 19, 2015 at 12:26:11PM +0530, Bharata B Rao wrote:
> Add support to hot remove pc-dimm memory devices.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c         | 114 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/ppc/spapr_drc.c     |  21 +++++++++
>  include/hw/ppc/spapr.h |   2 +
>  3 files changed, 136 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 06d000d..441012d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2110,6 +2110,109 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +typedef struct sPAPRDIMMState {
> +    uint32_t nr_lmbs;
> +} sPAPRDIMMState;
> +
> +/*
> + * Called from spapr_drc.c: set_isolation_state().
> + *
> + * If the drc is being marked as ISOLATED, ensure that the corresponding
> + * LMB is part of the DIMM device which is being deleted.
> + */
> +int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc,
> +                                sPAPRDRIsolationState state)
> +{
> +    DeviceState *dev = drc->dev;
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +
> +    if (state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> +        return 0;
> +    }

This check doesn't really seem to fit with the function's name.  Would
it be clearer if you moved the state test to the caller?

> +
> +    if (!dimm->delete_pending) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void spapr_lmb_release(DeviceState *dev, void *opaque)
> +{
> +    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
> +    HotplugHandler *hotplug_ctrl = NULL;
> +    Error *local_err = NULL;
> +
> +    if (--ds->nr_lmbs) {
> +        return;
> +    }
> +
> +    g_free(ds);
> +
> +    /*
> +     * Now that all the LMBs have been removed by the guest, call the
> +     * pc-dimm unplug handler to cleanup up the pc-dimm device.
> +     */
> +    hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +    hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> +}
> +
> +static void spapr_del_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
> +                           Error **errp)
> +{
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> +    Error *local_err = NULL;
> +    int i;
> +    sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState));
> +
> +    ds->nr_lmbs = nr_lmbs;
> +    for (i = 0; i < nr_lmbs; i++) {
> +        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> +                addr/SPAPR_MEMORY_BLOCK_SIZE);
> +        g_assert(drc);
> +
> +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +        drck->detach(drc, dev, spapr_lmb_release, ds, &local_err);
> +        addr += SPAPR_MEMORY_BLOCK_SIZE;
> +    }
> +    spapr_hotplug_req_remove_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
> +}
> +
> +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                Error **errp)
> +{
> +    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +
> +    pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
> +    object_unparent(OBJECT(dev));
> +}
> +
> +static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> +                                        DeviceState *dev, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    uint64_t size = memory_region_size(mr);
> +    uint64_t addr;
> +
> +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    dimm->delete_pending = true;
> +    spapr_del_lmbs(dev, addr, size, &local_err);
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> @@ -2157,7 +2260,15 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        error_setg(errp, "Memory hot unplug not supported by sPAPR");
> +        spapr_memory_unplug(hotplug_dev, dev, errp);
> +    }
> +}
> +
> +static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> +                                                DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        spapr_memory_unplug_request(hotplug_dev, dev, errp);
>      }
>  }
>  
> @@ -2191,6 +2302,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->get_hotplug_handler = spapr_get_hotpug_handler;
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
> +    hc->unplug_request = spapr_machine_device_unplug_request;
>  
>      smc->dr_lmb_enabled = false;
>      fwc->get_dev_path = spapr_get_fw_dev_path;
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8cbcf4d..b9d7c71 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include "hw/ppc/spapr_drc.h"
> +#include "hw/ppc/spapr.h"
>  #include "qom/object.h"
>  #include "hw/qdev.h"
>  #include "qapi/visitor.h"
> @@ -63,9 +64,29 @@ static int set_isolation_state(sPAPRDRConnector *drc,
>                                 sPAPRDRIsolationState state)
>  {
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    int ret;
>  
>      DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state);
>  
> +    /*
> +     * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> +     * belong to a DIMM device that is marked for removal.
> +     *
> +     * Currently the guest userspace tool drmgr that drives the memory
> +     * hotplug/unplug will just try to remove a set of 'removable' LMBs
> +     * in response to a hot unplug request that is based on drc-count.
> +     * If the LMB being removed doesn't belong to a DIMM device that is
> +     * actually being unplugged, fail the isolation request here.
> +     *
> +     * TODO: Calling out from spapr_drc.c like this doesn't look good.
> +     */
> +    if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> +        ret = spapr_lmb_in_removable_dimm(drc, state);
> +        if (ret) {
> +            return RTAS_OUT_HW_ERROR;
> +        }
> +    }
> +
>      drc->isolation_state = state;
>  
>      if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d87c6d4..2039ca1 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -594,6 +594,8 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
>                                         uint32_t count);
>  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>                                            uint32_t count);
> +int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc,
> +                                sPAPRDRIsolationState state);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {

-- 
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] [RFC PATCH v0 3/3] spapr: Memory hot-unplug support

[Michael Roth]

Quoting Bharata B Rao (2015-08-26 04:57:51)
> On Mon, Aug 24, 2015 at 09:39:31PM -0500, Michael Roth wrote:
> > Quoting Bharata B Rao (2015-08-19 01:56:11)
> > > Add support to hot remove pc-dimm memory devices.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c         | 114 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  hw/ppc/spapr_drc.c     |  21 +++++++++
> > >  include/hw/ppc/spapr.h |   2 +
> > >  3 files changed, 136 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 06d000d..441012d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2110,6 +2110,109 @@ out:
> > >      error_propagate(errp, local_err);
> > >  }
> > > 
> > > +typedef struct sPAPRDIMMState {
> > > +    uint32_t nr_lmbs;
> > > +} sPAPRDIMMState;
> > > +
> > > +/*
> > > + * Called from spapr_drc.c: set_isolation_state().
> > > + *
> > > + * If the drc is being marked as ISOLATED, ensure that the corresponding
> > > + * LMB is part of the DIMM device which is being deleted.
> > > + */
> > > +int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc,
> > > +                                sPAPRDRIsolationState state)
> > > +{
> > > +    DeviceState *dev = drc->dev;
> > > +    PCDIMMDevice *dimm = PC_DIMM(dev);
> > > +
> > > +    if (state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (!dimm->delete_pending) {
> > > +        return -1;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static void spapr_lmb_release(DeviceState *dev, void *opaque)
> > > +{
> > > +    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
> > > +    HotplugHandler *hotplug_ctrl = NULL;
> > > +    Error *local_err = NULL;
> > > +
> > > +    if (--ds->nr_lmbs) {
> > > +        return;
> > > +    }
> > > +
> > > +    g_free(ds);
> > > +
> > > +    /*
> > > +     * Now that all the LMBs have been removed by the guest, call the
> > > +     * pc-dimm unplug handler to cleanup up the pc-dimm device.
> > > +     */
> > > +    hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > > +    hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> > > +}
> > > +
> > > +static void spapr_del_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
> > > +                           Error **errp)
> > > +{
> > > +    sPAPRDRConnector *drc;
> > > +    sPAPRDRConnectorClass *drck;
> > > +    uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> > > +    Error *local_err = NULL;
> > > +    int i;
> > > +    sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState));
> > > +
> > > +    ds->nr_lmbs = nr_lmbs;
> > > +    for (i = 0; i < nr_lmbs; i++) {
> > > +        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> > > +                addr/SPAPR_MEMORY_BLOCK_SIZE);
> > > +        g_assert(drc);
> > > +
> > > +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +        drck->detach(drc, dev, spapr_lmb_release, ds, &local_err);
> > > +        addr += SPAPR_MEMORY_BLOCK_SIZE;
> > > +    }
> > > +    spapr_hotplug_req_remove_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
> > > +}
> > > +
> > > +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > +                                Error **errp)
> > > +{
> > > +    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> > > +    PCDIMMDevice *dimm = PC_DIMM(dev);
> > > +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > > +    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > +
> > > +    pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
> > > +    object_unparent(OBJECT(dev));
> > > +}
> > 
> > In the current code the unplug() and request_unplug() are mutually
> > exclusive. Are the plans on making the unplug() do something in the
> > prescence of request_unplug()? If so, I'd imagine it would've be a
> > forced removal, except maybe as a fallback if the request is determined
> > to fail somehow?
> 
> Like x86 memory hotremoval, our model too fits into async type of removal
> where we first send removal notification to guest in ->unplug_request() and
> when the guest indeed removes the memory, we cleanup the pc-dimm device
> in ->unplug().
> 
> Since we implement both ->unplug() and ->unplug_request(), and given that
> the removal works like above, I don't see why we would ever end up doing a
> forced removal from ->unplug().

Ah, sorry, misunderstanding on my part: as far as *qdev* lifecycle is
concerned, unplug() never gets called directly if unplug_request() is
defined, so it seemed the 2 were mutually exclusive. But in the case
of memory x86 unplug, unplug_request()'s implementation is such that
the unplug() handler gets called explicitly via guest acknowledgement's
callback, which is the same as what you've modelled here.

It's a little wierd that the unplug() callback is needlessly exposed
outside of the async unplug_request() implementation, but it does
allow for future code where maybe a --force option could be introduced
for certain situations where a guest isn't cooperative.

> 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index 8cbcf4d..b9d7c71 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -11,6 +11,7 @@
> > >   */
> > > 
> > >  #include "hw/ppc/spapr_drc.h"
> > > +#include "hw/ppc/spapr.h"
> > >  #include "qom/object.h"
> > >  #include "hw/qdev.h"
> > >  #include "qapi/visitor.h"
> > > @@ -63,9 +64,29 @@ static int set_isolation_state(sPAPRDRConnector *drc,
> > >                                 sPAPRDRIsolationState state)
> > >  {
> > >      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +    int ret;
> > > 
> > >      DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state);
> > > 
> > > +    /*
> > > +     * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> > > +     * belong to a DIMM device that is marked for removal.
> > > +     *
> > > +     * Currently the guest userspace tool drmgr that drives the memory
> > > +     * hotplug/unplug will just try to remove a set of 'removable' LMBs
> > > +     * in response to a hot unplug request that is based on drc-count.
> > > +     * If the LMB being removed doesn't belong to a DIMM device that is
> > > +     * actually being unplugged, fail the isolation request here.
> > > +     *
> > > +     * TODO: Calling out from spapr_drc.c like this doesn't look good.
> > > +     */
> > > +    if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > > +        ret = spapr_lmb_in_removable_dimm(drc, state);
> > > +        if (ret) {
> > > +            return RTAS_OUT_HW_ERROR;
> > > +        }
> > > +    }
> 
> I am not sure if this call out is the right way to do this. Do you have
> suggestions here ?

Since drc->awaiting_release is only set in cases where the corresponding
dimm is pending delete, I think maybe we can rely on
drc->awaiting_release instead and avoid the call out.

> 
> Regards,
> Bharata.
>