On Mon, Jun 05, 2017 at 11:32:07AM -0500, Michael Roth wrote: > Quoting David Gibson (2017-06-04 22:31:12) > > Currently we only have a single QOM type for all DRCs, but lots of > > places where we switch behaviour based on the DRC's PAPR defined type. > > This is a poor use of our existing type system. > > > > So, instead create QOM subclasses for each PAPR defined DRC type. We > > also introduce intermediate subclasses for physical and logical DRCs, > > a division which will be useful later on. > > > > Instead of being stored in the DRC object itself, the PAPR type is now > > stored in the class structure. There are still many places where we > > switch directly on the PAPR type value, but this at least provides the > > basis to start to remove those. > > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > Reviewed-by: Michael Roth <mdr...@linux.vnet.ibm.com> > > > --- > > hw/ppc/spapr.c | 5 +- > > hw/ppc/spapr_drc.c | 123 > > +++++++++++++++++++++++++++++++++------------ > > hw/ppc/spapr_pci.c | 3 +- > > include/hw/ppc/spapr_drc.h | 47 +++++++++++++++-- > > 4 files changed, 139 insertions(+), 39 deletions(-) > > > > Mike, here's an updated version of the patch addressing the problems > > you pointed out. If I can get an ack, I can merge the series, which > > would be nice. > > Series: > > Acked-by: Michael Roth <mdr...@linux.vnet.ibm.com>
Thanks, I've merged this into ppc-for-2.10.
>
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 5d10366..456f9e7 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1911,7 +1911,7 @@ static void
> > spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
> > uint64_t addr;
> >
> > addr = i * lmb_size + spapr->hotplug_memory.base;
> > - drc = spapr_dr_connector_new(OBJECT(spapr),
> > SPAPR_DR_CONNECTOR_TYPE_LMB,
> > + drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> > addr/lmb_size);
> > qemu_register_reset(spapr_drc_reset, drc);
> > }
> > @@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> >
> > if (mc->has_hotpluggable_cpus) {
> > sPAPRDRConnector *drc =
> > - spapr_dr_connector_new(OBJECT(spapr),
> > - SPAPR_DR_CONNECTOR_TYPE_CPU,
> > + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> > (core_id / smp_threads) * smt);
> >
> > qemu_register_reset(spapr_drc_reset, drc);
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index a35314e..8575022 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift
> > get_type_shift(sPAPRDRConnectorType type)
> > return shift;
> > }
> >
> > +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +
> > + return 1 << drck->typeshift;
> > +}
> > +
> > uint32_t spapr_drc_index(sPAPRDRConnector *drc)
> > {
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +
> > /* no set format for a drc index: it only needs to be globally
> > * unique. this is how we encode the DRC type on bare-metal
> > * however, so might as well do that here
> > */
> > - return (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) |
> > - (drc->id & DRC_INDEX_ID_MASK);
> > + return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
> > + | (drc->id & DRC_INDEX_ID_MASK);
> > }
> >
> > static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > @@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector
> > *drc,
> > * If the LMB being removed doesn't belong to a DIMM device that is
> > * actually being unplugged, fail the isolation request here.
> > */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> > !drc->awaiting_release) {
> > return RTAS_OUT_HW_ERROR;
> > @@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector
> > *drc,
> > }
> > }
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->allocation_state = state;
> > if (drc->awaiting_release &&
> > drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > @@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector
> > *drc,
> > return RTAS_OUT_SUCCESS;
> > }
> >
> > -sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> > -{
> > - return drc->type;
> > -}
> > -
> > static const char *get_name(sPAPRDRConnector *drc)
> > {
> > return drc->name;
> > @@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc)
> > static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense
> > *state)
> > {
> > if (drc->dev) {
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > /* for logical DR, we return a state of UNUSABLE
> > * iff the allocation state UNUSABLE.
> > @@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc,
> > sPAPRDREntitySense *state)
> > *state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> > }
> > } else {
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > /* PCI devices, and only PCI devices, use EMPTY
> > * in cases where we'd otherwise use UNUSABLE
> > */
> > @@ -368,7 +372,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState
> > *d, void *fdt,
> > error_setg(errp, "an attached device is still awaiting release");
> > return;
> > }
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > g_assert(drc->allocation_state ==
> > SPAPR_DR_ALLOCATION_STATE_USABLE);
> > }
> > g_assert(fdt || coldplug);
> > @@ -380,7 +384,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState
> > *d, void *fdt,
> > * may be accessing the device, we can easily crash the guest, so we
> > * we defer completion of removal in such cases to the reset() hook.
> > */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > }
> > drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
> > @@ -398,10 +402,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState
> > *d, void *fdt,
> > * 'physical' DR resources such as PCI where each device/resource is
> > * signalled individually.
> > */
> > - drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
> > + drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
> > ? true : coldplug;
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->awaiting_allocation = true;
> > }
> >
> > @@ -441,7 +445,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState
> > *d, Error **errp)
> > return;
> > }
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> > drc->awaiting_release = true;
> > @@ -458,8 +462,8 @@ static void detach(sPAPRDRConnector *drc, DeviceState
> > *d, Error **errp)
> >
> > drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> >
> > - /* Calling release callbacks based on drc->type. */
> > - switch (drc->type) {
> > + /* 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;
> > @@ -515,7 +519,7 @@ static void reset(DeviceState *d)
> > }
> >
> > /* non-PCI devices may be awaiting a transition to UNUSABLE */
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->awaiting_release) {
> > drck->set_allocation_state(drc,
> > SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> > }
> > @@ -544,7 +548,7 @@ static bool spapr_drc_needed(void *opaque)
> > * If there is dev plugged in, we need to migrate the DRC state when
> > * it is different from cold-plugged state
> > */
> > - switch (drc->type) {
> > + switch (spapr_drc_type(drc)) {
> > case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > rc = !((drc->isolation_state ==
> > SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
> > (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE)
> > &&
> > @@ -630,17 +634,12 @@ static void unrealize(DeviceState *d, Error **errp)
> > }
> > }
> >
> > -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > - sPAPRDRConnectorType type,
> > +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> > uint32_t id)
> > {
> > - sPAPRDRConnector *drc =
> > - SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> > + sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(type));
> > char *prop_name;
> >
> > - g_assert(type);
> > -
> > - drc->type = type;
> > drc->id = id;
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > @@ -670,7 +669,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > * DRC names as documented by PAPR+ v2.7, 13.5.2.4
> > * location codes as documented by PAPR+ v2.7, 12.3.1.5
> > */
> > - switch (drc->type) {
> > + switch (spapr_drc_type(drc)) {
> > case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > drc->name = g_strdup_printf("CPU %d", id);
> > break;
> > @@ -689,7 +688,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > }
> >
> > /* PCI slot always start in a USABLE state, and stay there */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> > }
> >
> > @@ -741,6 +740,27 @@ static void spapr_dr_connector_class_init(ObjectClass
> > *k, void *data)
> > dk->user_creatable = false;
> > }
> >
> > +static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> > +}
> > +
> > +static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> > +}
> > +
> > +static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> > +}
> > +
> > static const TypeInfo spapr_dr_connector_info = {
> > .name = TYPE_SPAPR_DR_CONNECTOR,
> > .parent = TYPE_DEVICE,
> > @@ -748,6 +768,42 @@ static const TypeInfo spapr_dr_connector_info = {
> > .instance_init = spapr_dr_connector_instance_init,
> > .class_size = sizeof(sPAPRDRConnectorClass),
> > .class_init = spapr_dr_connector_class_init,
> > + .abstract = true,
> > +};
> > +
> > +static const TypeInfo spapr_drc_physical_info = {
> > + .name = TYPE_SPAPR_DRC_PHYSICAL,
> > + .parent = TYPE_SPAPR_DR_CONNECTOR,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .abstract = true,
> > +};
> > +
> > +static const TypeInfo spapr_drc_logical_info = {
> > + .name = TYPE_SPAPR_DRC_LOGICAL,
> > + .parent = TYPE_SPAPR_DR_CONNECTOR,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .abstract = true,
> > +};
> > +
> > +static const TypeInfo spapr_drc_cpu_info = {
> > + .name = TYPE_SPAPR_DRC_CPU,
> > + .parent = TYPE_SPAPR_DRC_LOGICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_cpu_class_init,
> > +};
> > +
> > +static const TypeInfo spapr_drc_pci_info = {
> > + .name = TYPE_SPAPR_DRC_PCI,
> > + .parent = TYPE_SPAPR_DRC_PHYSICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_pci_class_init,
> > +};
> > +
> > +static const TypeInfo spapr_drc_lmb_info = {
> > + .name = TYPE_SPAPR_DRC_LMB,
> > + .parent = TYPE_SPAPR_DRC_LOGICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_lmb_class_init,
> > };
> >
> > /* helper functions for external users */
> > @@ -858,7 +914,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset,
> > Object *owner,
> > continue;
> > }
> >
> > - if ((drc->type & drc_type_mask) == 0) {
> > + if ((spapr_drc_type(drc) & drc_type_mask) == 0) {
> > continue;
> > }
> >
> > @@ -878,7 +934,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset,
> > Object *owner,
> >
> > /* ibm,drc-types */
> > drc_types = g_string_append(drc_types,
> > - spapr_drc_get_type_str(drc->type));
> > +
> > spapr_drc_get_type_str(spapr_drc_type(drc)));
> > drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
> > }
> >
> > @@ -1210,6 +1266,11 @@ out:
> > static void spapr_drc_register_types(void)
> > {
> > type_register_static(&spapr_dr_connector_info);
> > + type_register_static(&spapr_drc_physical_info);
> > + type_register_static(&spapr_drc_logical_info);
> > + type_register_static(&spapr_drc_cpu_info);
> > + type_register_static(&spapr_drc_pci_info);
> > + type_register_static(&spapr_drc_lmb_info);
> >
> > spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> > rtas_set_indicator);
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 7a208a7..50d673b 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1761,8 +1761,7 @@ static void spapr_phb_realize(DeviceState *dev, Error
> > **errp)
> > /* allocate connectors for child PCI devices */
> > if (sphb->dr_enabled) {
> > for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
> > - spapr_dr_connector_new(OBJECT(phb),
> > - SPAPR_DR_CONNECTOR_TYPE_PCI,
> > + spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
> > (sphb->index << 16) | i);
> > }
> > }
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 10e7c24..969c16d 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -26,6 +26,48 @@
> > #define SPAPR_DR_CONNECTOR(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > TYPE_SPAPR_DR_CONNECTOR)
> >
> > +#define TYPE_SPAPR_DRC_PHYSICAL "spapr-drc-physical"
> > +#define SPAPR_DRC_PHYSICAL_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj,
> > TYPE_SPAPR_DRC_PHYSICAL)
> > +#define SPAPR_DRC_PHYSICAL_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> > + TYPE_SPAPR_DRC_PHYSICAL)
> > +#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_PHYSICAL)
> > +
> > +#define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
> > +#define SPAPR_DRC_LOGICAL_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj,
> > TYPE_SPAPR_DRC_LOGICAL)
> > +#define SPAPR_DRC_LOGICAL_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> > + TYPE_SPAPR_DRC_LOGICAL)
> > +#define SPAPR_DRC_LOGICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_LOGICAL)
> > +
> > +#define TYPE_SPAPR_DRC_CPU "spapr-drc-cpu"
> > +#define SPAPR_DRC_CPU_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_CPU)
> > +#define SPAPR_DRC_CPU_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass,
> > TYPE_SPAPR_DRC_CPU)
> > +#define SPAPR_DRC_CPU(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_CPU)
> > +
> > +#define TYPE_SPAPR_DRC_PCI "spapr-drc-pci"
> > +#define SPAPR_DRC_PCI_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PCI)
> > +#define SPAPR_DRC_PCI_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass,
> > TYPE_SPAPR_DRC_PCI)
> > +#define SPAPR_DRC_PCI(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_PCI)
> > +
> > +#define TYPE_SPAPR_DRC_LMB "spapr-drc-lmb"
> > +#define SPAPR_DRC_LMB_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LMB)
> > +#define SPAPR_DRC_LMB_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass,
> > TYPE_SPAPR_DRC_LMB)
> > +#define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_LMB)
> > +
> > /*
> > * Various hotplug types managed by sPAPRDRConnector
> > *
> > @@ -134,7 +176,6 @@ typedef struct sPAPRDRConnector {
> > /*< private >*/
> > DeviceState parent;
> >
> > - sPAPRDRConnectorType type;
> > uint32_t id;
> > Object *owner;
> > const char *name;
> > @@ -163,6 +204,7 @@ typedef struct sPAPRDRConnectorClass {
> > DeviceClass parent;
> >
> > /*< public >*/
> > + sPAPRDRConnectorTypeShift typeshift;
> >
> > /* accessors for guest-visible (generally via RTAS) DR state */
> > uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> > @@ -186,8 +228,7 @@ typedef struct sPAPRDRConnectorClass {
> > uint32_t spapr_drc_index(sPAPRDRConnector *drc);
> > sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
> >
> > -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > - sPAPRDRConnectorType type,
> > +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> > uint32_t id);
> > sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
> > sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
