Re: [Qemu-devel] KVM call minutes for Feb 8

[Blue Swirl]

On Mon, Feb 14, 2011 at 10:53 PM, Anthony Liguori <address@hidden> wrote:
> On 02/14/2011 11:31 AM, Blue Swirl wrote:
>>
>> I don't understand. The caller just does
>> if (isa_serial_init()) {
>>   error();
>> }
>> or
>> if (serial_init()) {
>>   error();
>> }
>>
>> If you mean inside isa_serial_init() vs. serial_init(), that may be
>> true since isa_serial_init has to check for qdev failures, but the to
>> the caller both should be identical.
>>
>
> The problem with qdev is there's too much boiler plate code which makes it
> hard to give examples :-)  Here's precisely what I'm talking about:
>
> static int serial_isa_initfn(ISADevice *dev)
> {
>    static int index;
>    ISASerialState *isa = DO_UPCAST(ISASerialState, dev, dev);
>    SerialState *s = &isa->state;
>
>    if (isa->index == -1)
>        isa->index = index;
>    if (isa->index >= MAX_SERIAL_PORTS)
>        return -1;
>    if (isa->iobase == -1)
>        isa->iobase = isa_serial_io[isa->index];
>    if (isa->isairq == -1)
>        isa->isairq = isa_serial_irq[isa->index];
>    index++;
>
>    s->baudbase = 115200;
>    isa_init_irq(dev, &s->irq, isa->isairq);
>    serial_init_core(s);
>    qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3);
>
>    register_ioport_write(isa->iobase, 8, 1, serial_ioport_write, s);
>    register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, s);
>    isa_init_ioport_range(dev, isa->iobase, 8);
>    return 0;
> }
>
> SerialState *serial_init(int base, qemu_irq irq, int baudbase,
>                         CharDriverState *chr)
> {
>    SerialState *s;
>
>    s = qemu_mallocz(sizeof(SerialState));
>
>    s->irq = irq;
>    s->baudbase = baudbase;
>    s->chr = chr;
>    serial_init_core(s);
>
>    vmstate_register(NULL, base, &vmstate_serial, s);
>
>    register_ioport_write(base, 8, 1, serial_ioport_write, s);
>    register_ioport_read(base, 8, 1, serial_ioport_read, s);
>    return s;
> }
>
> static ISADeviceInfo serial_isa_info = {
>    .qdev.name  = "isa-serial",
>    .qdev.size  = sizeof(ISASerialState),
>    .qdev.vmsd  = &vmstate_isa_serial,
>    .init       = serial_isa_initfn,
>    .qdev.props = (Property[]) {
>        DEFINE_PROP_UINT32("index", ISASerialState, index,   -1),
>        DEFINE_PROP_HEX32("iobase", ISASerialState, iobase,  -1),
>        DEFINE_PROP_UINT32("irq",   ISASerialState, isairq,  -1),
>        DEFINE_PROP_CHR("chardev",  ISASerialState, state.chr),
>        DEFINE_PROP_END_OF_LIST(),
>    },
> };
>
> static void serial_register_devices(void)
> {
>    isa_qdev_register(&serial_isa_info);
> }
>
> device_init(serial_register_devices)
>
>
> To create a device, I need to do:
>
> {
>     ISADevice *dev;
>
>     dev = isa_create("isa-serial");
>     if (dev == NULL) {
>          return error;
>     }
>     if (qdev_set_uint32(&dev->qdev, "index", index)) {
>          goto err;
>     }
>     if (qdev_set_uint32(&dev->qdev, "iobase", iobase)) {
>          goto err;
>     }
>     if (qdev_set_uint32(&dev->qdev, "irq", irq)) {
>         goto err;
>     }
>     if (qdev_set_chr(&dev->qdev, "chardev", chr)) {
>         goto err;
>     }
>     if (qdev_init(&dev->qdev)) {
>         goto err;
>     }
>     return 0;
> err:
>     qdev_destroy(&dev->qdev);
>     return -1;
> }
>
> This is simply not a reasonable API to use to create devices.

This can be wrapped in a static inline function, with similar
signature to what you propose:

static inline ISADevice *serial_init(int base, qemu_irq irq, int
baudbase, CharDriverState *chr);

>  There are two
> ways we can make this more managable.  The first is gobject-style vararg
> constructor coupled with a type safe wrapper.  So...
>
> ISASerialDevice *isa_serial_device_new(uint32_t index, uint32_t iobase,
> uint32_t irq, CharDriverState *chr)
> {
>      return isa_device_create_va("isa-seral", "index", index, "iobase",
> iobase, "irq", irq, "chardev", chr, NULL);
> }
>
> Now this can be used in a reasonable fashion.  However, we can do even
> better if we change the way qdev is done.   Consider the following:
>
> SerialState *serial_init(int base, qemu_irq irq, int baudbase,
>                         CharDriverState *chr)
> {
>    SerialState *s;
>
>    s = qemu_mallocz(sizeof(SerialState));
>
>    s->irq = irq;
>    s->baudbase = baudbase;
>    s->chr = chr;
>    serial_init_core(s);
>
>    vmstate_register(NULL, base, &vmstate_serial, s);
>
>    register_ioport_write(base, 8, 1, serial_ioport_write, s);
>    register_ioport_read(base, 8, 1, serial_ioport_read, s);
>    return s;
> }
>
> ISASerialDevice *isa_serial_new(uint32_t index, uint32_t iobase, uint32_t
> irq, CharDriverState *chr)
> {
>    static int index;
>    ISADevice *dev = isa_create("isa-serial");
>    ISASerialState *isa = DO_UPCAST(ISASerialState, dev, dev);
>    SerialState *s = &isa->state;
>
>    if (index == -1)
>        index = index;
>    if (index >= MAX_SERIAL_PORTS)
>        return NULL;
>    if (iobase == -1)
>        iobase = isa_serial_io[index];
>    if (isairq == -1)
>        isairq = isa_serial_irq[index];
>    index++;
>
>    s->baudbase = 115200;
>    isa_init_irq(dev, &s->irq, isairq);
>    serial_init_core(s);
>    qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3);
>
>    register_ioport_write(isa->iobase, 8, 1, serial_ioport_write, s);
>    register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, s);
>    isa_init_ioport_range(dev, isa->iobase, 8);
>    return isa;
> }
>
> static ISADevice *isa_seral_init_qdev(QemuOpts *opts)
> {
>      uint32_t index = qemu_opt_get_uint32(opts, "index", -1);
>      uint32_t irq = qemu_opt_get_uint32(opts, "irq", -1);
>      uint32_t iobase = qemu_opt_get_uint32(opts, "iobase", -1);
>      CharDriverState *chr = qemu_opt_get_chr(opts, "chardev");
>
>      return isa_serial_new(index, irq, iobase, chr);
> }
>
> static void serial_register_devices(void)
> {
>    isa_qdev_register("isa-serial", isa_serial_init_qdev);
> }
>
> device_init(serial_register_devices)
>
> The advantage of this model is that we can totally ignore the factory
> interface for devices that we don't care about exposing to the user.  So the
> isa_seral_init_qdev part is totally optional.

I'd still like to have the inline wrapper over the factory interface,
probably with similar signature to isa_serial_new. Then there would be
two functions, one going through qdev and the other bypassing it. I
don't see how that would be useful.

The callers of the direct interface would force linkage between them
and so it would be impossible to build QEMU with that device. We don't
need that flexibility for every device though, but I don't see any
advantages for using the direct interface either.

Why shouldn't we want all devices to be exposed to the user? For
example, there are still devices which don't show up in 'info qtree',
which is a shame.