| Message ID | 20170620015332.13874-4-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On Tue, 20 Jun 2017 09:53:30 +0800 David Gibson <david@gibson.dropbear.id.au> 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. Reviewed-by: Greg Kurz <groug@kaod.org> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > 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);
On 20/06/2017 03:53, David Gibson 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. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Laurent Vivier <lvivier@redhat.com> > --- > 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); >
Quoting Greg Kurz (2017-06-20 11:51:45) > On Tue, 20 Jun 2017 09:53:30 +0800 > David Gibson <david@gibson.dropbear.id.au> 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. > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > 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); >
On Tue, Jun 20, 2017 at 02:24:08PM -0500, Michael Roth wrote: > Quoting Greg Kurz (2017-06-20 11:51:45) > > On Tue, 20 Jun 2017 09:53:30 +0800 > > David Gibson <david@gibson.dropbear.id.au> 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 was already thinking of doing that in a future patch :) > 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. True. We went from the callback to this ugly switch when we made the DRCs migratable (well, sort of, except for the many remaining migration problems these cleanups are addressing amongst other things). But moving the callback registration from attach time to new time would do just as well. Well I'll think about it and do one or the other in future.
On Tue, 20 Jun 2017 14:24:08 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > Quoting Greg Kurz (2017-06-20 11:51:45) > > On Tue, 20 Jun 2017 09:53:30 +0800 > > David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org> > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > --- > > > 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); > > >
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);
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. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_drc.c | 22 ++++++---------------- include/hw/ppc/spapr_drc.h | 1 + 2 files changed, 7 insertions(+), 16 deletions(-)