Am 30.03.2015 um 16:12 schrieb Markus Armbruster:
> Andreas Färber <afaer...@suse.de> writes:
>
>> Move the parallel_hds[] code to the PReP machine.
>
> 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ärber <afaer...@suse.de>
>> ---
>> 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, Error
>> **errp)
>> ISABus *bus;
>> CharDriverState *chr;
>> DriveInfo *drive;
>> + Error *local_err = NULL;
>> char name[5];
>> int i;
>>
>> @@ -278,20 +279,19 @@ static void pc87312_realize(DeviceState *dev, Error
>> **errp)
>> pc87312_hard_reset(s);
>>
>> if (is_parallel_enabled(s)) {
>> - chr = parallel_hds[0];
>> - if (chr == NULL) {
>> - chr = qemu_chr_new("par0", "null", NULL);
>> - }
>> - isa = isa_create(bus, "isa-parallel");
>> - d = DEVICE(isa);
>> - qdev_prop_set_uint32(d, "index", 0);
>> + d = 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 = isa;
>> trace_pc87312_info_parallel(get_parallel_iobase(s),
>> get_parallel_irq(s));
>> + object_property_set_bool(OBJECT(&s->parallel), true, "realized",
>> + &local_err);
>> + object_unref(OBJECT(&s->parallel));
>
> 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;
>> + }
>> }
>>
>> for (i = 0; i < 2; i++) {
>> @@ -351,6 +351,11 @@ static void pc87312_initfn(Object *obj)
>> PC87312State *s = PC87312(obj);
>>
>> memory_region_init_io(&s->io, obj, &pc87312_io_ops, s, "pc87312", 2);
>> +
>> + object_initialize(&s->parallel, sizeof(s->parallel), TYPE_ISA_PARALLEL);
>> + object_property_add_alias(obj, "par0-chardev",
>> + OBJECT(&s->parallel), "chardev",
>> &error_abort);
>> + qdev_prop_set_uint32(DEVICE(&s->parallel), "index", 0);
>> }
>>
>> static const VMStateDescription vmstate_pc87312 = {
>> 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 = isa_create(isa_bus, TYPE_PC87312);
>> dev = DEVICE(isa);
>> + chr = parallel_hds[0];
>> + if (chr == NULL) {
>> + chr = qemu_chr_new("par0", "null", NULL);
>> + }
>> + if (chr != NULL) {
>> + qdev_prop_set_chr(dev, "par0-chardev", chr);
>> + }
>> qdev_prop_set_uint8(dev, "config", 13); /* fdc, ser0, ser1, par0 */
>> qdev_init_nofail(dev);
>>
>
> 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.
>
> 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
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)
