Re: [Qemu-devel] [RFC] Plan for moving forward with QOM

[Anthony Liguori]

On 12/13/2011 11:40 AM, Paul Brook wrote:
> > It's multiple inheritance but interfaces (implements) cannot carry state
> > which avoids an inheritance diamond problem.
>
> > > It sounds like some
> > > form of multiple inheritance, but it's unclear to me which should be the
> > > primary type.  For PCI host bridges like the i440fx we currently have to
> > > split them into two, one fot the host bus and the other for the PCI bus.
> > >   It feels like if we design the model properly this should no longer be
> > > necessary.
> >
> > Yes, that's exactly what this solves.  I think we could probably make
> > i440fx is-a PCIDevice that implements a PciBus.  It's the same thing we
> > would do with a PCI Bridge.
>
> Hmm, so how does that work given all of SysbusDevice, PCIDevice and PciBus
> have state?

Depends.  If you look at PCIBus as an example:

struct PCIBus {
    BusState qbus;
    uint8_t devfn_min;

This is a characteristic that isn't common to all PCI busses.

    pci_set_irq_fn set_irq;
    pci_map_irq_fn map_irq;
    pci_hotplug_fn hotplug;

These are essentially overload points and would be more natural if PCIBus was an 
interface.

    DeviceState *hotplug_qdev;

This is a trick to support hotplug that can be refactored more nicely.

    void *irq_opaque;

This is part of the set_irq interface.

    PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
    PCIDevice *parent_dev;

These are naturally links.

    MemoryRegion *address_space_mem;
    MemoryRegion *address_space_io;

This would naturally be replaced with a pure virtual function overloaded by the 
child.  A virtual function may be even better that simply used 
get_system_mem()/get_system_io().

    QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
    QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */

Relics of the fact that PCI hasn't been full converted to an object model.

    /* The bus IRQ state is the logical OR of the connected devices.
       Keep a count of the number of devices with raised IRQs.  */
    int nirq;
    int *irq_count;

Probably more naturally expressed as an AND gate.  With a stateful pin model, 
this would end up modeled quite nicely.

So at least for PCI, I think PCIBus as an interface works very well.

> Making PciBus a separate (composition/shild) object is IMHO not a
> bad thing anyway - I dislike saying that the device and the bus are the same
> thing.

Yeah, in some cases, this will almost certainly be the right thing to do.

> That still leaves both PCIDevice and SysBusDevice state.  The i440fx
> is a fairly simple device, but more complex bridges have additional state that
> needs to be accessible from both the PCI and SysBus interfaces.

SysBus is so badly abused today I think we would have to reevaluate what to do 
with it once we fix up some of the devices that make use of it.

> Similar examples occur when we have an audio codec with both an i2c command
> interface and an SSI data port.
>
> > > I'm not so sure we should have Is-a at all.
> >
> > A point that has been repeatedly brought up is that you can avoid primary
> > inheritance and just rely on composition using composed interfaces as your
> > communication vehicle.
> >
> > I think in some cases, that will make sense, and the object model allows
> > that.
>
> I'm not sure choice is a good thing here!  One of the mistakes I made with
> qdev was to insufficiently document how the device model was supposed to fit
> together.

FWIW, my next TODO is to write up a document on QOM.

I will say, I learned a lot about the design of qdev from rewriting it :-)

> This resulted in several bastardised qdev "conversions" involving
> gratuitous layering violations.

Yes, I never understood your comments at first but now I do understand where we 
went wrong and am very eager to avoid duplicating that.

> The end result was that these devices are
> effectively useless as qdev objects, and propagate all the problems that qdev
> was designed to fix.
>
> It would be good to at least have guidelines on how and why the different
> approaches should be used.  Especially for those of us who maybe aren't
> convesant with the more theoretical/academic principles/terminology behind OO
> design.

Yeah, that's the bit I need help with.  I'll be putting together some slides for 
the next KVM call in January and will also post them on qemu-devel.  I'm going 
to try very hard to explain what the design points are in reasonable terms.

> If it's only a good idea in some cases then you can pretty much guarantee a
> large number of developers will try and use it in the cases where it isn't,
> and vice-versa ;-)
>
> > > It's somewhat unlear what the purpose of composition is.  Using
> > > composition for piix4::piiix and piix3::i8042 seems wrong.  In fact
> > > piix3 shouldn't have any knowledge of i8042 at all, it's just annother
> > > random ISA device.
> >
> > i8042 is not a random ISA device.  It lives in the piix3 and the piix3 has
> > intimate knowledge of it.  When model a20 line propagation, this is
> > actually an important detail.
>
> Huh? I see absoutely no relationship between piix3 and i8042.
> It happens that on the PC machine they are etched into the same piece of
> silicon and one of the i8042 GPIO pins it connected to the A20 gate input, but
> the devices have no actual knowledge of each other.

Composition == "etching to the same piece of silicon".  Nothing more, nothing 
less.

A device has no knowledge of the fact that it is a child of another device.

> If you're wanting to provide a convenient way of instantiating common clumps
> of devices then I think that should be separate.  Likewise defining convenient
> machine specific aliases for users.

There is a separate container device that lets you create arbitrary collections 
of devices, but in many cases, we can achieve better code reuse by using 
composition.

Consider the serial device.  In QOM, I've got it modelled as:

MMSerial has-a UART
ISASerial has-a UART

And there are lots of cases where a design is compatible with a UART16650A but 
adds additional logic/features.  In those cases, we would do:

MCFSerial is-a UART

Now you could certainly remove the has-a logic and make it so something had to 
instantiate a UART separately from the ISASerial device and connect things up 
but that's overly complicated IMHO.

> > > What happens when my device has multiple parts that need to be referred
> > > to by other devices?  An interrupt controller is the most obvious
> > > example, but similar issues occur for any device that has multiple
> > > connections of the same type.  Am I expected to create a whole bunch of
> > > dummy devices and a private interface to bounce everything back to the
> > > main device, somethig like:
> > >
> > > Type: MagicInterruptController
> > >
> > >     Is-a: SystemBusDevice
> > >     Implements: MagicInterruptInterface
> > >
> > >     Properties:
> > >       in_pin0: Composition<MagicInterruptPin, pin_number=0,
> > >         controller=this>
> > >       in_pin1: Composition<MagicInterruptPin, pin_number=1,
> > >         controller=this>
> > >       out_pin: link<IRQ>
> >
> > No, there would be a single 'Pin' class that would be used for irqs.  There
> > is no need to subclass it.
> >
> > > Type: MagicInterruptController
> > >
> > >     Is-a: SystemBusDevice
> > >
> > >     Properties:
> > >       in_pin0: Target<IRQ>
> > >       in_pin1: Target<IRQ>
> > >       out_pin: Link<IRQ>
> >
> > So this is more like how it works today except s/Link/child/ and
> > s/Target/link/g.
>
> Isn't that the other way around?  The input pin owns the IRQ object, and the
> output pin is the board/machine/user configured reference to that object.

It just depends on your view of input vs. output.  Either way works out just as 
well.

> So the link is to an Interface, not a device?

Devices are Objects

All Objects have Types.  An interface is just another Type.  Types can inherit 
from other types.

All devices inherit from the DeviceState type.

A link has a Type associated with it.  The Type can be an interface, or it can 
be a stateful base class, or it can be a Type that isn't derived at all from 
Device (like a CharDriverState).

>  To support multiple instances
> of the same interface we use a closure-like proxy object?

If you needed to, but I think most of the time, that's not necessary.

Regards,

Anthony Liguori

> I guess it we assume the number of interface classes is relatively small that
> should work.
>
> Paul