Re: [Qemu-devel] [PATCH RFC 0/3] basic support for composing sysbus devices

[Anthony Liguori]

On 06/13/2011 03:59 PM, Blue Swirl wrote:
> On Sun, Jun 12, 2011 at 10:21 PM, Anthony Liguori<address@hidden>  wrote:
> > On 06/12/2011 12:12 PM, Avi Kivity wrote:
> > > On 06/10/2011 06:43 PM, Anthony Liguori wrote:
> > > > > What exactly is so very wrong about buses that they need to die?
> > > >
> > > > They force a device tree. The device model shouldn't be a tree, but a
> > > > directed graph.
> > >
> > > Right. As an example, you configure PCI interrupt routing and the memory
> > > controller by writing to a PCI device, which logically doesn't have
> > > access to any of this stuff if it's behind the PCI bus.
> > >
> > > However, I don't think buses should die. They should be available as an
> > > easy way to model the devices that do follow the rules. But we should
> > > also expose everything else for the exceptional cases.
> > >
> > > > It's perfectly fine to have a type called PCIBus that I440FX extends,
> > > > but qdev shouldn't have explicit knowledge of something called a "bus"
> > > > IMHO. Doing this forces a limited mechanism of connecting devices
> > > > because it creates an artificial tree (by implying a parent/child
> > > > relationship). It makes composition difficult if not impossible.
> > >
> > > I think qdev buses are useful as long as they don't enforce their
> > > interfaces. That is, a qdev that is a child of a qbus has access to the
> > > qbus's interfaces, but also access to other stuff.
> >
> > I see two independent data structures.  The first is the "instantiation
> > tree".
> >
> > The instantiation tree may look like this:
> >
> > +-- i440fx
> > |  |
> > |  +-- PIIX3
> > |  |  |
> > |  |  +-- mc146818a
> > |  |  +-- uart
> > |  |  +-- DMA
> > |  |  +-- keyboard controller
> > |  |  +-- (remaining platform ISA devices
> > |  |
> > |  +-- UHCI USB controller
> > |  +-- IDE controller
> > |
> > +-- e1000
> > +-- cirrus-vga
> > +-- virtio-balloon-pci
> > +-- IDE disk0
> >
> > Instantiating i440fx makes a bunch of default stuff.  This is composition.
> >   Everything else requires explicit instantiation.  This is, strictly
> > speaking, the parent/child relationships.  If you destroy i440fx, all of
> > it's children have to also go away (by definition). Nothing about bus
> > relationship is implied here.  Even if i440fx exposes a PCI bus, the PIIX3
> > is a child of i440fx even though e1000 is not (even if they're both PCI
> > devices).
>
> I actually like this slot idea in place of buses. But wouldn't there
> be two classes of devices (or two APIs), slot devices and composable
> devices?

All devices have properties.  We have this today in qdev.  What's 
missing is to have a properties who's type is a socket for another 
device.  We really want to be able to do:

static DeviceInfo i440fx_info = {
    .name = "i440fx",
    .props = (Property[]){
       DEFINE_PROP_PLUG(I440FXState, piix3),
       DEFINE_PROP_SOCKET(I440FXState, slot[0]),
       DEFINE_PROP_SOCKET(I440FXState, slot[1]),
       ...
    },
};

Which suggests that we really need to move away from declarative device 
definitions.  It makes it hard to have variable numbers of properties.

In this case, piix3 would be defined as:

struct I440FXState {
    PIIX3 piix3;
    PCIDevice slots[32];
};

Which suggests we need an initfn to do the following:

void i440fx_initfn(...) {
   qdev_init_inplace(&dev->piix3, "PIIX3");
   dev->slot[1] = &dev->piix3->bus;
}

This gets hard to do well in C though.  I'm not sure how we could make 
DEFINE_PROP_PLUG/SOCKET type safe.

Regards,

Anthony Liguori