Re: [Qemu-ppc] [RFC PATCH v1] spapr: Memory hot-unplug support

[Nathan Fontenot]

On 10/26/2015 04:53 AM, Bharata B Rao wrote:
> Add support to hot remove pc-dimm memory devices.
> 
> TODO: In response to memory hot removal operation on a DIMM device,
> guest kernel might refuse to offline a few LMBs that are part of that device.
> In such cases, we will have a DIMM device that has some LMBs online and some
> LMBs offline. To avoid this situation, drmgr could be enhanced to support
> a command line option that results in removal of all the LMBs or none.
>

We had discussed an update to drmgr previously that would allow one to specify
that they wanted to remove a certain number of LMBs starting at a specified drc
index. I would assume we could incorporate the notion that it is an all or
nothing request into that.

Unfortunately right now drmgr will attempt to remove all of the requested LMBs
but it does a best effort. This results in situations where some LMB's are not
removed and some are.

-Nathan 
 
> Signed-off-by: Bharata B Rao <address@hidden>
> ---
> Changes in v1:
> - Got rid of the patch that introduced a field in PCDIMMDevice to track
>   DIMM marked for removal since we can track that using within DRC
>   object.
> - Removed the patch that added return value to rtas_set_indicator()
>   since the required changes are already pushed by Michael Roth.
> 
> v0:
> 
>  hw/ppc/spapr.c     | 90 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/ppc/spapr_drc.c | 18 +++++++++++
>  2 files changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e1202ce..f5b1ac2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2174,6 +2174,85 @@ out:
>      error_propagate(errp, local_err);
>  }
> 
> +typedef struct sPAPRDIMMState {
> +    uint32_t nr_lmbs;
> +} sPAPRDIMMState;
> +
> +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;
> +    }
> +
> +    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)
>  {
> @@ -2221,7 +2300,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);
>      }
>  }
> 
> @@ -2263,6 +2350,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> void *data)
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
>      mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> +    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 5d6ea7c..59b6ea9 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"
> @@ -77,6 +78,23 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>          }
>      }
> 
> +    /*
> +     * Fail any request 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.
> +     */
> +    if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> +        if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> +             !drc->awaiting_release) {
> +            return RTAS_OUT_HW_ERROR;
> +        }
> +    }
> +
>      drc->isolation_state = state;
> 
>      if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
>