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

[Anthony Liguori]

On 07/26/2011 01:26 PM, Paolo Bonzini wrote:
> On 07/26/2011 05:34 PM, Anthony Liguori wrote:
> > > And as such it can add data members. But when a device is on two buses,
> > > you cannot have both of them adding data members. I know MI is hard to
> > > get right, and in fact I'm not proposing to do MI---not even interface
> > > inheritance. I don't want to have any base class but DeviceState.
> >
> > I could use a concrete example here, but here's how this would be
> > expressed:
> >
> > class MyWeirdDevice : public MyBaseDevice, implements PciDevice,
> > IsaDevice
> > {
> > PciBus *pci_bus;
> > IsaDevice *isa_bus;
> > };
> >
> > Which actually models hw pretty well. A device that exists on two busses
> > has to have two sockets that can accept the plugs from the busses
> > they're joining.
> >
> > It's pretty much exactly how it works in real life.
>
> 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).

I don't think having a generic PciSocket class that offloads PCI 
knowledge to another device is the right model (assuming that's why 
you're suggesting).  It's basically proxying PCI to another device.

> > > Once you have something like this for a device that bridges two buses,
> > > interfaces require a lot of boilerplate for useless getters/setters.
> >
> > Can you elaborate?
>
> If you store data (configuration space etc.) in the device, and the bus
> has to access it, you need getters/setters in the interface. Letting the
> bus hold an interior reference to the PciSocket (perhaps adding a single
> get_device_for_socket function to the PciSocketOps) solves the problem.

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.  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.

> > > > The same applies equally to IDE.
> > > >
> > > > ide->primary.master = disk1;
> > > > ide->secondary.master = cdrom;
> > >
> > > For IDE, an equally good model would be:
> > >
> > > ide->primary.add(disk1);
> > > disk1.masterSlave = MASTER;
> > > ide->secondary.add(cdrom);
> > > cdrom.masterSlave = MASTER;
> >
> > 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.

> > > > 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.

> > > > I think all of the requirements you've outlined are currently
> > > > handled in
> > > > QOM.
> > >
> > > They more than likely are. The question is whether they're handled in
> > > the most programmer-efficient manner, and whether the advantages of a
> > > single grand unified object model for host and guest devices is worth
> > > the effort.
> >
> > 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.

> If starting from a blank slate, I would be much more enthusiastic. But
> all that QEMU does _not_ need is yet another incomplete transition.

This is an oversimplification and an argument that needs to end.

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.

If you look at QAPI, we only merged the bits that had value for a 
specific feature (guest agent).  The remaining bits (QMP conversion), 
has been completely, but will be merged all at once.  The mistake we can 
make in an effort like this is introduce a partial conversion that has 
no value in and of itself.

Because there's little incentive to help with partial conversions if 
there isn't an immediate return on investment.  That's why I've stuck 
with focusing on the char layer to start with.  It's value that stands 
on its own.

That's the thing we need to focus on.  The problem with our past 
attempts at making large changes is that we didn't deconstruct it in 
such a way that each phase added clear value.

Regards,

Anthony Liguori

> Paolo