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

[Anthony Liguori]

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

Regards,

Anthony Liguori