Re: [Qemu-devel] [PATCH 3/5] spapr: Add DRC release method

[Greg Kurz]

On Tue, 20 Jun 2017 14:24:08 -0500
Michael Roth <address@hidden> wrote:

> Quoting Greg Kurz (2017-06-20 11:51:45)
> > On Tue, 20 Jun 2017 09:53:30 +0800
> > David Gibson <address@hidden> wrote:
> >   
> > > At the moment, spapr_drc_release() has an ugly switch on the DRC type to
> > > call the right, device-specific release function.  This cleans it up by
> > > doing that via a proper QOM method.
> > > 
> > > It's still arguably an abstraction violation for the DRC code to call into
> > > the specific device code, but one mess at a time.
> > >   
> > 
> > Maybe a second step would be to move DRC subclasses to spapr.c (CPU, LMB) 
> > and
> > spapr_pci.c (PCI) ? This would allow each device-specific release function 
> > to
> > have local scope at least.  
> 
> I kind of prefer the proposed approach of registering a callback
> function via drc_new(). This would make it easier to implement
> unit tests without needing to rely on stub functions, and keeps the
> type-specific handling constrained to matters relating to the DRC
> itself and not the resources associated with them.
> 

Yeah, this could work too and it would address David's concern about
abstraction violation in a clean way.

> > 
> > Reviewed-by: Greg Kurz <address@hidden>
> >   
> > > Signed-off-by: David Gibson <address@hidden>
> > > ---
> > >  hw/ppc/spapr_drc.c         | 22 ++++++----------------
> > >  include/hw/ppc/spapr_drc.h |  1 +
> > >  2 files changed, 7 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index e5dff16..32e39f2 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -370,22 +370,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, 
> > > DeviceState *d, void *fdt,
> > >  
> > >  static void spapr_drc_release(sPAPRDRConnector *drc)
> > >  {
> > > -    /* Calling release callbacks based on spapr_drc_type(drc). */
> > > -    switch (spapr_drc_type(drc)) {
> > > -    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > > -        spapr_core_release(drc->dev);
> > > -        break;
> > > -    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > > -        spapr_phb_remove_pci_device_cb(drc->dev);
> > > -        break;
> > > -    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> > > -        spapr_lmb_release(drc->dev);
> > > -        break;
> > > -    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> > > -    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> > > -    default:
> > > -        g_assert(false);
> > > -    }
> > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +
> > > +    drck->release(drc->dev);
> > >  
> > >      drc->awaiting_release = false;
> > >      g_free(drc->fdt);
> > > @@ -629,6 +616,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, 
> > > void *data)
> > >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> > >      drck->typename = "CPU";
> > >      drck->drc_name_prefix = "CPU ";
> > > +    drck->release = spapr_core_release;
> > >  }
> > >  
> > >  static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> > > @@ -638,6 +626,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, 
> > > void *data)
> > >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> > >      drck->typename = "28";
> > >      drck->drc_name_prefix = "C";
> > > +    drck->release = spapr_phb_remove_pci_device_cb;
> > >  }
> > >  
> > >  static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > > @@ -647,6 +636,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, 
> > > void *data)
> > >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> > >      drck->typename = "MEM";
> > >      drck->drc_name_prefix = "LMB ";
> > > +    drck->release = spapr_lmb_release;
> > >  }
> > >  
> > >  static const TypeInfo spapr_dr_connector_info = {
> > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > index d9cacb3..6fd84d1 100644
> > > --- a/include/hw/ppc/spapr_drc.h
> > > +++ b/include/hw/ppc/spapr_drc.h
> > > @@ -217,6 +217,7 @@ typedef struct sPAPRDRConnectorClass {
> > >      sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
> > >      uint32_t (*isolate)(sPAPRDRConnector *drc);
> > >      uint32_t (*unisolate)(sPAPRDRConnector *drc);
> > > +    void (*release)(DeviceState *dev);
> > >  
> > >      /* QEMU interfaces for managing hotplug operations */
> > >      bool (*release_pending)(sPAPRDRConnector *drc);  
> >   
>

pgp5qWvfyDVRJ.pgp
Description: OpenPGP digital signature