Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types

[Michael S. Tsirkin]

On Mon, Jul 22, 2013 at 07:42:43PM +0200, Andreas Färber wrote:
> Am 21.07.2013 22:26, schrieb Michael S. Tsirkin:
> > On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
> >> Signed-off-by: Andreas Färber <address@hidden>
> >> ---
> >>  hw/pci-bridge/ioh3420.c            | 23 ++++++++++-------------
> >>  hw/pci-bridge/xio3130_downstream.c | 23 ++++++++++-------------
> >>  hw/pci-bridge/xio3130_upstream.c   | 15 +++++++--------
> >>  hw/pci/pcie_port.c                 | 22 ++++++++++++++++++++++
> >>  include/hw/pci/pcie_port.h         | 14 ++++++++++++--
> >>  5 files changed, 61 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> >> index 728f658..83db054 100644
> >> --- a/hw/pci-bridge/ioh3420.c
> >> +++ b/hw/pci-bridge/ioh3420.c
> >> @@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
> >>  
> >>  static int ioh3420_initfn(PCIDevice *d)
> >>  {
> >> -    PCIBridge *br = PCI_BRIDGE(d);
> >> -    PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >> -    PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
> >> +    PCIEPort *p = PCIE_PORT(d);
> >> +    PCIESlot *s = PCIE_SLOT(d);
> >>      int rc;
> >>  
> >>      rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
> >> @@ -148,9 +147,7 @@ err_bridge:
> >>  
> >>  static void ioh3420_exitfn(PCIDevice *d)
> >>  {
> >> -    PCIBridge *br = PCI_BRIDGE(d);
> >> -    PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >> -    PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
> >> +    PCIESlot *s = PCIE_SLOT(d);
> >>  
> >>      pcie_aer_exit(d);
> >>      pcie_chassis_del_slot(s);
> >> @@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool 
> >> multifunction,
> >>      qdev_prop_set_uint16(qdev, "slot", slot);
> >>      qdev_init_nofail(qdev);
> >>  
> >> -    return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
> >> +    return PCIE_SLOT(d);
> >>  }
> >>  
> >>  static const VMStateDescription vmstate_ioh3420 = {
> >> @@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
> >>      .minimum_version_id_old = 1,
> >>      .post_load = pcie_cap_slot_post_load,
> >>      .fields = (VMStateField[]) {
> >> -        VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
> >> -        VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
> >> +        VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
> >> +        VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
> >>                         vmstate_pcie_aer_log, PCIEAERLog),
> >>          VMSTATE_END_OF_LIST()
> >>      }
> >>  };
> >>  
> >>  static Property ioh3420_properties[] = {
> >> -    DEFINE_PROP_UINT8("port", PCIESlot, port.port, 0),
> >> +    DEFINE_PROP_UINT8("port", PCIEPort, port, 0),
> >>      DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0),
> >>      DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0),
> >> -    DEFINE_PROP_UINT16("aer_log_max", PCIESlot,
> >> -                       port.br.parent_obj.exp.aer_log.log_max,
> >> +    DEFINE_PROP_UINT16("aer_log_max", PCIDevice,
> >> +                       exp.aer_log.log_max,
> >>                         PCIE_AER_LOG_MAX_DEFAULT),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> > 
> > 
> > This looks scary. This does a cast to different types
> > without any checks at all.
> 
> It doesn't. AFAIU both VMStateDescription and qdev properties store a
> numerical offset from the object pointer. CC'ing Juan and Paolo/Anthony.
> 
> I.e., if I don't find a counter-example we can drop the arguments from
> VMSTATE_PCI_DEVICE() and VMSTATE_PCIE_DEVICE() completely since the
> offset would always be 0 and the field name can be hardcoded, cf.
> VMSTATE_CPU() in qom/cpu.h.

Currently VMSTATE macros are safe since they all use
the same structure.

You are making them use different types with no
compile or runtime checking that the correct
type is used.

> 
> > What are the advantages of this hack?
> 
> The concrete problems I encountered in this series were a) 80 chars
> limit and b) parent_obj.parent_obj.foo.
> Since offsetof(MyState, parent_obj.foo) == offsetof(ParentState, foo),
> this was a neat way to shorten the line.

I don't think it's a good reason.

> The main point of the series is introducing abstract QOM types matching
> a specific struct. Renaming the parent field helps catch users that
> should no longer be accessing it, both in compile-testing and in future
> patch review.
> 
> What we would do about VMState when someone at some future point wants
> to move PCI state into a QOM interface I have no clue, I'll let the
> person who throws the first stone worry about that. ;)
> 
> Regards,
> Andreas

NACK

Please find a way to have code that won't silently
break when we refactor code.
Normal QOM type use has runtime checks to address that.


> >> @@ -228,7 +225,7 @@ static void ioh3420_class_init(ObjectClass *klass, 
> >> void *data)
> >>  
> >>  static const TypeInfo ioh3420_info = {
> >>      .name          = "ioh3420",
> >> -    .parent        = TYPE_PCI_BRIDGE,
> >> +    .parent        = TYPE_PCIE_SLOT,
> >>      .instance_size = sizeof(PCIESlot),
> >>      .class_init    = ioh3420_class_init,
> >>  };
> >> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> >> b/hw/pci-bridge/xio3130_downstream.c
> >> index 9acce3f..549ef26 100644
> >> --- a/hw/pci-bridge/xio3130_downstream.c
> >> +++ b/hw/pci-bridge/xio3130_downstream.c
> >> @@ -56,9 +56,8 @@ static void xio3130_downstream_reset(DeviceState *qdev)
> >>  
> >>  static int xio3130_downstream_initfn(PCIDevice *d)
> >>  {
> >> -    PCIBridge *br = PCI_BRIDGE(d);
> >> -    PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >> -    PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
> >> +    PCIEPort *p = PCIE_PORT(d);
> >> +    PCIESlot *s = PCIE_SLOT(d);
> >>      int rc;
> >>  
> >>      rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
> >> @@ -113,9 +112,7 @@ err_bridge:
> >>  
> >>  static void xio3130_downstream_exitfn(PCIDevice *d)
> >>  {
> >> -    PCIBridge *br = PCI_BRIDGE(d);
> >> -    PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >> -    PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
> >> +    PCIESlot *s = PCIE_SLOT(d);
> >>  
> >>      pcie_aer_exit(d);
> >>      pcie_chassis_del_slot(s);
> >> @@ -147,7 +144,7 @@ PCIESlot *xio3130_downstream_init(PCIBus *bus, int 
> >> devfn, bool multifunction,
> >>      qdev_prop_set_uint16(qdev, "slot", slot);
> >>      qdev_init_nofail(qdev);
> >>  
> >> -    return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
> >> +    return PCIE_SLOT(d);
> >>  }
> >>  
> >>  static const VMStateDescription vmstate_xio3130_downstream = {
> >> @@ -157,19 +154,19 @@ static const VMStateDescription 
> >> vmstate_xio3130_downstream = {
> >>      .minimum_version_id_old = 1,
> >>      .post_load = pcie_cap_slot_post_load,
> >>      .fields = (VMStateField[]) {
> >> -        VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
> >> -        VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
> >> +        VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
> >> +        VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
> >>                         vmstate_pcie_aer_log, PCIEAERLog),
> >>          VMSTATE_END_OF_LIST()
> >>      }
> >>  };
> >>  
> >>  static Property xio3130_downstream_properties[] = {
> >> -    DEFINE_PROP_UINT8("port", PCIESlot, port.port, 0),
> >> +    DEFINE_PROP_UINT8("port", PCIEPort, port, 0),
> >>      DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0),
> >>      DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0),
> >> -    DEFINE_PROP_UINT16("aer_log_max", PCIESlot,
> >> -                       port.br.parent_obj.exp.aer_log.log_max,
> >> +    DEFINE_PROP_UINT16("aer_log_max", PCIDevice,
> >> +                       exp.aer_log.log_max,
> >>                         PCIE_AER_LOG_MAX_DEFAULT),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> > 
> > 
> > Same question.
> > 
> >> @@ -195,7 +192,7 @@ static void xio3130_downstream_class_init(ObjectClass 
> >> *klass, void *data)
> >>  
> >>  static const TypeInfo xio3130_downstream_info = {
> >>      .name          = "xio3130-downstream",
> >> -    .parent        = TYPE_PCI_BRIDGE,
> >> +    .parent        = TYPE_PCIE_SLOT,
> >>      .instance_size = sizeof(PCIESlot),
> >>      .class_init    = xio3130_downstream_class_init,
> >>  };
> >> diff --git a/hw/pci-bridge/xio3130_upstream.c 
> >> b/hw/pci-bridge/xio3130_upstream.c
> >> index 6bbcf98..b1ee464 100644
> >> --- a/hw/pci-bridge/xio3130_upstream.c
> >> +++ b/hw/pci-bridge/xio3130_upstream.c
> >> @@ -53,8 +53,7 @@ static void xio3130_upstream_reset(DeviceState *qdev)
> >>  
> >>  static int xio3130_upstream_initfn(PCIDevice *d)
> >>  {
> >> -    PCIBridge *br = PCI_BRIDGE(d);
> >> -    PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >> +    PCIEPort *p = PCIE_PORT(d);
> >>      int rc;
> >>  
> >>      rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
> >> @@ -125,7 +124,7 @@ PCIEPort *xio3130_upstream_init(PCIBus *bus, int 
> >> devfn, bool multifunction,
> >>      qdev_prop_set_uint8(qdev, "port", port);
> >>      qdev_init_nofail(qdev);
> >>  
> >> -    return DO_UPCAST(PCIEPort, br, br);
> >> +    return PCIE_PORT(d);
> >>  }
> >>  
> >>  static const VMStateDescription vmstate_xio3130_upstream = {
> >> @@ -134,8 +133,8 @@ static const VMStateDescription 
> >> vmstate_xio3130_upstream = {
> >>      .minimum_version_id = 1,
> >>      .minimum_version_id_old = 1,
> >>      .fields = (VMStateField[]) {
> >> -        VMSTATE_PCIE_DEVICE(br.parent_obj, PCIEPort),
> >> -        VMSTATE_STRUCT(br.parent_obj.exp.aer_log, PCIEPort, 0,
> >> +        VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
> >> +        VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
> >>                         vmstate_pcie_aer_log, PCIEAERLog),
> >>          VMSTATE_END_OF_LIST()
> >>      }
> >> @@ -143,8 +142,8 @@ static const VMStateDescription 
> >> vmstate_xio3130_upstream = {
> >>  
> >>  static Property xio3130_upstream_properties[] = {
> >>      DEFINE_PROP_UINT8("port", PCIEPort, port, 0),
> >> -    DEFINE_PROP_UINT16("aer_log_max", PCIEPort,
> >> -                       br.parent_obj.exp.aer_log.log_max,
> >> +    DEFINE_PROP_UINT16("aer_log_max", PCIDevice,
> >> +                       exp.aer_log.log_max,
> >>      PCIE_AER_LOG_MAX_DEFAULT),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> > 
> > Same question.
> > 
> > 
> >> @@ -170,7 +169,7 @@ static void xio3130_upstream_class_init(ObjectClass 
> >> *klass, void *data)
> >>  
> >>  static const TypeInfo xio3130_upstream_info = {
> >>      .name          = "x3130-upstream",
> >> -    .parent        = TYPE_PCI_BRIDGE,
> >> +    .parent        = TYPE_PCIE_PORT,
> >>      .instance_size = sizeof(PCIEPort),
> >>      .class_init    = xio3130_upstream_class_init,
> >>  };
> >> diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
> >> index 91b53a0..a5333a7 100644
> >> --- a/hw/pci/pcie_port.c
> >> +++ b/hw/pci/pcie_port.c
> >> @@ -116,3 +116,25 @@ void pcie_chassis_del_slot(PCIESlot *s)
> >>  {
> >>      QLIST_REMOVE(s, next);
> >>  }
> >> +
> >> +static const TypeInfo pcie_port_type_info = {
> >> +    .name = TYPE_PCIE_PORT,
> >> +    .parent = TYPE_PCI_BRIDGE,
> >> +    .instance_size = sizeof(PCIEPort),
> >> +    .abstract = true,
> >> +};
> >> +
> >> +static const TypeInfo pcie_slot_type_info = {
> >> +    .name = TYPE_PCIE_SLOT,
> >> +    .parent = TYPE_PCIE_PORT,
> >> +    .instance_size = sizeof(PCIESlot),
> >> +    .abstract = true,
> >> +};
> >> +
> >> +static void pcie_port_register_types(void)
> >> +{
> >> +    type_register_static(&pcie_port_type_info);
> >> +    type_register_static(&pcie_slot_type_info);
> >> +}
> >> +
> >> +type_init(pcie_port_register_types)
> >> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> >> index d89aa61..e167bf7 100644
> >> --- a/include/hw/pci/pcie_port.h
> >> +++ b/include/hw/pci/pcie_port.h
> >> @@ -24,8 +24,13 @@
> >>  #include "hw/pci/pci_bridge.h"
> >>  #include "hw/pci/pci_bus.h"
> >>  
> >> +#define TYPE_PCIE_PORT "pcie-port"
> >> +#define PCIE_PORT(obj) OBJECT_CHECK(PCIEPort, (obj), TYPE_PCIE_PORT)
> >> +
> >>  struct PCIEPort {
> >> -    PCIBridge   br;
> >> +    /*< private >*/
> >> +    PCIBridge   parent_obj;
> >> +    /*< public >*/
> >>  
> >>      /* pci express switch port */
> >>      uint8_t     port;
> >> @@ -33,8 +38,13 @@ struct PCIEPort {
> >>  
> >>  void pcie_port_init_reg(PCIDevice *d);
> >>  
> >> +#define TYPE_PCIE_SLOT "pcie-slot"
> >> +#define PCIE_SLOT(obj) OBJECT_CHECK(PCIESlot, (obj), TYPE_PCIE_SLOT)
> >> +
> >>  struct PCIESlot {
> >> -    PCIEPort    port;
> >> +    /*< private >*/
> >> +    PCIEPort    parent_obj;
> >> +    /*< public >*/
> >>  
> >>      /* pci express switch port with slot */
> >>      uint8_t     chassis;
> >> -- 
> >> 1.8.1.4
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg