Re: [PATCH 0/9] hw: Use QOM alias properties and few QOM/QDev cleanups
On 06/02/2023 23:04, Philippe Mathieu-Daudé wrote:
On 6/2/23 22:54, Mark Cave-Ayland wrote:
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
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.
I noticed some qdev gpio namespace issues. Thanks for pointing that
qdev_pass_gpios() restriction.
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", >uart0, TYPE_UART);
object_initialize_child(obj, "uart1", >uart1, TYPE_UART);
}
Hmm OK, this is the approach used in IMX:
@@ -120,8 +120,12 @@ static void fsl_imx6ul_init(Object *obj)
* Ethernet
*/
for (i = 0; i < FSL_IMX6UL_NUM_ETHS; i++) {
+ g_autofree gchar *propname = g_strdup_printf("fec%d-phy-num", i + 1);
snprintf(name, NAME_SIZE, "eth%d", i);
object_initialize_child(obj, name, >eth[i], TYPE_IMX_ENET);
+ qdev_prop_set_uint32(DEVICE(>eth[i]), "phy-num", i);
+ object_property_add_alias(obj, propname,
+ OBJECT(>eth[i]), "phy-num");
}
But then this is how it is used today:
static Property fsl_imx6ul_properties[] = {
- DEFINE_PROP_UINT32("fec1-phy-num", FslIMX6ULState, phy_num[0], 0),
- DEFINE_PROP_UINT32("fec2-phy-num", FslIMX6ULState, phy_num[1], 1),
DEFINE_PROP_END_OF_LIST(),
};
What do you mean by "you end up accessing the child UART properties with
different names, but only when using that particular parent container
device(!)."? I tend to see QOM modelling as matching hardware design,
so if a container is used, there is a similar relationship / hierarchy
in the hardware, then accessing the children via a particular parent
container path sounds the correct way. QOM indexed child must have the
same meaning in the hardware layout.
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.
OK I think this is the same explanation as what I just wrote earlier.
Yes indeed, I think we're on the same page here now.
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
Re: [PATCH 0/9] hw: Use QOM alias properties and few QOM/QDev cleanups
On 6/2/23 22:54, Mark Cave-Ayland wrote:
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
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.
I noticed some qdev gpio namespace issues. Thanks for pointing that
qdev_pass_gpios() restriction.
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", >uart0, TYPE_UART);
object_initialize_child(obj, "uart1", >uart1, TYPE_UART);
}
Hmm OK, this is the approach used in IMX:
@@ -120,8 +120,12 @@ static void fsl_imx6ul_init(Object *obj)
* Ethernet
*/
for (i = 0; i < FSL_IMX6UL_NUM_ETHS; i++) {
+g_autofree gchar *propname = g_strdup_printf("fec%d-phy-num", i
+ 1);
snprintf(name, NAME_SIZE, "eth%d", i);
object_initialize_child(obj, name, >eth[i], TYPE_IMX_ENET);
+qdev_prop_set_uint32(DEVICE(>eth[i]), "phy-num", i);
+object_property_add_alias(obj, propname,
+ OBJECT(>eth[i]), "phy-num");
}
But then this is how it is used today:
static Property fsl_imx6ul_properties[] = {
-DEFINE_PROP_UINT32("fec1-phy-num", FslIMX6ULState, phy_num[0], 0),
-DEFINE_PROP_UINT32("fec2-phy-num", FslIMX6ULState, phy_num[1], 1),
DEFINE_PROP_END_OF_LIST(),
};
What do you mean by "you end up accessing the child UART properties with
different names, but only when using that particular parent container
device(!)."? I tend to see QOM modelling as matching hardware design,
so if a container is used, there is a similar relationship / hierarchy
in the hardware, then accessing the children via a particular parent
container path sounds the correct way. QOM indexed child must have the
same meaning in the hardware layout.
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.
OK I think this is the same explanation as what I just wrote earlier.
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).
IRQ wiring is an unsolved problem in my TODO, in particular when a
Re: [PATCH 0/9] hw: Use QOM alias properties and few QOM/QDev cleanups
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", >uart0, TYPE_UART);
object_initialize_child(obj, "uart1", >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
Re: [PATCH 0/9] hw: Use QOM alias properties and few QOM/QDev cleanups
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?
Re: [PATCH 0/9] hw: Use QOM alias properties and few QOM/QDev cleanups
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? ATB, Mark.
[PATCH 0/9] hw: Use QOM alias properties and few QOM/QDev cleanups
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 -- 2.38.1
