Re: [Qemu-devel] [RFC][PATCH 0/21] QEMU Object Model

[Paolo Bonzini]

On 07/26/2011 09:23 PM, Anthony Liguori wrote:
> On 07/26/2011 01:26 PM, Paolo Bonzini wrote:
> > On 07/26/2011 05:34 PM, Anthony Liguori wrote:
> > You could just as well say that real life works like this:
> >
> > class PciSocket {
> > PciBus *pci_bus;
> > uint8_t *config;
> > uint8_t *cmask;
> > uint8_t *wmask;
> > uint8_t *w1cmask;
> > uint8_t *used;
> > uint32_t devfn;
> > char name[64];
> > PCIIORegion io_regions[PCI_NUM_REGIONS];
> > ...
> >
> > };
> >
> > class IsaSocket {
> > IsaBus *isa_bus;
> > uint32_t isairq[2]; // not sure this is a good idea, just
> > int nirqs; // mimicking qdev
> > uint16_t ioports[32];// statically assigned unlike PCI bars
> > int nioports;
> > }
> >
> > class MyWeirdDevice : public MyBaseDevice {
> > PciSocket pci;
> > IsaSocket isa;
> > };
>
> Hrm, I'm not sure I buy that entirely. I think it would be:
>
> class MyWeirdPciView : public PciDevice {
> PciBus *bus;
> MyWeirdDevice *dev;
> };
>
> class MyWeirdIsaView : public IsaDevice {
> IsaBus *bus;
> MyWeirdDevice *dev;
> };
>
> class MyWeirdDevice : public MyBaseDevice {
> MyWeirdPciView pci;
> MyWeirdIsaView isa;
> }
>
> The views are basically bridges between PCI/ISA and an internal
> interface (MyWeirdDevice).

That's yet another possibility.  There are two difference with what I 
had written:

1) the back-pointer from MyWeird*View to MyWeirdDevice replaced by 
container_of.  That's cosmetic;

2) I'm not considering the sockets to be separate devices.  That's the 
difference between "proxying PCI to another device" (wrong, hardware 
does not do that) and "isolating PCI knowledge within a sub-object of 
the device" (just an encapsulation choice, hardware is irrelevant).

> I don't think the proxy design pattern is the right thing to use.
> 95% of the time, the device is intrinsically a PCI device.

It's not a proxy.  It's replacing inheritance with containment to get 
the flexibility of data MI without the complexity of diamond hierarchies.

> The other 5% of the
> time, the device has a well defined interface, and then there is
> effectively a PCI bridge. But that PCI bridge isn't generic, it's
> specific to that custom interface.

Yes, that can be represented by composition if you wish (an 
ISASerialState containing an 8250 is the canonical example; the 8139 
example below is another).

> > > There's a pin in the IDE cable that determines master or slave depending
> > > on whether it's raised high.
> >
> > Yes, that's the "newer" way. There used to be jumpers to choose between
> > master, slave and cable-select.
>
> That jumper raises the bit on the wire.

Ah ok, I was confusing it with the cable-select wire.  My point was that 
you can choose the jumper representation (low-level) or the cable-select 
representation (high-level, actually matches modern hardware, even 
better from the user POV).  One is easier to manage and better in all 
aspects, but both make sense.  It's irrelevant to this discussion anyway.

> > > > > Interfaces are the right way to do this. Getting MI right is fairly
> > > > > hard
> > > >
> > > > But we don't need is-a, we need has-a. Multiple is-a is harder than
> > > > single is-a. Multiple has-a does not add any complication.
> > >
> > > Yeah, that's what plug properties are for :-)
> >
> > I agree, but at the cost of pointer chasing and making it harder to
> > implement get_device_for_socket for buses that need it (in the above
> > sketch it can be a simple container_of).
>
> Can we be a bit more concrete as I'm having a hard time following your
> logic. You're assuming a generic PciSocket class, right? I think that's
> not the right approach, as an example:
>
> class Rtl8139PciBridge : public PciDevice
> {
> Rtl8139 rtldev;
> };
>
> class Rtl8139IsaBridge : public IsaDevice
> {
> Rtl8139 rtldev;
> };
>
> With Avi's new memory API, we'd have:
>
> class Rtl8139 : public Device
> {
> MemoryRegion region[2];
> Pin irq;
> };
>
> And then the construction code for Rtl8139PciBridge would register the
> regions as bars, and connect the PCI lnk to rtldev.irq.
>
> The ISA code would do something similar.

Yes, this looks nice (modulo s/Rtl8139/Rtl8139 */).  But it is not that 
much more flexible than qdev 1.0.

You're right that for the case of two parents above we were looking at a 
contrived example.  The Goldfish platform provides a more interesting 
one.  There you have an interrupt controller and a bus enumerator 
device.  Most devices are connected to both, but conflating them is 
wrong for many reasons;

1) the bus enumerator itself uses an interrupt (raised on hotplug);

2) you can enumerate devices that do not have an interrupt line, and you 
shouldn't need to connect such a device to an interrupt controller;

3) the interrupt controller and bus enumerator use two separate MMIO areas;

4) in principle, other devices could use the interrupt controller (which 
is the only component connected to the CPU's interrupt line) without 
being enumerated.

5) A device with two interrupts has two "interrupt interfaces" and only 
one "enumerator interface".

6) The enumerator interface does not have bus semantics.  The enumerator 
also enumerates itself so it would act as both the bus host and a device 
on the bus.

Composition then lets you use something like this:

    class GoldfishPIC : Device {
        Pin parent;
        GoldfishInterruptPin *children[32];
        Pin (*init_socket_for_irq_num) (GoldfishInterruptPin *, int);
    };

    class GoldfishInterruptPin {
        GoldfishPIC *pic;
        Pin irqnum;
    };

    class GoldfishEnumerator : Device {
        GoldfishInterruptPin        irq;
        GoldfishBusDeviceInfo       info;
        List<GoldfishBusDeviceInfo> allDevices;
        ...
    };

    class GoldfishBusDeviceInfo {
        GoldfishEnumerator *parent;
        char *name;
        int id;
        ram_addr_t mmio;
        int mmio_size;
        int irq_base;
        int irq_count;
    };

    class GoldfishTTY : Device {
        GoldfishInterruptPin irq;
        GoldfishBusDeviceInfo info;
        CharDev *chardev;
    };

Here you do not need a bus to mediate the access between the TTY device 
and its parents, it would only introduce unnecessary complication 
(boilerplate and pointer chasing).  It also would not match hardware at 
all in the case of the enumerator.

> > > Indeed. I think that it's a no brainer for the backends and that's why
> > > I'm starting there.
> >
> > I don't think it's a no brainer. It's simply much easier, but right now
> > it is also a solution in search of a problem (if all you want is dynamic
> > creation of character devices, you could do that without a generic
> > object model).
>
> And that solves the problem yet again for one layer. But what about
> block, fsdev, and DisplayState?  We can keep reinventing the wheel over
> and over again in slightly different ways or we can try to do something
> that will work for more things.
>
> We need more commonality in QEMU. Things are hard to follow because
> every subsystem is an island.

I am not saying you're wrong.  I am saying it is very hard to do it on a 
mature (no matter how messy) code base.

> We need lots of new transitions, we need to strive to make things
> better. But we need to do things in such a way that:
>
> (1) we have a good idea of what we're going to end up with at the end of
> the day
>
> (2) there is incremental value being added to QEMU at every step of the way
>
> This cannot be done by simply hacking some bits here and there. It
> requires design, planning, and flag days when appropriate.

Agreed.  The problem I have with QOM is (2).  I am not sure we do get 
incremental value at every step of the way.  We do get incremental value 
in the char layer, but we also get additional complexity until the 
transition is over.

Paolo