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

[Markus Armbruster <armbru@redhat.com>]

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.

> 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?

> +        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?

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.

> diff --git a/include/hw/isa/pc87312.h b/include/hw/isa/pc87312.h
> index bf74470..b473b3b 100644
> --- a/include/hw/isa/pc87312.h
> +++ b/include/hw/isa/pc87312.h
> @@ -26,6 +26,7 @@
>  #define QEMU_PC87312_H
>  
>  #include "hw/isa/isa.h"
> +#include "hw/char/parallel.h"
>  
>  
>  #define TYPE_PC87312 "pc87312"
> @@ -37,9 +38,7 @@ typedef struct PC87312State {
>      uint32_t iobase;
>      uint8_t config; /* initial configuration */
>  
> -    struct {
> -        ISADevice *dev;
> -    } parallel;
> +    ISAParallelState parallel;
>  
>      struct {
>          ISADevice *dev;