Re: [Qemu-devel] [RFC PATCH v0 4/5] spapr: Support hotplug by specifying DRC count

[Bharata B Rao]

On Mon, Aug 03, 2015 at 04:55:01PM +1000, David Gibson wrote:
> On Mon, Aug 03, 2015 at 11:05:42AM +0530, Bharata B Rao wrote:
> > Support hotplug identifier type RTAS_LOG_V6_HP_ID_DRC_COUNT that allows
> > hotplugging of DRCs by specifying the DRC count.
> > 
> > While we are here, rename
> > 
> > spapr_hotplug_req_add_event() to spapr_hotplug_req_add_by_index()
> > spapr_hotplug_req_remove_event() to spapr_hotplug_req_remove_by_index()
> > 
> > so that they match with spapr_hotplug_req_add_by_count().
> > 
> > Signed-off-by: Bharata B Rao <address@hidden>
> > ---
> >  hw/ppc/spapr.c         |  2 +-
> >  hw/ppc/spapr_events.c  | 47 ++++++++++++++++++++++++++++++++++++++---------
> >  hw/ppc/spapr_pci.c     |  4 ++--
> >  include/hw/ppc/spapr.h |  8 ++++++--
> >  4 files changed, 47 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 669dc43..13af9be 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2072,7 +2072,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t 
> > addr, uint64_t size,
> >  
> >          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >          drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> > -        spapr_hotplug_req_add_event(drc);
> > +        spapr_hotplug_req_add_by_index(drc);
> >          addr += SPAPR_MEMORY_BLOCK_SIZE;
> >      }
> >  }
> > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > index 98bf7ae..744ea62 100644
> > --- a/hw/ppc/spapr_events.c
> > +++ b/hw/ppc/spapr_events.c
> > @@ -386,7 +386,9 @@ static void spapr_powerdown_req(Notifier *n, void 
> > *opaque)
> >      qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
> >  }
> >  
> > -static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t 
> > hp_action)
> > +static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> > +                                    sPAPRDRConnectorType drc_type,
> > +                                    uint32_t drc)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      struct hp_log_full *new_hp;
> > @@ -395,8 +397,6 @@ static void spapr_hotplug_req_event(sPAPRDRConnector 
> > *drc, uint8_t hp_action)
> >      struct rtas_event_log_v6_maina *maina;
> >      struct rtas_event_log_v6_mainb *mainb;
> >      struct rtas_event_log_v6_hp *hp;
> > -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > -    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> >  
> >      new_hp = g_malloc0(sizeof(struct hp_log_full));
> >      hdr = &new_hp->hdr;
> > @@ -427,8 +427,7 @@ static void spapr_hotplug_req_event(sPAPRDRConnector 
> > *drc, uint8_t hp_action)
> >      hp->hdr.section_length = cpu_to_be16(sizeof(*hp));
> >      hp->hdr.section_version = 1; /* includes extended modifier */
> >      hp->hotplug_action = hp_action;
> > -    hp->drc.index = cpu_to_be32(drck->get_index(drc));
> > -    hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
> > +    hp->hotplug_identifier = hp_id;
> >  
> >      switch (drc_type) {
> >      case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > @@ -445,19 +444,49 @@ static void spapr_hotplug_req_event(sPAPRDRConnector 
> > *drc, uint8_t hp_action)
> >          return;
> >      }
> >  
> > +    if (hp_id == RTAS_LOG_V6_HP_ID_DRC_COUNT) {
> > +        hp->drc.count = cpu_to_be32(drc);
> 
> I'm a bit confused as to how this message can work with *only* a
> count and not some sort of base index.

Right and this can be an issue when we start supporting memory removal
and the currrent userspace drmgr tool will just go ahead and remove
'count' number of LMBs that have been marked for removal and there is
really no association b/n these LMBs to the LMBs that make up the
pc-dimm device that is being removed.

One solution is to extend the rtas event header and include the base
drc-index (with count type of identifier)  so that we know exactly which
set of LMBs to remove for the given pc-dimm device.

Michael has some thoughts about alternative ways in thich we can achieve
removal of correct LMBs without needing to pass the base drc-index.

Regards,
Bharata.