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

[Anthony Liguori <anthony@codemonkey.ws>]

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