Re: [Qemu-devel] [RFC PATCH v2 23/23] spapr: Memory hotplug support

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Thu, Mar 26, 2015 at 02:57:45PM +1100, David Gibson wrote:
> On Mon, Mar 23, 2015 at 07:06:04PM +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 <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c        | 119 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  hw/ppc/spapr_events.c |   3 ++
> >  2 files changed, 120 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 4e844ab..bc46acd 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -61,7 +61,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:
> > @@ -902,6 +903,10 @@ int spapr_h_cas_compose_response(target_ulong addr, target_ulong size,
> >          _FDT((spapr_populate_memory(spapr, fdt)));
> >      }
> >  
> > +    if (spapr->dr_lmb_enabled) {
> > +        _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
> > +    }
> > +
> >      /* Pack resulting tree */
> >      _FDT((fdt_pack(fdt)));
> >  
> > @@ -2185,6 +2190,109 @@ static void spapr_cpu_socket_unplug(HotplugHandler *hotplug_dev,
> >      object_child_foreach(OBJECT(dev), spapr_cpu_core_unplug, errp);
> >  }
> >  
> > +static void spapr_add_lmbs(uint64_t addr, uint64_t size, Error **errp)
> > +{
> > +    sPAPRDRConnector *drc;
> > +    uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> > +    int i;
> > +
> > +    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> > +        error_setg(errp, "Hotplugged memory size must be a multiple of "
> > +                      "%d MB", SPAPR_MEMORY_BLOCK_SIZE/(1024 * 1024));
> 
> s/MB/MiB/  there seems to be precedent for using the mebibyte term in
> qemu.
> 
> Also you can use the MiB #define instead of (1024 * 1024).
> 
> > +        return;
> > +    }
> > +
> > +    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);
> > +        spapr_hotplug_req_add_event(drc);
> > +        addr += SPAPR_MEMORY_BLOCK_SIZE;
> > +    }
> > +}
> > +
> > +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 existing_dimms_capacity = 0;
> > +    uint64_t align = TARGET_PAGE_SIZE;
> > +    uint64_t addr;
> > +
> > +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (memory_region_get_alignment(mr) && ms->enforce_aligned_dimm) {
> > +        align = memory_region_get_alignment(mr);
> > +    }
> > +
> > +    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;
> > +    }
> > +
> > +    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (existing_dimms_capacity + memory_region_size(mr) >
> > +        machine->maxram_size - machine->ram_size) {
> > +        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> > +                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> > +                   existing_dimms_capacity,
> > +                   machine->maxram_size - machine->ram_size);
> > +        goto out;
> > +    }
> > +
> > +    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    trace_mhp_pc_dimm_assigned_address(addr);
> > +
> > +    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> > +                                 machine->ram_slots, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    trace_mhp_pc_dimm_assigned_slot(slot);
> > +
> > +    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > +        error_setg(&local_err, "hypervisor has no free memory slots left");
> > +        goto out;
> > +    }
> > +
> > +    memory_region_add_subregion(&ms->hotplug_memory,
> > +                                addr - ms->hotplug_memory_base, mr);
> > +    vmstate_register_ram(mr, dev);
> > +
> > +    spapr_add_lmbs(addr, memory_region_size(mr), &local_err);
> 
> It really looks as if it should be possible to make a common function
> covering most of this and pc_dimm_plug, with the only difference being
> the final call to do the arch specific stuff.  Even that might be able
> to just be a hotplug handler call if you add a parameter for the
> apprpirate hotplughandler object, instead of assuming the acpi device
> on the PC side.

This routine and the equivalent x86 implementation use hotplug_memory_base
and hotplug_memory fields from sPAPRMachineState and PCMachineState
respectively. If we move these fields to MachineState, then it will be much
easier to share the common code. Is that fine ?

Regards,
Bharata.