Re: [Qemu-devel] [PATCH RFC for-2.3? 2/8] pc87312: Create isa-parallel in-place and add alias par0-chardev property

["Andreas Färber" <afaerber@suse.de>]

Am 30.03.2015 um 16:12 schrieb Markus Armbruster:
> Andreas Färber <afaerber@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 <afaerber@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)