Re: [Qemu-devel] [RFC] New device API

[Paul Brook <paul@codesourcery.com>]

> > +
> > +/* Register a new device type.  */
> > +DeviceType *qdev_register(const char *name, int size, qdev_initfn init,
> > +                          void *opaque)
> > +{
> > +    DeviceType *t;
> > +
> > +    assert(size >= sizeof(DeviceState));
> > +
> > +    t = qemu_mallocz(sizeof(DeviceType));
> > +    t->next = device_type_list;
> > +    device_type_list = t;
> > +    t->name = qemu_strdup(name);
> > +    t->size = size;
> > +    t->init = init;
> > +    t->opaque = opaque;
> > +
> > +    return t;
> > +}
>
> void virtio_blk_register(void)
> {
>     pci_qdev_register("virtio-blk", sizeof(VirtIOBlock), virtio_blk_init);
> }
>
> So you'd expect all "device types" to be registered by the time the
> actual machine initialization starts. Similar to modular device drivers
> in Linux.

They are.

>     bs = qdev_init_bdrv(&pci_dev->qdev, IF_VIRTIO);
>     s->vdev.get_config = virtio_blk_update_config;
>
> And eventually you'd move qdev_init_bdrv (and all BlockDriverState
> knowledge) to happen somewhere else other than device emulation code?

I'm not sure what you're getting at here. The whole point of qdev_init_bdrv is 
that it isolates devices from the block device configuration.

> > +/* This structure should not be accessed directly.  We declare it here
> > +   so that it can be embedded in individual device state structures.  */
> > +struct DeviceState
> > +{
> > +    const char *name;
> > +    DeviceType *type;
> > +    void *bus;
>
> I suppose parent_bus is more descriptive?.
>
> Here you would have a tree node, eventually.

I've deliberately kept the device API independent of the configuration 
structure. We need some awareness of bus-device interaction because this 
tends to define how the device interacts with the rest of the system. I don't 
want to force this onto a particular topology though.

> > +    int num_irq;
> > +    qemu_irq irqs[QDEV_MAX_IRQ];
> > +    qemu_irq *irqp[QDEV_MAX_IRQ];
> > +    int num_irq_sink;
> > +    qemu_irq *irq_sink;
>
> This is somewhat complicated. So you need irqp pointers and irqs array
> due to the way IRQ initialization is performed?

Yes. It allows single-pass device initialisation. My earlier prototype had 
multi-pass initialization, which gets confusing and often results in quire a 
bit of duplication. You end up with a first pass saying "I'm going to need X, 
Y and Z", then a second pass that actually retrieves X, Y and Z.


> > +DeviceType *qdev_register(const char *name, int size, qdev_initfn init,
> > +                          void *opaque);
>
> Don't you want to cover savevm/restore callbacks at this level too?
>
> Device destruction surely (for hotunplug). Passing a structure with
> callbacks would be nicer.

I've tried to restrict this to the minimum information needed to instantiate 
the device. Everything else can be sorted out during initialisation. This is 
related to the single-pass initialisation I mentioned above.

> > +/* Register device properties.  */
> > +void qdev_init_mmio(DeviceState *dev, target_phys_addr_t size, int
> > iofunc); +void qdev_init_mmio_cb(DeviceState *dev, target_phys_addr_t
> > size, +                       mmio_mapfunc cb);
> > +void qdev_init_irq(DeviceState *dev, qemu_irq *p);
> > +void qdev_pass_irq(DeviceState *dev, DeviceState *target);
>
> Can you explain how init_irq_sink/pass_irq are supposed to work?

There are two end to each virtual IRQ line. The source (device that raises the 
IRQ) and the sink (device that reacts to the IRQ).

qdev_init_irq sets up an IRQ source. As mentioned above the irq object itself 
is populated after device intiialization (but before the machine starts).

qdev_init_irq_sink sets up a set of IRQ sinks.

qdev_pass_irq is a hack that passes through IRQ sources from a different 
device on this device. It's equivalent to:

proxy_irq_handler(dev, n, level)
{
  qemu_set_irq(dev->irq[n], level)
}

my_device_init(dev, target)
{
  qemu_irq *proxy;
  proxy = qemu_allocate_irqs(dev, proxy_irq_handler, target->num_irq);
  for (i = 0; i < target->num_irq; i++) 
    qdev_connect_irq(target, proxy[i]);
    qdev_init_irq(dev, &dev->irq[i]);
  }
}


> Markus should have more substantial comments (he's on vacation
> this week). The tree driven machine initialization introduces some
> complications (such as parent_bus->child device dependencies) while
> handling a lot of the problems you are ignoring (eg the ->config method
> is responsible for linking host device -> emulated device information,
> etc).

I still don't believe multipass device initialization/configuration is 
necessary. I have kept tree driven initialization in mind when implementing 
the device API.

Paul