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

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

[Philippe Mathieu-Daudé]

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

[PATCH 1/9] hw/i386/sgx: Do not open-code qdev_realize_and_unref()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/sgx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index db004d17a6..5ddc5d7ea2 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -299,7 +299,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
                                 &sgx_epc->mr);
 
     for (list = x86ms->sgx_epc_list; list; list = list->next) {
-        obj = object_new("sgx-epc");
+        obj = object_new(TYPE_SGX_EPC);
 
         /* set the memdev link with memory backend */
         object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
@@ -307,8 +307,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
         /* set the numa node property for sgx epc object */
         object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
                              &error_fatal);
-        object_property_set_bool(obj, "realized", true, &error_fatal);
-        object_unref(obj);
+        qdev_realize_and_unref(DEVICE(obj), NULL, &error_fatal);
     }
 
     if ((sgx_epc->base + sgx_epc->size) < sgx_epc->base) {
-- 
2.38.1

Re: [PATCH 1/9] hw/i386/sgx: Do not open-code qdev_realize_and_unref()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/i386/sgx.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> index db004d17a6..5ddc5d7ea2 100644
> --- a/hw/i386/sgx.c
> +++ b/hw/i386/sgx.c
> @@ -299,7 +299,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>                                  &sgx_epc->mr);
>  
>      for (list = x86ms->sgx_epc_list; list; list = list->next) {
> -        obj = object_new("sgx-epc");
> +        obj = object_new(TYPE_SGX_EPC);

I wonder why this doesn't use qdev_new().
>  
>          /* set the memdev link with memory backend */
>          object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
> @@ -307,8 +307,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>          /* set the numa node property for sgx epc object */
>          object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
>                               &error_fatal);
> -        object_property_set_bool(obj, "realized", true, &error_fatal);
> -        object_unref(obj);
> +        qdev_realize_and_unref(DEVICE(obj), NULL, &error_fatal);

Yes, please!  Must have crept in after I converted all realizations.
I can see two more:

hw/pci/pcie_sriov.c:        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
linux-user/syscall.c:            object_property_set_bool(OBJECT(cpu), "realized", false, NULL);

>      }
>  
>      if ((sgx_epc->base + sgx_epc->size) < sgx_epc->base) {

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 1/9] hw/i386/sgx: Do not open-code qdev_realize_and_unref()

[Philippe Mathieu-Daudé]

On 3/2/23 13:32, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/sgx.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
>> index db004d17a6..5ddc5d7ea2 100644
>> --- a/hw/i386/sgx.c
>> +++ b/hw/i386/sgx.c
>> @@ -299,7 +299,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>                                   &sgx_epc->mr);
>>   
>>       for (list = x86ms->sgx_epc_list; list; list = list->next) {
>> -        obj = object_new("sgx-epc");
>> +        obj = object_new(TYPE_SGX_EPC);
> 
> I wonder why this doesn't use qdev_new().

OK.

>>           /* set the memdev link with memory backend */
>>           object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
>> @@ -307,8 +307,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>           /* set the numa node property for sgx epc object */
>>           object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
>>                                &error_fatal);
>> -        object_property_set_bool(obj, "realized", true, &error_fatal);
>> -        object_unref(obj);
>> +        qdev_realize_and_unref(DEVICE(obj), NULL, &error_fatal);
> 
> Yes, please!  Must have crept in after I converted all realizations.
> I can see two more:
> 
> hw/pci/pcie_sriov.c:        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
> linux-user/syscall.c:            object_property_set_bool(OBJECT(cpu), "realized", false, NULL);

No: these would be qdev_UNrealize_and_unref(). Do we want it?
Maybe to avoid open-coding it, yes?

>>       }
>>   
>>       if ((sgx_epc->base + sgx_epc->size) < sgx_epc->base) {
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

Re: [PATCH 1/9] hw/i386/sgx: Do not open-code qdev_realize_and_unref()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 3/2/23 13:32, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/i386/sgx.c | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
>>> index db004d17a6..5ddc5d7ea2 100644
>>> --- a/hw/i386/sgx.c
>>> +++ b/hw/i386/sgx.c
>>> @@ -299,7 +299,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>>                                   &sgx_epc->mr);
>>>   
>>>       for (list = x86ms->sgx_epc_list; list; list = list->next) {
>>> -        obj = object_new("sgx-epc");
>>> +        obj = object_new(TYPE_SGX_EPC);
>> 
>> I wonder why this doesn't use qdev_new().
>
> OK.

Observation, not demand!

>>>           /* set the memdev link with memory backend */
>>>           object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
>>> @@ -307,8 +307,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>>           /* set the numa node property for sgx epc object */
>>>           object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
>>>                                &error_fatal);
>>> -        object_property_set_bool(obj, "realized", true, &error_fatal);
>>> -        object_unref(obj);
>>> +        qdev_realize_and_unref(DEVICE(obj), NULL, &error_fatal);
>> 
>> Yes, please!  Must have crept in after I converted all realizations.
>> I can see two more:
>> 
>> hw/pci/pcie_sriov.c:        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
>> linux-user/syscall.c:            object_property_set_bool(OBJECT(cpu), "realized", false, NULL);
>
> No: these would be qdev_UNrealize_and_unref().

You're right.

>                                                Do we want it?
> Maybe to avoid open-coding it, yes?

I think so.  See

    dc3edf8d8a qdev: Convert to qdev_unrealize() manually
    981c3dcd94 qdev: Convert to qdev_unrealize() with Coccinelle

>>>       }
>>>   
>>>       if ((sgx_epc->base + sgx_epc->size) < sgx_epc->base) {
>> 
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks!

[PATCH 2/9] hw/ppc/sam460ex: Correctly set MAL properties

[Philippe Mathieu-Daudé]

MAL properties are declared as uint8_t:

  static Property ppc4xx_mal_properties[] = {
      DEFINE_PROP_UINT8("txc-num", Ppc4xxMalState, txcnum, 0),
      DEFINE_PROP_UINT8("rxc-num", Ppc4xxMalState, rxcnum, 0),
      DEFINE_PROP_END_OF_LIST(),
  };

Set the property using qdev_prop_set_uint8().

Fixes: da116a8aab ("ppc/ppc405: QOM'ify MAL")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/sam460ex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4a22ce3761..cf065aae0e 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -389,8 +389,8 @@ static void sam460ex_init(MachineState *machine)
 
     /* MAL */
     dev = qdev_new(TYPE_PPC4xx_MAL);
-    qdev_prop_set_uint32(dev, "txc-num", 4);
-    qdev_prop_set_uint32(dev, "rxc-num", 16);
+    qdev_prop_set_uint8(dev, "txc-num", 4);
+    qdev_prop_set_uint8(dev, "rxc-num", 16);
     ppc4xx_dcr_realize(PPC4xx_DCR_DEVICE(dev), cpu, &error_fatal);
     object_unref(OBJECT(dev));
     sbdev = SYS_BUS_DEVICE(dev);
-- 
2.38.1

Re: [PATCH 2/9] hw/ppc/sam460ex: Correctly set MAL properties

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
> MAL properties are declared as uint8_t:
>
>  static Property ppc4xx_mal_properties[] = {
>      DEFINE_PROP_UINT8("txc-num", Ppc4xxMalState, txcnum, 0),
>      DEFINE_PROP_UINT8("rxc-num", Ppc4xxMalState, rxcnum, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> Set the property using qdev_prop_set_uint8().
>
> Fixes: da116a8aab ("ppc/ppc405: QOM'ify MAL")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Apparently this makes no difference but MAL is also only there so the 
firmware can program it but does nothing otherwise.

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/ppc/sam460ex.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 4a22ce3761..cf065aae0e 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -389,8 +389,8 @@ static void sam460ex_init(MachineState *machine)
>
>     /* MAL */
>     dev = qdev_new(TYPE_PPC4xx_MAL);
> -    qdev_prop_set_uint32(dev, "txc-num", 4);
> -    qdev_prop_set_uint32(dev, "rxc-num", 16);
> +    qdev_prop_set_uint8(dev, "txc-num", 4);
> +    qdev_prop_set_uint8(dev, "rxc-num", 16);
>     ppc4xx_dcr_realize(PPC4xx_DCR_DEVICE(dev), cpu, &error_fatal);
>     object_unref(OBJECT(dev));
>     sbdev = SYS_BUS_DEVICE(dev);
>

[PATCH 3/9] hw/arm/nrf51: QOM-alias 'flash-size' property in SoC object

[Philippe Mathieu-Daudé]

No need to use an intermediate 'flash-size' property in the
SoC object. Alias the property, so when the machine (here
microbit) sets the value on the SoC, it is propagated to
the flash object.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/microbit.c          |  5 ++++-
 hw/arm/nrf51_soc.c         | 10 +---------
 hw/nvram/nrf51_nvm.c       |  6 +++++-
 include/hw/arm/nrf51_soc.h |  1 -
 4 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index 50df362088..79b5574884 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -36,6 +36,7 @@ static void microbit_init(MachineState *machine)
     MicrobitMachineState *s = MICROBIT_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *mr;
+    int64_t flash_size;
 
     object_initialize_child(OBJECT(machine), "nrf51", &s->nrf51,
                             TYPE_NRF51_SOC);
@@ -43,6 +44,8 @@ static void microbit_init(MachineState *machine)
     object_property_set_link(OBJECT(&s->nrf51), "memory",
                              OBJECT(system_memory), &error_fatal);
     sysbus_realize(SYS_BUS_DEVICE(&s->nrf51), &error_fatal);
+    flash_size = object_property_get_int(OBJECT(&s->nrf51),
+                                         "flash-size", &error_abort);
 
     /*
      * Overlap the TWI stub device into the SoC.  This is a microbit-specific
@@ -57,7 +60,7 @@ static void microbit_init(MachineState *machine)
                                         mr, -1);
 
     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
-                       0, s->nrf51.flash_size);
+                       0, flash_size);
 }
 
 static void microbit_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 34da0d62f0..cc4a636c51 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -24,9 +24,7 @@
  * are supported in the future, add a sub-class of NRF51SoC for
  * the specific variants
  */
-#define NRF51822_FLASH_PAGES    256
 #define NRF51822_SRAM_PAGES     16
-#define NRF51822_FLASH_SIZE     (NRF51822_FLASH_PAGES * NRF51_PAGE_SIZE)
 #define NRF51822_SRAM_SIZE      (NRF51822_SRAM_PAGES * NRF51_PAGE_SIZE)
 
 #define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
@@ -122,11 +120,6 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
                        BASE_TO_IRQ(NRF51_RNG_BASE)));
 
     /* UICR, FICR, NVMC, FLASH */
-    if (!object_property_set_uint(OBJECT(&s->nvm), "flash-size",
-                                  s->flash_size, errp)) {
-        return;
-    }
-
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->nvm), errp)) {
         return;
     }
@@ -199,6 +192,7 @@ static void nrf51_soc_init(Object *obj)
     object_initialize_child(obj, "rng", &s->rng, TYPE_NRF51_RNG);
 
     object_initialize_child(obj, "nvm", &s->nvm, TYPE_NRF51_NVM);
+    object_property_add_alias(obj, "flash-size", OBJECT(&s->nvm), "flash-size");
 
     object_initialize_child(obj, "gpio", &s->gpio, TYPE_NRF51_GPIO);
 
@@ -215,8 +209,6 @@ static Property nrf51_soc_properties[] = {
     DEFINE_PROP_LINK("memory", NRF51State, board_memory, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_UINT32("sram-size", NRF51State, sram_size, NRF51822_SRAM_SIZE),
-    DEFINE_PROP_UINT32("flash-size", NRF51State, flash_size,
-                       NRF51822_FLASH_SIZE),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
index 7f1db8c423..bfae028fcd 100644
--- a/hw/nvram/nrf51_nvm.c
+++ b/hw/nvram/nrf51_nvm.c
@@ -26,6 +26,9 @@
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 
+#define NRF51822_FLASH_PAGES    256
+#define NRF51822_FLASH_SIZE     (NRF51822_FLASH_PAGES * NRF51_PAGE_SIZE)
+
 /*
  * FICR Registers Assignments
  * CODEPAGESIZE      0x010
@@ -358,7 +361,8 @@ static void nrf51_nvm_reset(DeviceState *dev)
 }
 
 static Property nrf51_nvm_properties[] = {
-    DEFINE_PROP_UINT32("flash-size", NRF51NVMState, flash_size, 0x40000),
+    DEFINE_PROP_UINT32("flash-size", NRF51NVMState,
+                       flash_size, NRF51822_FLASH_SIZE),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index e52a56e75e..8cf0c21614 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -45,7 +45,6 @@ struct NRF51State {
     MemoryRegion twi;
 
     uint32_t sram_size;
-    uint32_t flash_size;
 
     MemoryRegion *board_memory;
 
-- 
2.38.1

[PATCH 4/9] hw/arm/fsl-imx: QOM-alias 'phy-num' property in SoC object

[Philippe Mathieu-Daudé]

No need to use intermediate 'fec-phy-num' properties in the
SoC object. Alias the properties, so when the machines set
the value on the SoC, it is propagated to the network device
object.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/fsl-imx25.c          |  3 +--
 hw/arm/fsl-imx6.c           |  3 +--
 hw/arm/fsl-imx6ul.c         |  8 ++++----
 hw/arm/fsl-imx7.c           | 12 ++++++------
 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 -
 8 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 24c4374590..eff58e1f7a 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -55,6 +55,7 @@ static void fsl_imx25_init(Object *obj)
     }
 
     object_initialize_child(obj, "fec", &s->fec, TYPE_IMX_FEC);
+    object_property_add_alias(obj, "fec-phy-num", OBJECT(&s->fec), "phy-num");
 
     object_initialize_child(obj, "rngc", &s->rngc, TYPE_IMX_RNGC);
 
@@ -169,7 +170,6 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
                                             epit_table[i].irq));
     }
 
-    object_property_set_uint(OBJECT(&s->fec), "phy-num", s->phy_num, &err);
     qdev_set_nic_properties(DEVICE(&s->fec), &nd_table[0]);
 
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->fec), errp)) {
@@ -315,7 +315,6 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
 }
 
 static Property fsl_imx25_properties[] = {
-    DEFINE_PROP_UINT32("fec-phy-num", FslIMX25State, phy_num, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 00dafe3f62..4f870c928c 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -100,6 +100,7 @@ static void fsl_imx6_init(Object *obj)
 
 
     object_initialize_child(obj, "eth", &s->eth, TYPE_IMX_ENET);
+    object_property_add_alias(obj, "fec-phy-num", OBJECT(&s->eth), "phy-num");
 }
 
 static void fsl_imx6_realize(DeviceState *dev, Error **errp)
@@ -377,7 +378,6 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
                                             spi_table[i].irq));
     }
 
-    object_property_set_uint(OBJECT(&s->eth), "phy-num", s->phy_num, &err);
     qdev_set_nic_properties(DEVICE(&s->eth), &nd_table[0]);
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->eth), errp)) {
         return;
@@ -451,7 +451,6 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
 }
 
 static Property fsl_imx6_properties[] = {
-    DEFINE_PROP_UINT32("fec-phy-num", FslIMX6State, phy_num, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index d88d6cc1c5..8b3939e8c5 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -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, &s->eth[i], TYPE_IMX_ENET);
+        qdev_prop_set_uint32(DEVICE(&s->eth[i]), "phy-num", i);
+        object_property_add_alias(obj, propname,
+                                  OBJECT(&s->eth[i]), "phy-num");
     }
 
     /* USB */
@@ -424,8 +428,6 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
             FSL_IMX6UL_ENET2_TIMER_IRQ,
         };
 
-        object_property_set_uint(OBJECT(&s->eth[i]), "phy-num",
-                                 s->phy_num[i], &error_abort);
         object_property_set_uint(OBJECT(&s->eth[i]), "tx-ring-num",
                                  FSL_IMX6UL_ETH_NUM_TX_RINGS, &error_abort);
         qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
@@ -618,8 +620,6 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
 }
 
 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(),
 };
 
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index afc7480799..df035c9314 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -102,8 +102,12 @@ static void fsl_imx7_init(Object *obj)
      * Ethernet
      */
     for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
-            snprintf(name, NAME_SIZE, "eth%d", i);
-            object_initialize_child(obj, name, &s->eth[i], TYPE_IMX_ENET);
+        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, &s->eth[i], TYPE_IMX_ENET);
+        qdev_prop_set_uint32(DEVICE(&s->eth[i]), "phy-num", i);
+        object_property_add_alias(obj, propname,
+                                  OBJECT(&s->eth[i]), "phy-num");
     }
 
     /*
@@ -402,8 +406,6 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
             FSL_IMX7_ENET2_ADDR,
         };
 
-        object_property_set_uint(OBJECT(&s->eth[i]), "phy-num",
-                                 s->phy_num[i], &error_abort);
         object_property_set_uint(OBJECT(&s->eth[i]), "tx-ring-num",
                                  FSL_IMX7_ETH_NUM_TX_RINGS, &error_abort);
         qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
@@ -599,8 +601,6 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
 }
 
 static Property fsl_imx7_properties[] = {
-    DEFINE_PROP_UINT32("fec1-phy-num", FslIMX7State, phy_num[0], 0),
-    DEFINE_PROP_UINT32("fec2-phy-num", FslIMX7State, phy_num[1], 1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/arm/fsl-imx25.h b/include/hw/arm/fsl-imx25.h
index 1b1086e945..e377f8e79a 100644
--- a/include/hw/arm/fsl-imx25.h
+++ b/include/hw/arm/fsl-imx25.h
@@ -66,7 +66,6 @@ struct FslIMX25State {
     MemoryRegion   rom[2];
     MemoryRegion   iram;
     MemoryRegion   iram_alias;
-    uint32_t       phy_num;
 };
 
 /**
diff --git a/include/hw/arm/fsl-imx6.h b/include/hw/arm/fsl-imx6.h
index 83291457cf..f7d1a94640 100644
--- a/include/hw/arm/fsl-imx6.h
+++ b/include/hw/arm/fsl-imx6.h
@@ -74,7 +74,6 @@ struct FslIMX6State {
     MemoryRegion   caam;
     MemoryRegion   ocram;
     MemoryRegion   ocram_alias;
-    uint32_t       phy_num;
 };
 
 
diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index 7812e516a5..5217eeb8ff 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -88,8 +88,6 @@ struct FslIMX6ULState {
     MemoryRegion       caam;
     MemoryRegion       ocram;
     MemoryRegion       ocram_alias;
-
-    uint32_t           phy_num[FSL_IMX6UL_NUM_ETHS];
 };
 
 enum FslIMX6ULMemoryMap {
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index 4e5e071864..16c68a4937 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -82,7 +82,6 @@ struct FslIMX7State {
     IMX7GPRState       gpr;
     ChipideaState      usb[FSL_IMX7_NUM_USBS];
     DesignwarePCIEHost pcie;
-    uint32_t           phy_num[FSL_IMX7_NUM_ETHS];
 };
 
 enum FslIMX7MemoryMap {
-- 
2.38.1

[PATCH 5/9] hw/usb/hcd-ohci: Include missing 'sysbus.h' header

[Philippe Mathieu-Daudé]

Avoid when including "hw/usb/hcd-ohci.h":

  hw/usb/hcd-ohci.h:100:5: error: unknown type name 'SysBusDevice'
      SysBusDevice parent_obj;
      ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/usb/hcd-ohci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/hcd-ohci.h b/hw/usb/hcd-ohci.h
index 11ac57058d..e5e6b434fd 100644
--- a/hw/usb/hcd-ohci.h
+++ b/hw/usb/hcd-ohci.h
@@ -21,6 +21,7 @@
 #ifndef HCD_OHCI_H
 #define HCD_OHCI_H
 
+#include "hw/sysbus.h"
 #include "sysemu/dma.h"
 #include "hw/usb.h"
 #include "qom/object.h"
-- 
2.38.1

Re: [PATCH 5/9] hw/usb/hcd-ohci: Include missing 'sysbus.h' header

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Avoid when including "hw/usb/hcd-ohci.h":
>
>   hw/usb/hcd-ohci.h:100:5: error: unknown type name 'SysBusDevice'
>       SysBusDevice parent_obj;
>       ^
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/usb/hcd-ohci.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/usb/hcd-ohci.h b/hw/usb/hcd-ohci.h
> index 11ac57058d..e5e6b434fd 100644
> --- a/hw/usb/hcd-ohci.h
> +++ b/hw/usb/hcd-ohci.h
> @@ -21,6 +21,7 @@
>  #ifndef HCD_OHCI_H
>  #define HCD_OHCI_H
>  
> +#include "hw/sysbus.h"
>  #include "sysemu/dma.h"
>  #include "hw/usb.h"
>  #include "qom/object.h"

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH 6/9] hw/display/sm501: QOM-alias 'dma-offset' property in chipset object

[Philippe Mathieu-Daudé]

No need to use an intermediate 'dma-offset' property in the
chipset object. Alias the property, so when the machine (here
r2d-plus) sets the value on the chipset, it is propagated to
the OHCI object.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/sm501.c | 22 +++++++++++-----------
 hw/sh4/r2d.c       |  2 +-
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 52e42585af..49a648e952 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -28,6 +28,7 @@
 #include "qapi/error.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "hw/usb/hcd-ohci.h"
 #include "hw/char/serial.h"
 #include "ui/console.h"
 #include "hw/sysbus.h"
@@ -1942,7 +1943,7 @@ struct SM501SysBusState {
     /*< public >*/
     SM501State state;
     uint32_t vram_size;
-    uint32_t base;
+    OHCISysBusState ohci;
     SerialMM serial;
 };
 
@@ -1950,7 +1951,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
 {
     SM501SysBusState *s = SYSBUS_SM501(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-    DeviceState *usb_dev;
     MemoryRegion *mr;
 
     sm501_init(&s->state, dev, s->vram_size);
@@ -1963,13 +1963,10 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->state.mmio_region);
 
     /* bridge to usb host emulation module */
-    usb_dev = qdev_new("sysbus-ohci");
-    qdev_prop_set_uint32(usb_dev, "num-ports", 2);
-    qdev_prop_set_uint64(usb_dev, "dma-offset", s->base);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(usb_dev), &error_fatal);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(&s->ohci), &error_fatal);
     memory_region_add_subregion(&s->state.mmio_region, SM501_USB_HOST,
-                       sysbus_mmio_get_region(SYS_BUS_DEVICE(usb_dev), 0));
-    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(usb_dev));
+                       sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ohci), 0));
+    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->ohci));
 
     /* bridge to serial emulation module */
     sysbus_realize(SYS_BUS_DEVICE(&s->serial), &error_fatal);
@@ -1980,7 +1977,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
 
 static Property sm501_sysbus_properties[] = {
     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
-    DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2016,15 +2012,19 @@ static void sm501_sysbus_class_init(ObjectClass *klass, void *data)
 static void sm501_sysbus_init(Object *o)
 {
     SM501SysBusState *sm501 = SYSBUS_SM501(o);
+    OHCISysBusState *ohci = &sm501->ohci;
     SerialMM *smm = &sm501->serial;
 
+    object_initialize_child(o, "ohci", ohci, TYPE_SYSBUS_OHCI);
+    object_property_add_alias(o, "base", OBJECT(ohci), "dma-offset");
+    qdev_prop_set_uint32(DEVICE(ohci), "num-ports", 2);
+
     object_initialize_child(o, "serial", smm, TYPE_SERIAL_MM);
     qdev_set_legacy_instance_id(DEVICE(smm), SM501_UART0, 2);
     qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
     qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN);
 
-    object_property_add_alias(o, "chardev",
-                              OBJECT(smm), "chardev");
+    object_property_add_alias(o, "chardev", OBJECT(smm), "chardev");
 }
 
 static const TypeInfo sm501_sysbus_info = {
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 39fc4f19d9..279724ffbb 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -274,7 +274,7 @@ static void r2d_init(MachineState *machine)
     dev = qdev_new("sysbus-sm501");
     busdev = SYS_BUS_DEVICE(dev);
     qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
-    qdev_prop_set_uint32(dev, "base", 0x10000000);
+    qdev_prop_set_uint64(dev, "base", 0x10000000);
     qdev_prop_set_chr(dev, "chardev", serial_hd(2));
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, 0x10000000);
-- 
2.38.1

Re: [PATCH 6/9] hw/display/sm501: QOM-alias 'dma-offset' property in chipset object

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4455 bytes --]

On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
> No need to use an intermediate 'dma-offset' property in the
> chipset object. Alias the property, so when the machine (here
> r2d-plus) sets the value on the chipset, it is propagated to
> the OHCI object.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/display/sm501.c | 22 +++++++++++-----------
> hw/sh4/r2d.c       |  2 +-
> 2 files changed, 12 insertions(+), 12 deletions(-)

It does not seem to be any simpler by the number of lines but maybe a bit 
cleaner. I wonder if it would worth renaming the property to dma-offset to 
match that of ohci so it's less confusing what it refers to. It's only 
used by r2d and this patch already changing that so would be an easy 
change.

Regards,
BALATON Zoltan

> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 52e42585af..49a648e952 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -28,6 +28,7 @@
> #include "qapi/error.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> +#include "hw/usb/hcd-ohci.h"
> #include "hw/char/serial.h"
> #include "ui/console.h"
> #include "hw/sysbus.h"
> @@ -1942,7 +1943,7 @@ struct SM501SysBusState {
>     /*< public >*/
>     SM501State state;
>     uint32_t vram_size;
> -    uint32_t base;
> +    OHCISysBusState ohci;
>     SerialMM serial;
> };
>
> @@ -1950,7 +1951,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> {
>     SM501SysBusState *s = SYSBUS_SM501(dev);
>     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> -    DeviceState *usb_dev;
>     MemoryRegion *mr;
>
>     sm501_init(&s->state, dev, s->vram_size);
> @@ -1963,13 +1963,10 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>     sysbus_init_mmio(sbd, &s->state.mmio_region);
>
>     /* bridge to usb host emulation module */
> -    usb_dev = qdev_new("sysbus-ohci");
> -    qdev_prop_set_uint32(usb_dev, "num-ports", 2);
> -    qdev_prop_set_uint64(usb_dev, "dma-offset", s->base);
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(usb_dev), &error_fatal);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(&s->ohci), &error_fatal);
>     memory_region_add_subregion(&s->state.mmio_region, SM501_USB_HOST,
> -                       sysbus_mmio_get_region(SYS_BUS_DEVICE(usb_dev), 0));
> -    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(usb_dev));
> +                       sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ohci), 0));
> +    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->ohci));
>
>     /* bridge to serial emulation module */
>     sysbus_realize(SYS_BUS_DEVICE(&s->serial), &error_fatal);
> @@ -1980,7 +1977,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>
> static Property sm501_sysbus_properties[] = {
>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> -    DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2016,15 +2012,19 @@ static void sm501_sysbus_class_init(ObjectClass *klass, void *data)
> static void sm501_sysbus_init(Object *o)
> {
>     SM501SysBusState *sm501 = SYSBUS_SM501(o);
> +    OHCISysBusState *ohci = &sm501->ohci;
>     SerialMM *smm = &sm501->serial;
>
> +    object_initialize_child(o, "ohci", ohci, TYPE_SYSBUS_OHCI);
> +    object_property_add_alias(o, "base", OBJECT(ohci), "dma-offset");
> +    qdev_prop_set_uint32(DEVICE(ohci), "num-ports", 2);
> +
>     object_initialize_child(o, "serial", smm, TYPE_SERIAL_MM);
>     qdev_set_legacy_instance_id(DEVICE(smm), SM501_UART0, 2);
>     qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
>     qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN);
>
> -    object_property_add_alias(o, "chardev",
> -                              OBJECT(smm), "chardev");
> +    object_property_add_alias(o, "chardev", OBJECT(smm), "chardev");
> }
>
> static const TypeInfo sm501_sysbus_info = {
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index 39fc4f19d9..279724ffbb 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -274,7 +274,7 @@ static void r2d_init(MachineState *machine)
>     dev = qdev_new("sysbus-sm501");
>     busdev = SYS_BUS_DEVICE(dev);
>     qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
> -    qdev_prop_set_uint32(dev, "base", 0x10000000);
> +    qdev_prop_set_uint64(dev, "base", 0x10000000);
>     qdev_prop_set_chr(dev, "chardev", serial_hd(2));
>     sysbus_realize_and_unref(busdev, &error_fatal);
>     sysbus_mmio_map(busdev, 0, 0x10000000);
>

Re: [PATCH 6/9] hw/display/sm501: QOM-alias 'dma-offset' property in chipset object

[Philippe Mathieu-Daudé]

On 3/2/23 14:05, BALATON Zoltan wrote:
> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
>> No need to use an intermediate 'dma-offset' property in the
>> chipset object. Alias the property, so when the machine (here
>> r2d-plus) sets the value on the chipset, it is propagated to
>> the OHCI object.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/display/sm501.c | 22 +++++++++++-----------
>> hw/sh4/r2d.c       |  2 +-
>> 2 files changed, 12 insertions(+), 12 deletions(-)
> 
> It does not seem to be any simpler by the number of lines but maybe a 
> bit cleaner.

Well it also moves to the "Embed QOM objects" pattern which Peter prefers.
Note this device doesn't implement unrealize().

> I wonder if it would worth renaming the property to 
> dma-offset to match that of ohci so it's less confusing what it refers 
> to. It's only used by r2d and this patch already changing that so would 
> be an easy change.

We can't because TYPE_PCI_SM501 is user-creatable, so we need to
go thru the whole deprecation process and we don't have any API to
deprecate QOM properties yet.

I'll add these comments to the description.

Re: [PATCH 6/9] hw/display/sm501: QOM-alias 'dma-offset' property in chipset object

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
> On 3/2/23 14:05, BALATON Zoltan wrote:
>> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
>>> No need to use an intermediate 'dma-offset' property in the
>>> chipset object. Alias the property, so when the machine (here
>>> r2d-plus) sets the value on the chipset, it is propagated to
>>> the OHCI object.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> hw/display/sm501.c | 22 +++++++++++-----------
>>> hw/sh4/r2d.c       |  2 +-
>>> 2 files changed, 12 insertions(+), 12 deletions(-)
>> 
>> It does not seem to be any simpler by the number of lines but maybe a bit 
>> cleaner.
>
> Well it also moves to the "Embed QOM objects" pattern which Peter prefers.
> Note this device doesn't implement unrealize().

True. Maybe worth mentioning in the commit message to make this more 
explicit. I saw it in the patch but did not think about that.

>> I wonder if it would worth renaming the property to dma-offset to match 
>> that of ohci so it's less confusing what it refers to. It's only used by 
>> r2d and this patch already changing that so would be an easy change.
>
> We can't because TYPE_PCI_SM501 is user-creatable, so we need to
> go thru the whole deprecation process and we don't have any API to
> deprecate QOM properties yet.

But the sm501 PCI device only creates the display part hence it has no 
base option only vram-size (see sm501_pci_properties) so only the sysbus 
version has this property. Is this still a problem in that case?

Regards,
BALATON Zoltan

> I'll add these comments to the description.

Re: [PATCH 6/9] hw/display/sm501: QOM-alias 'dma-offset' property in chipset object

[Philippe Mathieu-Daudé]

On 3/2/23 14:50, BALATON Zoltan wrote:
> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
>> On 3/2/23 14:05, BALATON Zoltan wrote:
>>> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
>>>> No need to use an intermediate 'dma-offset' property in the
>>>> chipset object. Alias the property, so when the machine (here
>>>> r2d-plus) sets the value on the chipset, it is propagated to
>>>> the OHCI object.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> hw/display/sm501.c | 22 +++++++++++-----------
>>>> hw/sh4/r2d.c       |  2 +-
>>>> 2 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>> It does not seem to be any simpler by the number of lines but maybe a 
>>> bit cleaner.
>>
>> Well it also moves to the "Embed QOM objects" pattern which Peter 
>> prefers.
>> Note this device doesn't implement unrealize().
> 
> True. Maybe worth mentioning in the commit message to make this more 
> explicit. I saw it in the patch but did not think about that.
> 
>>> I wonder if it would worth renaming the property to dma-offset to 
>>> match that of ohci so it's less confusing what it refers to. It's 
>>> only used by r2d and this patch already changing that so would be an 
>>> easy change.
>>
>> We can't because TYPE_PCI_SM501 is user-creatable, so we need to
>> go thru the whole deprecation process and we don't have any API to
>> deprecate QOM properties yet.
> 
> But the sm501 PCI device only creates the display part hence it has no 
> base option only vram-size (see sm501_pci_properties) so only the sysbus 
> version has this property. Is this still a problem in that case?

Oh you are right, I misread the PCI/sysbus functions. Lucky me, thanks!

Re: [PATCH 6/9] hw/display/sm501: QOM-alias 'dma-offset' property in chipset object

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]

On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
> On 3/2/23 14:50, BALATON Zoltan wrote:
>> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
>>> On 3/2/23 14:05, BALATON Zoltan wrote:
>>>> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
>>>>> No need to use an intermediate 'dma-offset' property in the
>>>>> chipset object. Alias the property, so when the machine (here
>>>>> r2d-plus) sets the value on the chipset, it is propagated to
>>>>> the OHCI object.
>>>>> 
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>> hw/display/sm501.c | 22 +++++++++++-----------
>>>>> hw/sh4/r2d.c       |  2 +-
>>>>> 2 files changed, 12 insertions(+), 12 deletions(-)
>>>> 
>>>> It does not seem to be any simpler by the number of lines but maybe a bit 
>>>> cleaner.
>>> 
>>> Well it also moves to the "Embed QOM objects" pattern which Peter prefers.
>>> Note this device doesn't implement unrealize().
>> 
>> True. Maybe worth mentioning in the commit message to make this more 
>> explicit. I saw it in the patch but did not think about that.
>> 
>>>> I wonder if it would worth renaming the property to dma-offset to match 
>>>> that of ohci so it's less confusing what it refers to. It's only used by 
>>>> r2d and this patch already changing that so would be an easy change.
>>> 
>>> We can't because TYPE_PCI_SM501 is user-creatable, so we need to
>>> go thru the whole deprecation process and we don't have any API to
>>> deprecate QOM properties yet.
>> 
>> But the sm501 PCI device only creates the display part hence it has no base 
>> option only vram-size (see sm501_pci_properties) so only the sysbus version 
>> has this property. Is this still a problem in that case?
>
> Oh you are right, I misread the PCI/sysbus functions. Lucky me, thanks!

And I've just realized then we also don't need separate 
sm501_sysbus_properties[] and sm501_pci_properties[] so you can just keep 
one and call it sm501_properties.

Regards,
BALATON Zoltan

[PATCH 7/9] hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'

[Philippe Mathieu-Daudé]

DEFINE_PROP_DMAADDR() is only used once. Since it doesn't
add much value, simply remove it, along with the header
defining it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/usb/hcd-ohci-pci.c |  1 -
 hw/usb/hcd-ohci.c     |  3 +--
 include/hw/qdev-dma.h | 16 ----------------
 3 files changed, 1 insertion(+), 19 deletions(-)
 delete mode 100644 include/hw/qdev-dma.h

diff --git a/hw/usb/hcd-ohci-pci.c b/hw/usb/hcd-ohci-pci.c
index 6b630d35a7..92cc151264 100644
--- a/hw/usb/hcd-ohci-pci.c
+++ b/hw/usb/hcd-ohci-pci.c
@@ -25,7 +25,6 @@
 #include "migration/vmstate.h"
 #include "hw/pci/pci_device.h"
 #include "hw/sysbus.h"
-#include "hw/qdev-dma.h"
 #include "hw/qdev-properties.h"
 #include "trace.h"
 #include "hcd-ohci.h"
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 9d68036d23..26c377bf1b 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -33,7 +33,6 @@
 #include "hw/usb.h"
 #include "migration/vmstate.h"
 #include "hw/sysbus.h"
-#include "hw/qdev-dma.h"
 #include "hw/qdev-properties.h"
 #include "trace.h"
 #include "hcd-ohci.h"
@@ -2008,7 +2007,7 @@ static Property ohci_sysbus_properties[] = {
     DEFINE_PROP_STRING("masterbus", OHCISysBusState, masterbus),
     DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3),
     DEFINE_PROP_UINT32("firstport", OHCISysBusState, firstport, 0),
-    DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 0),
+    DEFINE_PROP_UINT64("dma-offset", OHCISysBusState, dma_offset, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/qdev-dma.h b/include/hw/qdev-dma.h
deleted file mode 100644
index b00391aa0c..0000000000
--- a/include/hw/qdev-dma.h
+++ /dev/null
@@ -1,16 +0,0 @@
-/*
- * Support for dma_addr_t typed properties
- *
- * Copyright (C) 2012 David Gibson, IBM Corporation.
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#ifndef HW_QDEV_DMA_H
-#define HW_QDEV_DMA_H
-
-#define DEFINE_PROP_DMAADDR(_n, _s, _f, _d)                               \
-    DEFINE_PROP_UINT64(_n, _s, _f, _d)
-
-#endif
-- 
2.38.1

[PATCH 8/9] hw/mips: Declare all length properties as unsigned

[Philippe Mathieu-Daudé]

Some length properties are signed, other unsigned:

  hw/mips/cps.c:183:    DEFINE_PROP_UINT32("num-vp", MIPSCPSState, num_vp, 1),
  hw/mips/cps.c:184:    DEFINE_PROP_UINT32("num-irq", MIPSCPSState, num_irq, 256),
  hw/misc/mips_cmgcr.c:215:    DEFINE_PROP_INT32("num-vp", MIPSGCRState, num_vps, 1),
  hw/misc/mips_cpc.c:167:    DEFINE_PROP_UINT32("num-vp", MIPSCPCState, num_vp, 0x1),
  hw/misc/mips_itu.c:552:    DEFINE_PROP_INT32("num-fifo", MIPSITUState, num_fifo,
  hw/misc/mips_itu.c:554:    DEFINE_PROP_INT32("num-semaphores", MIPSITUState,

Since negative values are not used (the minimum is '0'),
unify by declaring all properties as unsigned.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/mips_gic.c           |  4 ++--
 hw/mips/boston.c             |  2 +-
 hw/mips/cps.c                | 12 ++++++------
 hw/mips/malta.c              |  2 +-
 hw/misc/mips_cmgcr.c         |  2 +-
 hw/misc/mips_itu.c           |  4 ++--
 include/hw/intc/mips_gic.h   |  4 ++--
 include/hw/misc/mips_cmgcr.h |  2 +-
 include/hw/misc/mips_itu.h   |  4 ++--
 9 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/hw/intc/mips_gic.c b/hw/intc/mips_gic.c
index bda4549925..4bdc3b1bd1 100644
--- a/hw/intc/mips_gic.c
+++ b/hw/intc/mips_gic.c
@@ -439,8 +439,8 @@ static void mips_gic_realize(DeviceState *dev, Error **errp)
 }
 
 static Property mips_gic_properties[] = {
-    DEFINE_PROP_INT32("num-vp", MIPSGICState, num_vps, 1),
-    DEFINE_PROP_INT32("num-irq", MIPSGICState, num_irq, 256),
+    DEFINE_PROP_UINT32("num-vp", MIPSGICState, num_vps, 1),
+    DEFINE_PROP_UINT32("num-irq", MIPSGICState, num_irq, 256),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index a9d87f3437..21ad844519 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -702,7 +702,7 @@ static void boston_mach_init(MachineState *machine)
     object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS);
     object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type,
                             &error_fatal);
-    object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
+    object_property_set_uint(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
                             &error_fatal);
     qdev_connect_clock_in(DEVICE(&s->cps), "clk-in",
                           qdev_get_clock_out(dev, "cpu-refclk"));
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 2b436700ce..38acc57468 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -114,9 +114,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     /* Inter-Thread Communication Unit */
     if (itu_present) {
         object_initialize_child(OBJECT(dev), "itu", &s->itu, TYPE_MIPS_ITU);
-        object_property_set_int(OBJECT(&s->itu), "num-fifo", 16,
+        object_property_set_uint(OBJECT(&s->itu), "num-fifo", 16,
                                 &error_abort);
-        object_property_set_int(OBJECT(&s->itu), "num-semaphores", 16,
+        object_property_set_uint(OBJECT(&s->itu), "num-semaphores", 16,
                                 &error_abort);
         object_property_set_bool(OBJECT(&s->itu), "saar-present", saar_present,
                                  &error_abort);
@@ -133,7 +133,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
 
     /* Cluster Power Controller */
     object_initialize_child(OBJECT(dev), "cpc", &s->cpc, TYPE_MIPS_CPC);
-    object_property_set_int(OBJECT(&s->cpc), "num-vp", s->num_vp,
+    object_property_set_uint(OBJECT(&s->cpc), "num-vp", s->num_vp,
                             &error_abort);
     object_property_set_int(OBJECT(&s->cpc), "vp-start-running", 1,
                             &error_abort);
@@ -146,9 +146,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
 
     /* Global Interrupt Controller */
     object_initialize_child(OBJECT(dev), "gic", &s->gic, TYPE_MIPS_GIC);
-    object_property_set_int(OBJECT(&s->gic), "num-vp", s->num_vp,
+    object_property_set_uint(OBJECT(&s->gic), "num-vp", s->num_vp,
                             &error_abort);
-    object_property_set_int(OBJECT(&s->gic), "num-irq", 128,
+    object_property_set_uint(OBJECT(&s->gic), "num-irq", 128,
                             &error_abort);
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->gic), errp)) {
         return;
@@ -161,7 +161,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     gcr_base = env->CP0_CMGCRBase << 4;
 
     object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
-    object_property_set_int(OBJECT(&s->gcr), "num-vp", s->num_vp,
+    object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
                             &error_abort);
     object_property_set_int(OBJECT(&s->gcr), "gcr-rev", 0x800,
                             &error_abort);
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index ec172b111a..af9021316d 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1066,7 +1066,7 @@ static void create_cps(MachineState *ms, MaltaState *s,
     object_initialize_child(OBJECT(s), "cps", &s->cps, TYPE_MIPS_CPS);
     object_property_set_str(OBJECT(&s->cps), "cpu-type", ms->cpu_type,
                             &error_fatal);
-    object_property_set_int(OBJECT(&s->cps), "num-vp", ms->smp.cpus,
+    object_property_set_uint(OBJECT(&s->cps), "num-vp", ms->smp.cpus,
                             &error_fatal);
     qdev_connect_clock_in(DEVICE(&s->cps), "clk-in", s->cpuclk);
     sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal);
diff --git a/hw/misc/mips_cmgcr.c b/hw/misc/mips_cmgcr.c
index 3c8b37f700..66eb11662c 100644
--- a/hw/misc/mips_cmgcr.c
+++ b/hw/misc/mips_cmgcr.c
@@ -212,7 +212,7 @@ static const VMStateDescription vmstate_mips_gcr = {
 };
 
 static Property mips_gcr_properties[] = {
-    DEFINE_PROP_INT32("num-vp", MIPSGCRState, num_vps, 1),
+    DEFINE_PROP_UINT32("num-vp", MIPSGCRState, num_vps, 1),
     DEFINE_PROP_INT32("gcr-rev", MIPSGCRState, gcr_rev, 0x800),
     DEFINE_PROP_UINT64("gcr-base", MIPSGCRState, gcr_base, GCR_BASE_ADDR),
     DEFINE_PROP_LINK("gic", MIPSGCRState, gic_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/mips_itu.c b/hw/misc/mips_itu.c
index badef5c214..a06cdd10ea 100644
--- a/hw/misc/mips_itu.c
+++ b/hw/misc/mips_itu.c
@@ -549,9 +549,9 @@ static void mips_itu_reset(DeviceState *dev)
 }
 
 static Property mips_itu_properties[] = {
-    DEFINE_PROP_INT32("num-fifo", MIPSITUState, num_fifo,
+    DEFINE_PROP_UINT32("num-fifo", MIPSITUState, num_fifo,
                       ITC_FIFO_NUM_MAX),
-    DEFINE_PROP_INT32("num-semaphores", MIPSITUState, num_semaphores,
+    DEFINE_PROP_UINT32("num-semaphores", MIPSITUState, num_semaphores,
                       ITC_SEMAPH_NUM_MAX),
     DEFINE_PROP_BOOL("saar-present", MIPSITUState, saar_present, false),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/intc/mips_gic.h b/include/hw/intc/mips_gic.h
index eeb136e261..5e4c71edd4 100644
--- a/include/hw/intc/mips_gic.h
+++ b/include/hw/intc/mips_gic.h
@@ -211,8 +211,8 @@ struct MIPSGICState {
     /* GIC VP Timer */
     MIPSGICTimerState *gic_timer;
 
-    int32_t num_vps;
-    int32_t num_irq;
+    uint32_t num_vps;
+    uint32_t num_irq;
 };
 
 #endif /* MIPS_GIC_H */
diff --git a/include/hw/misc/mips_cmgcr.h b/include/hw/misc/mips_cmgcr.h
index 9fa58942d7..db4bf5f449 100644
--- a/include/hw/misc/mips_cmgcr.h
+++ b/include/hw/misc/mips_cmgcr.h
@@ -75,7 +75,7 @@ struct MIPSGCRState {
     SysBusDevice parent_obj;
 
     int32_t gcr_rev;
-    int32_t num_vps;
+    uint32_t num_vps;
     hwaddr gcr_base;
     MemoryRegion iomem;
     MemoryRegion *cpc_mr;
diff --git a/include/hw/misc/mips_itu.h b/include/hw/misc/mips_itu.h
index 50d961106d..ab6d286c38 100644
--- a/include/hw/misc/mips_itu.h
+++ b/include/hw/misc/mips_itu.h
@@ -57,8 +57,8 @@ struct MIPSITUState {
     SysBusDevice parent_obj;
     /*< public >*/
 
-    int32_t num_fifo;
-    int32_t num_semaphores;
+    uint32_t num_fifo;
+    uint32_t num_semaphores;
 
     /* ITC Storage */
     ITCStorageCell *cell;
-- 
2.38.1

[RFC PATCH 9/9] hw/mips/itu: Pass SAAR using QOM link property

[Philippe Mathieu-Daudé]

QOM objects shouldn't access each other internals fields
except using the QOM API.

mips_cps_realize() instantiates a TYPE_MIPS_ITU object, and
directly sets the 'saar' pointer:

   if (saar_present) {
       s->itu.saar = &env->CP0_SAAR;
   }

In order to avoid that, pass the MIPS_CPU object via a QOM
link property, and set the 'saar' pointer in mips_itu_realize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC because not tested.
---
 hw/mips/cps.c              | 23 ++++++-----------------
 hw/misc/mips_itu.c         | 26 ++++++++++++++++++--------
 include/hw/misc/mips_itu.h |  5 ++---
 3 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 38acc57468..2b5269ebf1 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -66,20 +66,17 @@ static bool cpu_mips_itu_supported(CPUMIPSState *env)
 static void mips_cps_realize(DeviceState *dev, Error **errp)
 {
     MIPSCPSState *s = MIPS_CPS(dev);
-    CPUMIPSState *env;
-    MIPSCPU *cpu;
-    int i;
     target_ulong gcr_base;
     bool itu_present = false;
-    bool saar_present = false;
 
     if (!clock_get(s->clock)) {
         error_setg(errp, "CPS input clock is not connected to an output clock");
         return;
     }
 
-    for (i = 0; i < s->num_vp; i++) {
-        cpu = MIPS_CPU(object_new(s->cpu_type));
+    for (int i = 0; i < s->num_vp; i++) {
+        MIPSCPU *cpu = MIPS_CPU(object_new(s->cpu_type));
+        CPUMIPSState *env = &cpu->env;
 
         /* All VPs are halted on reset. Leave powering up to CPC. */
         if (!object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
@@ -97,7 +94,6 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
         cpu_mips_irq_init_cpu(cpu);
         cpu_mips_clock_init(cpu);
 
-        env = &cpu->env;
         if (cpu_mips_itu_supported(env)) {
             itu_present = true;
             /* Attach ITC Tag to the VP */
@@ -107,22 +103,15 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
         qemu_register_reset(main_cpu_reset, cpu);
     }
 
-    cpu = MIPS_CPU(first_cpu);
-    env = &cpu->env;
-    saar_present = (bool)env->saarp;
-
     /* Inter-Thread Communication Unit */
     if (itu_present) {
         object_initialize_child(OBJECT(dev), "itu", &s->itu, TYPE_MIPS_ITU);
+        object_property_set_link(OBJECT(&s->itu), "cpu[0]",
+                                 OBJECT(first_cpu), &error_abort);
         object_property_set_uint(OBJECT(&s->itu), "num-fifo", 16,
                                 &error_abort);
         object_property_set_uint(OBJECT(&s->itu), "num-semaphores", 16,
                                 &error_abort);
-        object_property_set_bool(OBJECT(&s->itu), "saar-present", saar_present,
-                                 &error_abort);
-        if (saar_present) {
-            s->itu.saar = &env->CP0_SAAR;
-        }
         if (!sysbus_realize(SYS_BUS_DEVICE(&s->itu), errp)) {
             return;
         }
@@ -158,7 +147,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
                             sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gic), 0));
 
     /* Global Configuration Registers */
-    gcr_base = env->CP0_CMGCRBase << 4;
+    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
 
     object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
     object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
diff --git a/hw/misc/mips_itu.c b/hw/misc/mips_itu.c
index a06cdd10ea..0eda302db4 100644
--- a/hw/misc/mips_itu.c
+++ b/hw/misc/mips_itu.c
@@ -93,10 +93,10 @@ void itc_reconfigure(MIPSITUState *tag)
     uint64_t size = (1 * KiB) + (am[1] & ITC_AM1_ADDR_MASK_MASK);
     bool is_enabled = (am[0] & ITC_AM0_EN_MASK) != 0;
 
-    if (tag->saar_present) {
-        address = ((*(uint64_t *) tag->saar) & 0xFFFFFFFFE000ULL) << 4;
-        size = 1ULL << ((*(uint64_t *) tag->saar >> 1) & 0x1f);
-        is_enabled = *(uint64_t *) tag->saar & 1;
+    if (tag->saar) {
+        address = (tag->saar[0] & 0xFFFFFFFFE000ULL) << 4;
+        size = 1ULL << ((tag->saar[0] >> 1) & 0x1f);
+        is_enabled = tag->saar[0] & 1;
     }
 
     memory_region_transaction_begin();
@@ -157,7 +157,7 @@ static inline ITCView get_itc_view(hwaddr addr)
 static inline int get_cell_stride_shift(const MIPSITUState *s)
 {
     /* Minimum interval (for EntryGain = 0) is 128 B */
-    if (s->saar_present) {
+    if (s->saar) {
         return 7 + ((s->icr0 >> ITC_ICR0_BLK_GRAIN) &
                     ITC_ICR0_BLK_GRAIN_MASK);
     } else {
@@ -515,6 +515,7 @@ static void mips_itu_init(Object *obj)
 static void mips_itu_realize(DeviceState *dev, Error **errp)
 {
     MIPSITUState *s = MIPS_ITU(dev);
+    CPUMIPSState *env;
 
     if (s->num_fifo > ITC_FIFO_NUM_MAX) {
         error_setg(errp, "Exceed maximum number of FIFO cells: %d",
@@ -526,6 +527,15 @@ static void mips_itu_realize(DeviceState *dev, Error **errp)
                    s->num_semaphores);
         return;
     }
+    if (!s->cpu0) {
+        error_setg(errp, "Missing 'cpu[0]' property");
+        return;
+    }
+
+    env = &s->cpu0->env;
+    if (env->saarp) {
+        s->saar = env->CP0_SAAR;
+    }
 
     s->cell = g_new(ITCStorageCell, get_num_cells(s));
 }
@@ -534,8 +544,8 @@ static void mips_itu_reset(DeviceState *dev)
 {
     MIPSITUState *s = MIPS_ITU(dev);
 
-    if (s->saar_present) {
-        *(uint64_t *) s->saar = 0x11 << 1;
+    if (s->saar) {
+        s->saar[0] = 0x11 << 1;
         s->icr0 = get_num_cells(s) << ITC_ICR0_CELL_NUM;
     } else {
         s->ITCAddressMap[0] = 0;
@@ -553,7 +563,7 @@ static Property mips_itu_properties[] = {
                       ITC_FIFO_NUM_MAX),
     DEFINE_PROP_UINT32("num-semaphores", MIPSITUState, num_semaphores,
                       ITC_SEMAPH_NUM_MAX),
-    DEFINE_PROP_BOOL("saar-present", MIPSITUState, saar_present, false),
+    DEFINE_PROP_LINK("cpu[0]", MIPSITUState, cpu0, TYPE_MIPS_CPU, MIPSCPU *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/misc/mips_itu.h b/include/hw/misc/mips_itu.h
index ab6d286c38..35218b2d14 100644
--- a/include/hw/misc/mips_itu.h
+++ b/include/hw/misc/mips_itu.h
@@ -72,9 +72,8 @@ struct MIPSITUState {
     uint64_t icr0;
 
     /* SAAR */
-    bool saar_present;
-    void *saar;
-
+    uint64_t *saar;
+    MIPSCPU *cpu0;
 };
 
 /* Get ITC Configuration Tag memory region. */
-- 
2.38.1

Re: [RFC PATCH 9/9] hw/mips/itu: Pass SAAR using QOM link property

[Jiaxun Yang]

> 2023年2月3日 11:36，Philippe Mathieu-Daudé <philmd@linaro.org> 写道：
> 
> QOM objects shouldn't access each other internals fields
> except using the QOM API.
> 
> mips_cps_realize() instantiates a TYPE_MIPS_ITU object, and
> directly sets the 'saar' pointer:
> 
>   if (saar_present) {
>       s->itu.saar = &env->CP0_SAAR;
>   }
> 
> In order to avoid that, pass the MIPS_CPU object via a QOM
> link property, and set the 'saar' pointer in mips_itu_realize().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Tested-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

Tested with ITU cases.

> ---
> RFC because not tested.
> ---
> hw/mips/cps.c              | 23 ++++++-----------------
> hw/misc/mips_itu.c         | 26 ++++++++++++++++++--------
> include/hw/misc/mips_itu.h |  5 ++---
> 3 files changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 38acc57468..2b5269ebf1 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -66,20 +66,17 @@ static bool cpu_mips_itu_supported(CPUMIPSState *env)
> static void mips_cps_realize(DeviceState *dev, Error **errp)
> {
>     MIPSCPSState *s = MIPS_CPS(dev);
> -    CPUMIPSState *env;
> -    MIPSCPU *cpu;
> -    int i;
>     target_ulong gcr_base;
>     bool itu_present = false;
> -    bool saar_present = false;
> 
>     if (!clock_get(s->clock)) {
>         error_setg(errp, "CPS input clock is not connected to an output clock");
>         return;
>     }
> 
> -    for (i = 0; i < s->num_vp; i++) {
> -        cpu = MIPS_CPU(object_new(s->cpu_type));
> +    for (int i = 0; i < s->num_vp; i++) {
> +        MIPSCPU *cpu = MIPS_CPU(object_new(s->cpu_type));
> +        CPUMIPSState *env = &cpu->env;
> 
>         /* All VPs are halted on reset. Leave powering up to CPC. */
>         if (!object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
> @@ -97,7 +94,6 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>         cpu_mips_irq_init_cpu(cpu);
>         cpu_mips_clock_init(cpu);
> 
> -        env = &cpu->env;
>         if (cpu_mips_itu_supported(env)) {
>             itu_present = true;
>             /* Attach ITC Tag to the VP */
> @@ -107,22 +103,15 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>         qemu_register_reset(main_cpu_reset, cpu);
>     }
> 
> -    cpu = MIPS_CPU(first_cpu);
> -    env = &cpu->env;
> -    saar_present = (bool)env->saarp;
> -
>     /* Inter-Thread Communication Unit */
>     if (itu_present) {
>         object_initialize_child(OBJECT(dev), "itu", &s->itu, TYPE_MIPS_ITU);
> +        object_property_set_link(OBJECT(&s->itu), "cpu[0]",
> +                                 OBJECT(first_cpu), &error_abort);
>         object_property_set_uint(OBJECT(&s->itu), "num-fifo", 16,
>                                 &error_abort);
>         object_property_set_uint(OBJECT(&s->itu), "num-semaphores", 16,
>                                 &error_abort);
> -        object_property_set_bool(OBJECT(&s->itu), "saar-present", saar_present,
> -                                 &error_abort);
> -        if (saar_present) {
> -            s->itu.saar = &env->CP0_SAAR;
> -        }
>         if (!sysbus_realize(SYS_BUS_DEVICE(&s->itu), errp)) {
>             return;
>         }
> @@ -158,7 +147,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>                             sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gic), 0));
> 
>     /* Global Configuration Registers */
> -    gcr_base = env->CP0_CMGCRBase << 4;
> +    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
> 
>     object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
>     object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
> diff --git a/hw/misc/mips_itu.c b/hw/misc/mips_itu.c
> index a06cdd10ea..0eda302db4 100644
> --- a/hw/misc/mips_itu.c
> +++ b/hw/misc/mips_itu.c
> @@ -93,10 +93,10 @@ void itc_reconfigure(MIPSITUState *tag)
>     uint64_t size = (1 * KiB) + (am[1] & ITC_AM1_ADDR_MASK_MASK);
>     bool is_enabled = (am[0] & ITC_AM0_EN_MASK) != 0;
> 
> -    if (tag->saar_present) {
> -        address = ((*(uint64_t *) tag->saar) & 0xFFFFFFFFE000ULL) << 4;
> -        size = 1ULL << ((*(uint64_t *) tag->saar >> 1) & 0x1f);
> -        is_enabled = *(uint64_t *) tag->saar & 1;
> +    if (tag->saar) {
> +        address = (tag->saar[0] & 0xFFFFFFFFE000ULL) << 4;
> +        size = 1ULL << ((tag->saar[0] >> 1) & 0x1f);
> +        is_enabled = tag->saar[0] & 1;
>     }
> 
>     memory_region_transaction_begin();
> @@ -157,7 +157,7 @@ static inline ITCView get_itc_view(hwaddr addr)
> static inline int get_cell_stride_shift(const MIPSITUState *s)
> {
>     /* Minimum interval (for EntryGain = 0) is 128 B */
> -    if (s->saar_present) {
> +    if (s->saar) {
>         return 7 + ((s->icr0 >> ITC_ICR0_BLK_GRAIN) &
>                     ITC_ICR0_BLK_GRAIN_MASK);
>     } else {
> @@ -515,6 +515,7 @@ static void mips_itu_init(Object *obj)
> static void mips_itu_realize(DeviceState *dev, Error **errp)
> {
>     MIPSITUState *s = MIPS_ITU(dev);
> +    CPUMIPSState *env;
> 
>     if (s->num_fifo > ITC_FIFO_NUM_MAX) {
>         error_setg(errp, "Exceed maximum number of FIFO cells: %d",
> @@ -526,6 +527,15 @@ static void mips_itu_realize(DeviceState *dev, Error **errp)
>                    s->num_semaphores);
>         return;
>     }
> +    if (!s->cpu0) {
> +        error_setg(errp, "Missing 'cpu[0]' property");
> +        return;
> +    }
> +
> +    env = &s->cpu0->env;
> +    if (env->saarp) {
> +        s->saar = env->CP0_SAAR;
> +    }
> 
>     s->cell = g_new(ITCStorageCell, get_num_cells(s));
> }
> @@ -534,8 +544,8 @@ static void mips_itu_reset(DeviceState *dev)
> {
>     MIPSITUState *s = MIPS_ITU(dev);
> 
> -    if (s->saar_present) {
> -        *(uint64_t *) s->saar = 0x11 << 1;
> +    if (s->saar) {
> +        s->saar[0] = 0x11 << 1;
>         s->icr0 = get_num_cells(s) << ITC_ICR0_CELL_NUM;
>     } else {
>         s->ITCAddressMap[0] = 0;
> @@ -553,7 +563,7 @@ static Property mips_itu_properties[] = {
>                       ITC_FIFO_NUM_MAX),
>     DEFINE_PROP_UINT32("num-semaphores", MIPSITUState, num_semaphores,
>                       ITC_SEMAPH_NUM_MAX),
> -    DEFINE_PROP_BOOL("saar-present", MIPSITUState, saar_present, false),
> +    DEFINE_PROP_LINK("cpu[0]", MIPSITUState, cpu0, TYPE_MIPS_CPU, MIPSCPU *),
>     DEFINE_PROP_END_OF_LIST(),
> };
> 
> diff --git a/include/hw/misc/mips_itu.h b/include/hw/misc/mips_itu.h
> index ab6d286c38..35218b2d14 100644
> --- a/include/hw/misc/mips_itu.h
> +++ b/include/hw/misc/mips_itu.h
> @@ -72,9 +72,8 @@ struct MIPSITUState {
>     uint64_t icr0;
> 
>     /* SAAR */
> -    bool saar_present;
> -    void *saar;
> -
> +    uint64_t *saar;
> +    MIPSCPU *cpu0;
> };
> 
> /* Get ITC Configuration Tag memory region. */
> -- 
> 2.38.1
> 

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

[Mark Cave-Ayland]

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.

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

[Philippe Mathieu-Daudé]

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

[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.

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

[Philippe Mathieu-Daudé]

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", &s->uart0, TYPE_UART);
>         object_initialize_child(obj, "uart1", &s->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, &s->eth[i], TYPE_IMX_ENET);
+        qdev_prop_set_uint32(DEVICE(&s->eth[i]), "phy-num", i);
+        object_property_add_alias(obj, propname,
+                                  OBJECT(&s->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 bus
is involved...

> 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).

Hmm I see, but this seems a very restrictive use of QOM link property
concept IMHO. For me a QOM link allows to share pointer to any QOM
object (with the QOM type checked). Am I abusing the concept?

BTW DEFINE_PROP_xxx() macros are a QDev concept. In particular
DEFINE_PROP_LINK(). The 'realize' step is also a QDev concept...

Markus suggested I watch Paolo's QOM talk to use the standard
terminology from the expert. I suppose this is "QOM exegesis and
apocalypse" from 2014.

Thanks for the brainstorming and clarifications!

Phil.

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

[Mark Cave-Ayland]

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", &s->uart0, TYPE_UART);
>>         object_initialize_child(obj, "uart1", &s->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, &s->eth[i], TYPE_IMX_ENET);
> +        qdev_prop_set_uint32(DEVICE(&s->eth[i]), "phy-num", i);
> +        object_property_add_alias(obj, propname,
> +                                  OBJECT(&s->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 work (think PCI IRQs for example).
> 
> IRQ wiring is an unsolved problem in my TODO, in particular when a bus
> is involved...
> 
>> 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).
> 
> Hmm I see, but this seems a very restrictive use of QOM link property
> concept IMHO. For me a QOM link allows to share pointer to any QOM
> object (with the QOM type checked). Am I abusing the concept?
> 
> BTW DEFINE_PROP_xxx() macros are a QDev concept. In particular
> DEFINE_PROP_LINK(). The 'realize' step is also a QDev concept...

I thought we were talking alias properties here? I'm sure I replied separately to the 
RFC thread containing qdev_set_prop_link() - nothing wrong with link properties at 
all, but exposing them via qdev was only done recently by Markus (to avoid around 
passing opaques IIRC) and I'm not sure how that would be represented/parsed on the 
command line, if indeed that is still considered to be the way forward.

> Markus suggested I watch Paolo's QOM talk to use the standard
> terminology from the expert. I suppose this is "QOM exegesis and
> apocalypse" from 2014.
> 
> Thanks for the brainstorming and clarifications!

No problem, hopefully by starting to document some of these issues it will help 
decided the future direction as to how this should all work.


ATB,

Mark.