Re: [Qemu-devel] qdev for programmers writeup

[Paolo Bonzini]

On 07/11/2011 12:46 PM, Peter Maydell wrote:
> One point I'd like clarification on is when you need to invent
> a new bus type.

As rarely as possible, but as often as necessary? :P

New buses limit reusability of the models, but you need one whenever the 
existing buses do not express how two devices interact.

> Sometimes it's pretty obvious because there's
> a real hardware bus type there (PCI, SCSI) that you're modelling.
> It's the edge cases I find confusing -- for instance, do we need
> a qbus for the connection between an SD card controller and the
> SD card model (hw/sd.c) ? There's a well defined pluggable interface
> between those two parts but there's only ever one SD card so a "bus"
> would be a bit odd.

Perhaps not and you can use containment.  A very similar case is the 
16550 device, which has both an ISA (ioport-based) interface and a 
generic memory-mapped interface.  Anthony at some point argued that if 
serial_mm_init was qdev-ified, one should describe SerialState as a 
separate 16550 device, and then put a bus between {isa,mm}-serial and 
thisg 16550 device.  But perhaps including a SerialState struct into 
both ISASerialState and the (hypothetical) MMSerialState is enough.

I think you have to look at the properties of the child device and the 
interfaces between the devices.  For SerialState, the properties would 
be chardev and baudbase, and just a qemu_irq between the parent and 
child.  For SDState, the only property of the SDState would be the 
blockdev, and again a couple GPIO pins between the two.

It probably would be feasible to separate the two.  But then without a 
clean plan about hiding such internal devices, it is likely a useless 
complication for the user to see the existence of the SerialState and 
SDState.

> > Together with such
> > a bus type there should be a single root board-specific device that is
> > attached to SysBus.  An interrupt controller is usually a good candidate
> > for this because it takes qemu_irqs from the outside, and can make good
> > use of the specificities of SysBus.
>
> ...and this bit I don't understand. Why is SysBus a bad thing?

1) because SysBus devices are in general not accessible from the 
command-line or configuration files.

2) because SysBus hardcodes in the source code some things that ought to 
be device properties, for example the MMIO base address.  The presence 
of MMIO in sysbus_create_simple/sysbus_create_varargs is totally 
unnecessary IMHO.

> It generally seems to me to be the right way to represent a bit of
> hardware which is fundamentally providing a memory mapped interface
> plus some GPIO lines. If you make your board use sysbus then it's
> easy to just plug in any existing sysbus device model qemu already
> has; if every board has its own bus type instead then this reuse
> just becomes unnecessarily harder, it seems to me.

That's true, but the only way to plug in those device models would be 
with C code.  You cannot just play with -device to reconfigure them. 
It's not like SysBus has any problem; but it is right now the only 
choice you have if you want a reusable model, and that means that 
reusability can only be done at the cost of rebuilding QEMU.

For example, one reusable device is gpio_i2c.  However, I cannot simply 
take it and add it to a new board.  I need to add glue like this:

    /* dev is my GPIO device.  */
    i2c_dev = sysbus_create_simple("gpio_i2c", -1, NULL);
    qdev_connect_gpio_out(i2c_dev, 0, qdev_get_gpio_in(dev, 29));
    qdev_connect_gpio_out(dev, 3, qdev_get_gpio_in(i2c_dev, 0));
    qdev_connect_gpio_out(dev, 4, qdev_get_gpio_in(i2c_dev, 1));

and recompile QEMU.

In fact, perhaps qdev_{connect_gpio_out,get_gpio_in} should never have 
been public.  Imagine we added to qdev GPIO properties and we used them 
like this in gpio_i2c:

    /* gpio_out=N means "connect my 0th output pin to the parent's
       N-th input pin.  */
    DEFINE_PROP_GPIO_OUT("gpio_out", 0),

    /* gpio_in=N means "connect my 0th input pin to the parent's
       N-th output pin.  */
    DEFINE_PROP_GPIO_IN("gpio_in", 0),
    DEFINE_PROP_GPIO_IN("gpio_clk", 1)

Then we define a GPIOBus that is really a bare-bones BusState, with no 
MMIO and nothing.  However, GPIO chips would expose one such bus, and a 
lot of reusable components could be moved from SysBus to GPIOBus... and 
get -device configuration at once!  With this in place you can do:

    -device gpio_i2c,gpio_out=29,gpio_in=3,gpio_clk=4

or in a configuration file:

    [device "gpio_i2c"]
        gpio_out = 29
        gpio_in = 3
        gpio_clk = 4

or if you really have to do it in C:

    dev = qdev_create(&gpiobus->bus, "gpio_i2c");
    qdev_set_prop_set_gpio_out("gpio_out", 29);
    qdev_set_prop_set_gpio_in("gpio_in", 3);
    qdev_set_prop_set_gpio_in("gpio_clk", 4);
    qdev_init_nofail(dev);

Even the C code would already be an improvement, because the client code 
has no idea of the pin numbers of gpio_i2c.

Note that gpio_i2c is already a well-defined device, and it uses only a 
bunch of qdev gpio pins.  In a more complex case when you use the sysbus 
IRQ mechanism, sharing is going to be even harder (and this is what I 
had in mind when proposing that the interrupt controller have its own bus).

> Also having the interrupt controller be the board specific device
> which you attach to sysbus seems totally wrong to me. This doesn't
> match hardware at all -- the interrupt controller deals with
> interrupt lines and isn't in the path for memory transactions at
> all.

Well, it is clear that buses should be modelled after the way data 
flows.  But what is data?  If data is "what is being written", buses 
should be modelled after the way memory transactions flow.  If data is 
"what is being made available", buses are modelled more "after the way 
interrupts flow.  GPIO is a strange thing in the middle. :)

Unfortunately there is only one path you have to choose, which is 
perhaps the strongest limitation of qdev (unless you're working with PCI 
where the bus provides both MMIO and interrupts).  MMIO assignments for 
embedded boards are usually simple enough that I found an 
interrupt-driven design (SysBus->PIC->InterruptBus->GPIO->GPIOBus->...) 
clearer and easier to explain; that biased my writing.

Paolo