Re: [Qemu-devel] [RFC PATCH v4 2/5] spapr: Add LMB DR connectors

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

On Wed, Jun 24, 2015 at 07:49:31AM +0530, Bharata B Rao wrote:
> On Tue, Jun 23, 2015 at 11:32:34AM +1000, David Gibson wrote:
> > On Fri, Jun 19, 2015 at 03:47:54PM +0530, Bharata B Rao wrote:
> > > Enable memory hotplug for pseries 2.4 and add LMB DR connectors.
> > > With memory hotplug, enforce NUMA node memory size and maxmem to be
> > > a multiple of SPAPR_MEMORY_BLOCK_SIZE (256M) since that's the granularity
> > > in which LMBs are represented and hot-added.
> > > 
> > > LMB DR connectors will be used by the memory hotplug code.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > >                [spapr_drc_reset implementation]
> > > ---
> > >  hw/ppc/spapr.c         | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h |  2 ++
> > >  2 files changed, 80 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 87a29dc..f9af89b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -59,6 +59,7 @@
> > >  #include "hw/nmi.h"
> > >  
> > >  #include "hw/compat.h"
> > > +#include "qemu-common.h"
> > >  
> > >  #include <libfdt.h>
> > >  
> > > @@ -1436,10 +1437,76 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
> > >      qemu_register_reset(spapr_cpu_reset, cpu);
> > >  }
> > >  
> > > +static void spapr_drc_reset(void *opaque)
> > 
> > This function needs a different name, since it's only called for LMB
> > drcs, not all drcs.
> > 
> > > +{
> > > +    sPAPRDRConnector *drc = opaque;
> > > +    DeviceState *d = DEVICE(drc);
> > > +
> > > +    if (d) {
> > > +        device_reset(d);
> > > +    }
> > > +}
> > > +
> > > +static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
> > > +{
> > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > +    uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
> > > +    uint32_t nr_rma_lmbs = spapr->rma_size/lmb_size;
> > > +    uint32_t nr_lmbs = machine->maxram_size/lmb_size - nr_rma_lmbs;
> > > +    uint32_t nr_assigned_lmbs = machine->ram_size/lmb_size - nr_rma_lmbs;
> > > +    int i;
> > > +
> > > +    for (i = 0; i < nr_lmbs; i++) {
> > > +        sPAPRDRConnector *drc;
> > > +        uint64_t addr;
> > > +
> > > +        if (i < nr_assigned_lmbs) {
> > > +            addr = (i + nr_rma_lmbs) * lmb_size;
> > > +        } else {
> > > +            addr = (i - nr_assigned_lmbs) * lmb_size +
> > > +                SPAPR_MACHINE(qdev_get_machine())->hotplug_memory.base;
> > > +        }
> > > +
> > > +        drc = spapr_dr_connector_new(qdev_get_machine(),
> > > +                SPAPR_DR_CONNECTOR_TYPE_LMB, addr/lmb_size);
> > > +        qemu_register_reset(spapr_drc_reset, drc);
> > 
> > Actually.. I'm not sure what spapr_drc_reset is needed for at all.
> > Won't the device reset hook get called through the normal qdev path
> > anyway?  The PCI hotplug code doesn't have an explicit register_reset,
> > so why does the memory hotplug code need it?

In case of PCI hotplug, the reset for DR devices are handled by
spapr_phb_reset which walks the children and does device_reset for
all DR child devices.

In case of Memory and CPU hotplug, DR devices end up as children
of spapr machine object. So we have two options.

1. The current option where individual DR devices register a reset
handler (spapr_drc_reset) which gets invoked when ppc_spapr_reset
invokes qemu_devices_reset.

2. Let individual DR objects not register their own reset handlers, but
let ppc_spapr_reset walk through its children and invoke reset for them
like below:

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 060fadc..da31152 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1061,6 +1061,16 @@ static int spapr_check_htab_fd(sPAPRMachineState *spapr)
     return rc;
 }
 
+static int spapr_drc_reset(Object *child, void *opaque)
+{
+    DeviceState *d = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE);
+
+    if (d) {
+        device_reset(d);
+    }
+    return 0;
+}
+
 static void ppc_spapr_reset(void)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
@@ -1073,6 +1083,8 @@ static void ppc_spapr_reset(void)
     /* Reset the hash table & recalc the RMA */
     spapr_reset_htab(spapr);
 
+    object_child_foreach(qdev_get_machine(), spapr_drc_reset, NULL);
+
     qemu_devices_reset();
 
     /*

Let me know which one you prefer.

Regards,
Bharata.