From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34066) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ycao7-0001Rf-TG for qemu-devel@nongnu.org; Mon, 30 Mar 2015 10:36:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ycao4-0001VZ-GD for qemu-devel@nongnu.org; Mon, 30 Mar 2015 10:36:51 -0400 Message-ID: <55195F7E.7070309@suse.de> Date: Mon, 30 Mar 2015 16:36:46 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1427651603-9210-1-git-send-email-afaerber@suse.de> <1427651603-9210-3-git-send-email-afaerber@suse.de> <87619ik20c.fsf@blackfin.pond.sub.org> In-Reply-To: <87619ik20c.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC for-2.3? 2/8] pc87312: Create isa-parallel in-place and add alias par0-chardev property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: mark.cave-ayland@ilande.co.uk, qemu-devel@nongnu.org, Alexander Graf , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= , qemu-ppc@nongnu.org, hpoussin@reactos.org, jsnow@redhat.com Am 30.03.2015 um 16:12 schrieb Markus Armbruster: > Andreas F=C3=A4rber writes: >=20 >> Move the parallel_hds[] code to the PReP machine. >=20 > Could use a brief explanation *why* we move it. Yeah, the cover letter did say it probably needs a further iteration - I had bad mobile Internet connection and was in a hurry. >> Signed-off-by: Andreas F=C3=A4rber >> --- >> hw/isa/pc87312.c | 25 +++++++++++++++---------- >> hw/ppc/prep.c | 9 +++++++++ >> include/hw/isa/pc87312.h | 5 ++--- >> 3 files changed, 26 insertions(+), 13 deletions(-) >> >> diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c >> index 40a1106..ded6c01 100644 >> --- a/hw/isa/pc87312.c >> +++ b/hw/isa/pc87312.c >> @@ -268,6 +268,7 @@ static void pc87312_realize(DeviceState *dev, Erro= r **errp) >> ISABus *bus; >> CharDriverState *chr; >> DriveInfo *drive; >> + Error *local_err =3D NULL; >> char name[5]; >> int i; >> =20 >> @@ -278,20 +279,19 @@ static void pc87312_realize(DeviceState *dev, Er= ror **errp) >> pc87312_hard_reset(s); >> =20 >> if (is_parallel_enabled(s)) { >> - chr =3D parallel_hds[0]; >> - if (chr =3D=3D NULL) { >> - chr =3D qemu_chr_new("par0", "null", NULL); >> - } >> - isa =3D isa_create(bus, "isa-parallel"); >> - d =3D DEVICE(isa); >> - qdev_prop_set_uint32(d, "index", 0); >> + d =3D DEVICE(&s->parallel); >> + qdev_set_parent_bus(d, BUS(bus)); >> qdev_prop_set_uint32(d, "iobase", get_parallel_iobase(s)); >> qdev_prop_set_uint32(d, "irq", get_parallel_irq(s)); >> - qdev_prop_set_chr(d, "chardev", chr); >> - qdev_init_nofail(d); >> - s->parallel.dev =3D isa; >> trace_pc87312_info_parallel(get_parallel_iobase(s), >> get_parallel_irq(s)); >> + object_property_set_bool(OBJECT(&s->parallel), true, "realize= d", >> + &local_err); >> + object_unref(OBJECT(&s->parallel)); >=20 > Ignorant question: why unref here? In pair with qdev_set_parent_bus(), inlined from qdev_create(). A discussion of the exact placement recently took place in the context of pc_new_cpu() and ICC, resulting in: "[PATCH for-next] pc: Ensure non-zero CPU ref count after attaching to ICC bus" Basically, we want to defer the unref until the last local usage to avoid concurrent qom-set finalizing the object. >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> } >> =20 >> for (i =3D 0; i < 2; i++) { >> @@ -351,6 +351,11 @@ static void pc87312_initfn(Object *obj) >> PC87312State *s =3D PC87312(obj); >> =20 >> memory_region_init_io(&s->io, obj, &pc87312_io_ops, s, "pc87312",= 2); >> + >> + object_initialize(&s->parallel, sizeof(s->parallel), TYPE_ISA_PAR= ALLEL); >> + object_property_add_alias(obj, "par0-chardev", >> + OBJECT(&s->parallel), "chardev", &error= _abort); >> + qdev_prop_set_uint32(DEVICE(&s->parallel), "index", 0); >> } >> =20 >> static const VMStateDescription vmstate_pc87312 =3D { >> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c >> index 7f52662..1dfa051 100644 >> --- a/hw/ppc/prep.c >> +++ b/hw/ppc/prep.c >> @@ -40,6 +40,7 @@ >> #include "hw/isa/pc87312.h" >> #include "sysemu/block-backend.h" >> #include "sysemu/arch_init.h" >> +#include "sysemu/char.h" >> #include "sysemu/qtest.h" >> #include "exec/address-spaces.h" >> #include "elf.h" >> @@ -528,6 +529,7 @@ static void ppc_prep_init(MachineState *machine) >> PCIDevice *pci; >> ISABus *isa_bus; >> ISADevice *isa; >> + CharDriverState *chr; >> qemu_irq *cpu_exit_irq; >> int ppc_boot_device; >> DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; >> @@ -639,6 +641,13 @@ static void ppc_prep_init(MachineState *machine) >> /* Super I/O (parallel + serial ports) */ >> isa =3D isa_create(isa_bus, TYPE_PC87312); >> dev =3D DEVICE(isa); >> + chr =3D parallel_hds[0]; >> + if (chr =3D=3D NULL) { >> + chr =3D qemu_chr_new("par0", "null", NULL); >> + } >> + if (chr !=3D NULL) { >> + qdev_prop_set_chr(dev, "par0-chardev", chr); >> + } >> qdev_prop_set_uint8(dev, "config", 13); /* fdc, ser0, ser1, par0 = */ >> qdev_init_nofail(dev); >> =20 >=20 > Unlike the old code, this checks for qemu_chr_new() failure. Can this > happen? Probably a result of moving the code piecewise. Not sure about the API, to me it looks like it can return NULL and can emit an Error via error_report_err() but doesn't pass it out to the caller... > If it does, property "par0-chardev" remains null. > parallel_isa_realizefn() will then fail. Worth a comment, I think. >=20 > I don't like boards creating backends with IDs on their own as long as > we don't have a ID namespace reserved for the system. However, this is > preexisting, so let's not worry about it now. I think the problem was that PC handles this by creating devices only if it has a default or user-supplied backend. Here, we need all aggregate devices independent of what the user supplies, so to avoid segfaults/errors we create dummy ones. Regards, Andreas --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Felix Imend=C3=B6rffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=C3=BCrnberg)