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

Anthony Liguori
2011-02-14T20:53:26Z

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