| Message ID | 1439967371-15870-4-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 >
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.
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 {
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. >
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 {
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(-)