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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 13 Apr 2015 08:33:16 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> 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 ?
memory hotplug is not applicable to all machines so answer probably no.

But it should be possible create MachineHotplugMemory interface
to help us get around single parent class limit od QOM and get mem hotplug code
shared in a nice way.

> 
> Regards,
> Bharata.
>