From mboxrd@z Thu Jan 1 00:00:00 1970 From: Blue Swirl Subject: Re: [Qemu-devel] KVM call minutes for Feb 8 Date: Mon, 14 Feb 2011 23:25:36 +0200 Message-ID: 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> <4D599639.2030508@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Chris Wright , Gleb Natapov , kvm@vger.kernel.org, qemu-devel@nongnu.org, Markus Armbruster , Avi Kivity To: Anthony Liguori Return-path: Received: from mail-yw0-f46.google.com ([209.85.213.46]:56247 "EHLO mail-yw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750844Ab1BNVZ5 convert rfc822-to-8bit (ORCPT ); Mon, 14 Feb 2011 16:25:57 -0500 Received: by ywo7 with SMTP id 7so2263090ywo.19 for ; Mon, 14 Feb 2011 13:25:56 -0800 (PST) In-Reply-To: <4D599639.2030508@codemonkey.ws> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Feb 14, 2011 at 10:53 PM, Anthony Liguori wrote: > On 02/14/2011 11:31 AM, Blue Swirl wrote: >> >> I don't understand. The caller just does >> if (isa_serial_init()) { >> =C2=A0 error(); >> } >> or >> if (serial_init()) { >> =C2=A0 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 t= o >> the caller both should be identical. >> > > The problem with qdev is there's too much boiler plate code which mak= es it > hard to give examples :-) =C2=A0Here's precisely what I'm talking abo= ut: > > static int serial_isa_initfn(ISADevice *dev) > { > =C2=A0 =C2=A0static int index; > =C2=A0 =C2=A0ISASerialState *isa =3D DO_UPCAST(ISASerialState, dev, d= ev); > =C2=A0 =C2=A0SerialState *s =3D &isa->state; > > =C2=A0 =C2=A0if (isa->index =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0isa->index =3D index; > =C2=A0 =C2=A0if (isa->index >=3D MAX_SERIAL_PORTS) > =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; > =C2=A0 =C2=A0if (isa->iobase =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0isa->iobase =3D isa_serial_io[isa->index]; > =C2=A0 =C2=A0if (isa->isairq =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0isa->isairq =3D isa_serial_irq[isa->index]= ; > =C2=A0 =C2=A0index++; > > =C2=A0 =C2=A0s->baudbase =3D 115200; > =C2=A0 =C2=A0isa_init_irq(dev, &s->irq, isa->isairq); > =C2=A0 =C2=A0serial_init_core(s); > =C2=A0 =C2=A0qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3); > > =C2=A0 =C2=A0register_ioport_write(isa->iobase, 8, 1, serial_ioport_w= rite, s); > =C2=A0 =C2=A0register_ioport_read(isa->iobase, 8, 1, serial_ioport_re= ad, s); > =C2=A0 =C2=A0isa_init_ioport_range(dev, isa->iobase, 8); > =C2=A0 =C2=A0return 0; > } > > SerialState *serial_init(int base, qemu_irq irq, int baudbase, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 CharDriverState *chr) > { > =C2=A0 =C2=A0SerialState *s; > > =C2=A0 =C2=A0s =3D qemu_mallocz(sizeof(SerialState)); > > =C2=A0 =C2=A0s->irq =3D irq; > =C2=A0 =C2=A0s->baudbase =3D baudbase; > =C2=A0 =C2=A0s->chr =3D chr; > =C2=A0 =C2=A0serial_init_core(s); > > =C2=A0 =C2=A0vmstate_register(NULL, base, &vmstate_serial, s); > > =C2=A0 =C2=A0register_ioport_write(base, 8, 1, serial_ioport_write, s= ); > =C2=A0 =C2=A0register_ioport_read(base, 8, 1, serial_ioport_read, s); > =C2=A0 =C2=A0return s; > } > > static ISADeviceInfo serial_isa_info =3D { > =C2=A0 =C2=A0.qdev.name =C2=A0=3D "isa-serial", > =C2=A0 =C2=A0.qdev.size =C2=A0=3D sizeof(ISASerialState), > =C2=A0 =C2=A0.qdev.vmsd =C2=A0=3D &vmstate_isa_serial, > =C2=A0 =C2=A0.init =C2=A0 =C2=A0 =C2=A0 =3D serial_isa_initfn, > =C2=A0 =C2=A0.qdev.props =3D (Property[]) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_UINT32("index", ISASerialState= , index, =C2=A0 -1), > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_HEX32("iobase", ISASerialState= , iobase, =C2=A0-1), > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_UINT32("irq", =C2=A0 ISASerial= State, isairq, =C2=A0-1), > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_CHR("chardev", =C2=A0ISASerial= State, state.chr), > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_END_OF_LIST(), > =C2=A0 =C2=A0}, > }; > > static void serial_register_devices(void) > { > =C2=A0 =C2=A0isa_qdev_register(&serial_isa_info); > } > > device_init(serial_register_devices) > > > To create a device, I need to do: > > { > =C2=A0 =C2=A0 ISADevice *dev; > > =C2=A0 =C2=A0 dev =3D isa_create("isa-serial"); > =C2=A0 =C2=A0 if (dev =3D=3D NULL) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return error; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_set_uint32(&dev->qdev, "index", index)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_set_uint32(&dev->qdev, "iobase", iobase)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_set_uint32(&dev->qdev, "irq", irq)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_set_chr(&dev->qdev, "chardev", chr)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_init(&dev->qdev)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 return 0; > err: > =C2=A0 =C2=A0 qdev_destroy(&dev->qdev); > =C2=A0 =C2=A0 return -1; > } > > This is simply not a reasonable API to use to create devices. This can be wrapped in a static inline function, with similar signature to what you propose: static inline ISADevice *serial_init(int base, qemu_irq irq, int baudbase, CharDriverState *chr); > =C2=A0There are two > ways we can make this more managable. =C2=A0The first is gobject-styl= e vararg > constructor coupled with a type safe wrapper. =C2=A0So... > > ISASerialDevice *isa_serial_device_new(uint32_t index, uint32_t iobas= e, > uint32_t irq, CharDriverState *chr) > { > =C2=A0 =C2=A0 =C2=A0return isa_device_create_va("isa-seral", "index",= index, "iobase", > iobase, "irq", irq, "chardev", chr, NULL); > } > > Now this can be used in a reasonable fashion. =C2=A0However, we can d= o even > better if we change the way qdev is done. =C2=A0 Consider the followi= ng: > > SerialState *serial_init(int base, qemu_irq irq, int baudbase, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 CharDriverState *chr) > { > =C2=A0 =C2=A0SerialState *s; > > =C2=A0 =C2=A0s =3D qemu_mallocz(sizeof(SerialState)); > > =C2=A0 =C2=A0s->irq =3D irq; > =C2=A0 =C2=A0s->baudbase =3D baudbase; > =C2=A0 =C2=A0s->chr =3D chr; > =C2=A0 =C2=A0serial_init_core(s); > > =C2=A0 =C2=A0vmstate_register(NULL, base, &vmstate_serial, s); > > =C2=A0 =C2=A0register_ioport_write(base, 8, 1, serial_ioport_write, s= ); > =C2=A0 =C2=A0register_ioport_read(base, 8, 1, serial_ioport_read, s); > =C2=A0 =C2=A0return s; > } > > ISASerialDevice *isa_serial_new(uint32_t index, uint32_t iobase, uint= 32_t > irq, CharDriverState *chr) > { > =C2=A0 =C2=A0static int index; > =C2=A0 =C2=A0ISADevice *dev =3D isa_create("isa-serial"); > =C2=A0 =C2=A0ISASerialState *isa =3D DO_UPCAST(ISASerialState, dev, d= ev); > =C2=A0 =C2=A0SerialState *s =3D &isa->state; > > =C2=A0 =C2=A0if (index =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0index =3D index; > =C2=A0 =C2=A0if (index >=3D MAX_SERIAL_PORTS) > =C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL; > =C2=A0 =C2=A0if (iobase =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0iobase =3D isa_serial_io[index]; > =C2=A0 =C2=A0if (isairq =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0isairq =3D isa_serial_irq[index]; > =C2=A0 =C2=A0index++; > > =C2=A0 =C2=A0s->baudbase =3D 115200; > =C2=A0 =C2=A0isa_init_irq(dev, &s->irq, isairq); > =C2=A0 =C2=A0serial_init_core(s); > =C2=A0 =C2=A0qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3); > > =C2=A0 =C2=A0register_ioport_write(isa->iobase, 8, 1, serial_ioport_w= rite, s); > =C2=A0 =C2=A0register_ioport_read(isa->iobase, 8, 1, serial_ioport_re= ad, s); > =C2=A0 =C2=A0isa_init_ioport_range(dev, isa->iobase, 8); > =C2=A0 =C2=A0return isa; > } > > static ISADevice *isa_seral_init_qdev(QemuOpts *opts) > { > =C2=A0 =C2=A0 =C2=A0uint32_t index =3D qemu_opt_get_uint32(opts, "ind= ex", -1); > =C2=A0 =C2=A0 =C2=A0uint32_t irq =3D qemu_opt_get_uint32(opts, "irq",= -1); > =C2=A0 =C2=A0 =C2=A0uint32_t iobase =3D qemu_opt_get_uint32(opts, "io= base", -1); > =C2=A0 =C2=A0 =C2=A0CharDriverState *chr =3D qemu_opt_get_chr(opts, "= chardev"); > > =C2=A0 =C2=A0 =C2=A0return isa_serial_new(index, irq, iobase, chr); > } > > static void serial_register_devices(void) > { > =C2=A0 =C2=A0isa_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. = =C2=A0So the > isa_seral_init_qdev part is totally optional. I'd still like to have the inline wrapper over the factory interface, probably with similar signature to isa_serial_new. Then there would be two functions, one going through qdev and the other bypassing it. I don't see how that would be useful. The callers of the direct interface would force linkage between them and so it would be impossible to build QEMU with that device. We don't need that flexibility for every device though, but I don't see any advantages for using the direct interface either. Why shouldn't we want all devices to be exposed to the user? For example, there are still devices which don't show up in 'info qtree', which is a shame. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=54114 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pp5wK-0002se-9R for qemu-devel@nongnu.org; Mon, 14 Feb 2011 16:26:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pp5vh-0005xf-EP for qemu-devel@nongnu.org; Mon, 14 Feb 2011 16:26:36 -0500 Received: from mail-gy0-f173.google.com ([209.85.160.173]:60425) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pp5vh-0005xb-Aq for qemu-devel@nongnu.org; Mon, 14 Feb 2011 16:25:57 -0500 Received: by gye5 with SMTP id 5so2501403gye.4 for ; Mon, 14 Feb 2011 13:25:56 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <4D599639.2030508@codemonkey.ws> 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> <4D599639.2030508@codemonkey.ws> From: Blue Swirl Date: Mon, 14 Feb 2011 23:25:36 +0200 Message-ID: Subject: Re: [Qemu-devel] KVM call minutes for Feb 8 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Chris Wright , Gleb Natapov , kvm@vger.kernel.org, Markus Armbruster , qemu-devel@nongnu.org, Avi Kivity On Mon, Feb 14, 2011 at 10:53 PM, Anthony Liguori w= rote: > On 02/14/2011 11:31 AM, Blue Swirl wrote: >> >> I don't understand. The caller just does >> if (isa_serial_init()) { >> =C2=A0 error(); >> } >> or >> if (serial_init()) { >> =C2=A0 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 i= t > hard to give examples :-) =C2=A0Here's precisely what I'm talking about: > > static int serial_isa_initfn(ISADevice *dev) > { > =C2=A0 =C2=A0static int index; > =C2=A0 =C2=A0ISASerialState *isa =3D DO_UPCAST(ISASerialState, dev, dev); > =C2=A0 =C2=A0SerialState *s =3D &isa->state; > > =C2=A0 =C2=A0if (isa->index =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0isa->index =3D index; > =C2=A0 =C2=A0if (isa->index >=3D MAX_SERIAL_PORTS) > =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; > =C2=A0 =C2=A0if (isa->iobase =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0isa->iobase =3D isa_serial_io[isa->index]; > =C2=A0 =C2=A0if (isa->isairq =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0isa->isairq =3D isa_serial_irq[isa->index]; > =C2=A0 =C2=A0index++; > > =C2=A0 =C2=A0s->baudbase =3D 115200; > =C2=A0 =C2=A0isa_init_irq(dev, &s->irq, isa->isairq); > =C2=A0 =C2=A0serial_init_core(s); > =C2=A0 =C2=A0qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3); > > =C2=A0 =C2=A0register_ioport_write(isa->iobase, 8, 1, serial_ioport_write= , s); > =C2=A0 =C2=A0register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, = s); > =C2=A0 =C2=A0isa_init_ioport_range(dev, isa->iobase, 8); > =C2=A0 =C2=A0return 0; > } > > SerialState *serial_init(int base, qemu_irq irq, int baudbase, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 CharDriverState *chr) > { > =C2=A0 =C2=A0SerialState *s; > > =C2=A0 =C2=A0s =3D qemu_mallocz(sizeof(SerialState)); > > =C2=A0 =C2=A0s->irq =3D irq; > =C2=A0 =C2=A0s->baudbase =3D baudbase; > =C2=A0 =C2=A0s->chr =3D chr; > =C2=A0 =C2=A0serial_init_core(s); > > =C2=A0 =C2=A0vmstate_register(NULL, base, &vmstate_serial, s); > > =C2=A0 =C2=A0register_ioport_write(base, 8, 1, serial_ioport_write, s); > =C2=A0 =C2=A0register_ioport_read(base, 8, 1, serial_ioport_read, s); > =C2=A0 =C2=A0return s; > } > > static ISADeviceInfo serial_isa_info =3D { > =C2=A0 =C2=A0.qdev.name =C2=A0=3D "isa-serial", > =C2=A0 =C2=A0.qdev.size =C2=A0=3D sizeof(ISASerialState), > =C2=A0 =C2=A0.qdev.vmsd =C2=A0=3D &vmstate_isa_serial, > =C2=A0 =C2=A0.init =C2=A0 =C2=A0 =C2=A0 =3D serial_isa_initfn, > =C2=A0 =C2=A0.qdev.props =3D (Property[]) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_UINT32("index", ISASerialState, in= dex, =C2=A0 -1), > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_HEX32("iobase", ISASerialState, io= base, =C2=A0-1), > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_UINT32("irq", =C2=A0 ISASerialStat= e, isairq, =C2=A0-1), > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_CHR("chardev", =C2=A0ISASerialStat= e, state.chr), > =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_END_OF_LIST(), > =C2=A0 =C2=A0}, > }; > > static void serial_register_devices(void) > { > =C2=A0 =C2=A0isa_qdev_register(&serial_isa_info); > } > > device_init(serial_register_devices) > > > To create a device, I need to do: > > { > =C2=A0 =C2=A0 ISADevice *dev; > > =C2=A0 =C2=A0 dev =3D isa_create("isa-serial"); > =C2=A0 =C2=A0 if (dev =3D=3D NULL) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return error; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_set_uint32(&dev->qdev, "index", index)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_set_uint32(&dev->qdev, "iobase", iobase)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_set_uint32(&dev->qdev, "irq", irq)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_set_chr(&dev->qdev, "chardev", chr)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 if (qdev_init(&dev->qdev)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto err; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 return 0; > err: > =C2=A0 =C2=A0 qdev_destroy(&dev->qdev); > =C2=A0 =C2=A0 return -1; > } > > This is simply not a reasonable API to use to create devices. This can be wrapped in a static inline function, with similar signature to what you propose: static inline ISADevice *serial_init(int base, qemu_irq irq, int baudbase, CharDriverState *chr); > =C2=A0There are two > ways we can make this more managable. =C2=A0The first is gobject-style va= rarg > constructor coupled with a type safe wrapper. =C2=A0So... > > ISASerialDevice *isa_serial_device_new(uint32_t index, uint32_t iobase, > uint32_t irq, CharDriverState *chr) > { > =C2=A0 =C2=A0 =C2=A0return isa_device_create_va("isa-seral", "index", ind= ex, "iobase", > iobase, "irq", irq, "chardev", chr, NULL); > } > > Now this can be used in a reasonable fashion. =C2=A0However, we can do ev= en > better if we change the way qdev is done. =C2=A0 Consider the following: > > SerialState *serial_init(int base, qemu_irq irq, int baudbase, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 CharDriverState *chr) > { > =C2=A0 =C2=A0SerialState *s; > > =C2=A0 =C2=A0s =3D qemu_mallocz(sizeof(SerialState)); > > =C2=A0 =C2=A0s->irq =3D irq; > =C2=A0 =C2=A0s->baudbase =3D baudbase; > =C2=A0 =C2=A0s->chr =3D chr; > =C2=A0 =C2=A0serial_init_core(s); > > =C2=A0 =C2=A0vmstate_register(NULL, base, &vmstate_serial, s); > > =C2=A0 =C2=A0register_ioport_write(base, 8, 1, serial_ioport_write, s); > =C2=A0 =C2=A0register_ioport_read(base, 8, 1, serial_ioport_read, s); > =C2=A0 =C2=A0return s; > } > > ISASerialDevice *isa_serial_new(uint32_t index, uint32_t iobase, uint32_t > irq, CharDriverState *chr) > { > =C2=A0 =C2=A0static int index; > =C2=A0 =C2=A0ISADevice *dev =3D isa_create("isa-serial"); > =C2=A0 =C2=A0ISASerialState *isa =3D DO_UPCAST(ISASerialState, dev, dev); > =C2=A0 =C2=A0SerialState *s =3D &isa->state; > > =C2=A0 =C2=A0if (index =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0index =3D index; > =C2=A0 =C2=A0if (index >=3D MAX_SERIAL_PORTS) > =C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL; > =C2=A0 =C2=A0if (iobase =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0iobase =3D isa_serial_io[index]; > =C2=A0 =C2=A0if (isairq =3D=3D -1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0isairq =3D isa_serial_irq[index]; > =C2=A0 =C2=A0index++; > > =C2=A0 =C2=A0s->baudbase =3D 115200; > =C2=A0 =C2=A0isa_init_irq(dev, &s->irq, isairq); > =C2=A0 =C2=A0serial_init_core(s); > =C2=A0 =C2=A0qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3); > > =C2=A0 =C2=A0register_ioport_write(isa->iobase, 8, 1, serial_ioport_write= , s); > =C2=A0 =C2=A0register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, = s); > =C2=A0 =C2=A0isa_init_ioport_range(dev, isa->iobase, 8); > =C2=A0 =C2=A0return isa; > } > > static ISADevice *isa_seral_init_qdev(QemuOpts *opts) > { > =C2=A0 =C2=A0 =C2=A0uint32_t index =3D qemu_opt_get_uint32(opts, "index",= -1); > =C2=A0 =C2=A0 =C2=A0uint32_t irq =3D qemu_opt_get_uint32(opts, "irq", -1)= ; > =C2=A0 =C2=A0 =C2=A0uint32_t iobase =3D qemu_opt_get_uint32(opts, "iobase= ", -1); > =C2=A0 =C2=A0 =C2=A0CharDriverState *chr =3D qemu_opt_get_chr(opts, "char= dev"); > > =C2=A0 =C2=A0 =C2=A0return isa_serial_new(index, irq, iobase, chr); > } > > static void serial_register_devices(void) > { > =C2=A0 =C2=A0isa_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. =C2= =A0So the > isa_seral_init_qdev part is totally optional. I'd still like to have the inline wrapper over the factory interface, probably with similar signature to isa_serial_new. Then there would be two functions, one going through qdev and the other bypassing it. I don't see how that would be useful. The callers of the direct interface would force linkage between them and so it would be impossible to build QEMU with that device. We don't need that flexibility for every device though, but I don't see any advantages for using the direct interface either. Why shouldn't we want all devices to be exposed to the user? For example, there are still devices which don't show up in 'info qtree', which is a shame.