Re: [PATCH 0/9] hw: Use QOM alias properties and few QOM/QDev cleanups

[Mark Cave-Ayland]

On 06/02/2023 15:27, Philippe Mathieu-Daudé wrote:

> On 6/2/23 00:29, Mark Cave-Ayland wrote:
> > On 03/02/2023 11:36, Philippe Mathieu-Daudé wrote:
> >
> > > These patches are extracted from a QOM/QDev refactor series,
> > > so they are preliminary cleanups noticed while working on it:
> > >
> > > - Use correct type when calling qdev_prop_set_xxx()
> > > - Unify some qdev properties in MIPS models
> > > - Replace intermediate properties by link properties
> > > - Remove DEFINE_PROP_DMAADDR() macro which is used one time
> > > - Use qdev_realize_and_unref() instead of open-coding it
> > >
> > > Philippe Mathieu-Daudé (9):
> > >    hw/i386/sgx: Do not open-code qdev_realize_and_unref()
> > >    hw/ppc/sam460ex: Correctly set MAL properties
> > >    hw/arm/nrf51: QOM-alias 'flash-size' property in SoC object
> > >    hw/arm/fsl-imx: QOM-alias 'phy-num' property in SoC object
> > >    hw/usb/hcd-ohci: Include missing 'sysbus.h' header
> > >    hw/display/sm501: QOM-alias 'dma-offset' property in chipset object
> > >    hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'
> > >    hw/mips: Declare all length properties as unsigned
> > >    hw/mips/itu: Pass SAAR using QOM link property
> > >
> > >   hw/arm/fsl-imx25.c           |  3 +--
> > >   hw/arm/fsl-imx6.c            |  3 +--
> > >   hw/arm/fsl-imx6ul.c          |  8 ++++----
> > >   hw/arm/fsl-imx7.c            | 12 ++++++------
> > >   hw/arm/microbit.c            |  5 ++++-
> > >   hw/arm/nrf51_soc.c           | 10 +---------
> > >   hw/display/sm501.c           | 22 +++++++++++-----------
> > >   hw/i386/sgx.c                |  5 ++---
> > >   hw/intc/mips_gic.c           |  4 ++--
> > >   hw/mips/boston.c             |  2 +-
> > >   hw/mips/cps.c                | 35 ++++++++++++-----------------------
> > >   hw/mips/malta.c              |  2 +-
> > >   hw/misc/mips_cmgcr.c         |  2 +-
> > >   hw/misc/mips_itu.c           | 30 ++++++++++++++++++++----------
> > >   hw/nvram/nrf51_nvm.c         |  6 +++++-
> > >   hw/ppc/sam460ex.c            |  4 ++--
> > >   hw/sh4/r2d.c                 |  2 +-
> > >   hw/usb/hcd-ohci-pci.c        |  1 -
> > >   hw/usb/hcd-ohci.c            |  3 +--
> > >   hw/usb/hcd-ohci.h            |  1 +
> > >   include/hw/arm/fsl-imx25.h   |  1 -
> > >   include/hw/arm/fsl-imx6.h    |  1 -
> > >   include/hw/arm/fsl-imx6ul.h  |  2 --
> > >   include/hw/arm/fsl-imx7.h    |  1 -
> > >   include/hw/arm/nrf51_soc.h   |  1 -
> > >   include/hw/intc/mips_gic.h   |  4 ++--
> > >   include/hw/misc/mips_cmgcr.h |  2 +-
> > >   include/hw/misc/mips_itu.h   |  9 ++++-----
> > >   include/hw/qdev-dma.h        | 16 ----------------
> > >   29 files changed, 84 insertions(+), 113 deletions(-)
> > >   delete mode 100644 include/hw/qdev-dma.h
> >
> > I must admit to being slightly nervous about using QOM alias properties in this 
> > way, simply because you start creating implicit dependencies between QOM objects. 
> > How would this work when trying to build machines from configuration files and/or 
> > the monitor? Or are the changes restricted to container devices i.e. those which 
> > consist of in-built child devices?
>
> The latter. All parents forward a property to a contained child.
>
> The parent forwarding property is replaced by a link into the child,
> so accessing the parent property transparently access the child one.
>
> The dependencies are already explicit. We can not create a parent
> without its children (the children creation is implicit when we
> create the parent object).
>
> I thought this was the canonical QOM alias properties use. What is
> the normal use then?

The problem I've found with this approach in the past is that it fails when you have 
more than one child device of the same type.

For example imagine the scenario where there is a QEMU device that contains 2 child 
UARTs and each UART has a property to disable hardware handshaking: if you add a 
property alias to the container device, it can only map to a single child UART. 
Furthermore if you then try to alias the UART IRQs onto the container device using 
qdev_pass_gpios(), then that also fails with 2 UARTs because the gpios from each UART 
have the same property name.

You could then think about solving that problem by using object_property_add_alias() 
directly to specify a different property name for each UART's mapped property on the 
container device, but then you end up accessing the child UART properties with 
different names, but only when using that particular parent container device(!).

For this reason I've tended to avoid aliases and setup child objects from the 
container like this:

   static void container_init(Object *obj)
   {
       object_initialize_child(obj, "uart0", &s->uart0, TYPE_UART);
       object_initialize_child(obj, "uart1", &s->uart1, TYPE_UART);
   }

And then when configuring the board it is possible to obtain the UART references like 
this:

   uart0 = UART(object_resolve_path_component(OBJECT(container), "uart0"));
   irq0 = qdev_connect_gpio_out(DEVICE(uart0), 0, ... );

   uart1 = UART(object_resolve_path_component(OBJECT(container), "uart1"));
   irq1 = qdev_connect_gpio_out(DEVICE(uart1), 0, ... );

This allows all UART configuration to be done in the same way regardless of the 
parent container device and number of child devices, and without having to think 
about using different property names depending upon the container device.

One place where it could conceivably be useful is where you have a chip modelled as a 
device and you want to expose the memory regions and IRQs to an interface such as 
ISA, but often even that doesn't work (think PCI IRQs for example).

The only valid use cases I can think of are the /rtc property (which is an alias to 
the RTC device, regardless of where it exists in the QOM tree) and perhaps in future 
adding similar array aliases to the root of the machine that can point to things like 
block devices, network devices, chardevs and audio devices (i.e. anything that has a 
corresponding QEMU backend).


ATB,

Mark.