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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux