From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51159 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pp5QG-0003lP-GA for qemu-devel@nongnu.org; Mon, 14 Feb 2011 15:53:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pp5QE-0008H2-FE for qemu-devel@nongnu.org; Mon, 14 Feb 2011 15:53:28 -0500 Received: from mail-qy0-f180.google.com ([209.85.216.180]:40778) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pp5QE-0008Gm-A9 for qemu-devel@nongnu.org; Mon, 14 Feb 2011 15:53:26 -0500 Received: by qyk29 with SMTP id 29so4379158qyk.4 for ; Mon, 14 Feb 2011 12:53:25 -0800 (PST) Message-ID: <4D599639.2030508@codemonkey.ws> Date: Mon, 14 Feb 2011 14:53:13 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] KVM call minutes for Feb 8 References: <4D52F20A.7070009@codemonkey.ws> <4D539800.3070802@codemonkey.ws> <20110210090748.GD673@redhat.com> <4D53BD22.1040800@redhat.com> <20110210111354.GA21681@redhat.com> <4D53DF42.4030700@codemonkey.ws> <20110210132730.GB24525@redhat.com> <4D53F06C.9090500@codemonkey.ws> <20110210142044.GD24525@redhat.com> <4D540CC5.2@codemonkey.ws> <4D57F96B.7010004@codemonkey.ws> <4D583793.10409@codemonkey.ws> <4D585E3C.9010404@codemonkey.ws> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Chris Wright , Gleb Natapov , kvm@vger.kernel.org, Markus Armbruster , qemu-devel@nongnu.org, Avi Kivity 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