[Qemu-devel] [RFC PATCH 0/6] qom: introduce TypeInfo name aliases

[Qemu-devel] [RFC PATCH 0/6] qom: introduce TypeInfo name aliases

[Philippe Mathieu-Daudé]

Hi,

This RFC series is intended to simplify Flattened Device Tree support,
in particular the 'compatible' FDT entry, when Linux names mismatches
QEMU ones, but this is the same device modelled.

Eventually this might help to remove the QDevAlias qdev_alias_table[]
in qdev-monitor.c.

So far this is only a 'proof of concept'.
To see how the qtests perform, I only modified 3 devices, 2 used by the
Xilinx Zynq machines (Cadence), and the e1000 (used by the PXE test).

Regards,

Phil.

Philippe Mathieu-Daudé (6):
  qom: introduce TypeInfo name aliases
  hw/net/e1000: real device name is 'e1000-82540em', 'e1000' is an alias
  hw/char/cadence_uart: add FDT aliases
  arm/xlnx-zynq: use FDT names for the Cadence UART
  hw/net/cadence_gem: add FDT names as alias
  hw/arm/xlnx-zynq: use FDT names for the Cadence GEM

 include/qom/object.h   |  3 +++
 hw/arm/xilinx_zynq.c   |  2 ++
 hw/arm/xlnx-zynqmp.c   |  4 ++--
 hw/char/cadence_uart.c |  7 +++++++
 hw/net/cadence_gem.c   |  6 ++++++
 hw/net/e1000.c         |  5 ++++-
 qom/object.c           | 18 ++++++++++++++++--
 7 files changed, 40 insertions(+), 5 deletions(-)

-- 
2.15.1

[Qemu-devel] [RFC PATCH 1/6] qom: introduce TypeInfo name aliases

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qom/object.h |  3 +++
 qom/object.c         | 18 ++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index dc73d59660..5d6e8795d4 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -428,6 +428,8 @@ struct Object
  * TypeInfo:
  * @name: The name of the type.
  * @parent: The name of the parent type.
+ * @aliases: A list of alias names for the type. This should point to a static
+ *   array that's terminated with a zero filled element.
  * @instance_size: The size of the object (derivative of #Object).  If
  *   @instance_size is 0, then the size of the object will be the size of the
  *   parent object.
@@ -468,6 +470,7 @@ struct TypeInfo
 {
     const char *name;
     const char *parent;
+    const char **aliases;
 
     size_t instance_size;
     void (*instance_init)(Object *obj);
diff --git a/qom/object.c b/qom/object.c
index c58c52d518..c532a8aa65 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -83,10 +83,15 @@ static GHashTable *type_table_get(void)
 
 static bool enumerating_types;
 
-static void type_table_add(TypeImpl *ti)
+static void type_table_add_with_key(TypeImpl *ti, const char *key_name)
 {
     assert(!enumerating_types);
-    g_hash_table_insert(type_table_get(), (void *)ti->name, ti);
+    g_hash_table_insert(type_table_get(), (void *)key_name, ti);
+}
+
+static void type_table_add(TypeImpl *ti)
+{
+    type_table_add_with_key(ti, ti->name);
 }
 
 static TypeImpl *type_table_lookup(const char *name)
@@ -137,6 +142,15 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
     ti = type_new(info);
 
     type_table_add(ti);
+
+    if (info->aliases) {
+        int i;
+
+        for (i = 0; info->aliases[i]; i++) {
+            type_table_add_with_key(ti, info->aliases[i]);
+        }
+    }
+
     return ti;
 }
 
-- 
2.15.1

[Qemu-devel] [RFC PATCH 2/6] hw/net/e1000: real device name is 'e1000-82540em', 'e1000' is an alias

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/e1000.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 05a00cba31..2280f7fdf9 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1648,6 +1648,7 @@ typedef struct E1000Info {
     uint16_t   device_id;
     uint8_t    revision;
     uint16_t   phy_id2;
+    const char **aliases;
 } E1000Info;
 
 static void e1000_class_init(ObjectClass *klass, void *data)
@@ -1695,10 +1696,11 @@ static const TypeInfo e1000_base_info = {
 
 static const E1000Info e1000_devices[] = {
     {
-        .name      = "e1000",
+        .name      = "e1000-82540em",
         .device_id = E1000_DEV_ID_82540EM,
         .revision  = 0x03,
         .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
+        .aliases   = (const char * []) {"e1000", NULL},
     },
     {
         .name      = "e1000-82544gc",
@@ -1725,6 +1727,7 @@ static void e1000_register_types(void)
 
         type_info.name = info->name;
         type_info.parent = TYPE_E1000_BASE;
+        type_info.aliases = info->aliases;
         type_info.class_data = (void *)info;
         type_info.class_init = e1000_class_init;
         type_info.instance_init = e1000_instance_init;
-- 
2.15.1

[Qemu-devel] [RFC PATCH 3/6] hw/char/cadence_uart: add FDT aliases

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/cadence_uart.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 6143494060..6880827947 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -552,6 +552,13 @@ static void cadence_uart_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo cadence_uart_info = {
     .name          = TYPE_CADENCE_UART,
+    .aliases       = (const char * []) {
+                        "cdns,uart-r1p8",
+                        "xlnx,xuartps",     /* Zynq-7xxx SoC */
+                        "cdns,uart-r1p12",
+                        "xlnx,zynqmp-uart", /* Zynq Ultrascale+ MPSoC */
+                        NULL
+                     },
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(CadenceUARTState),
     .instance_init = cadence_uart_init,
-- 
2.15.1

[Qemu-devel] [RFC PATCH 4/6] arm/xlnx-zynq: use FDT names for the Cadence UART

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/xilinx_zynq.c | 1 +
 hw/arm/xlnx-zynqmp.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1836a4ed45..c45c364583 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -236,6 +236,7 @@ static void zynq_init(MachineState *machine)
     sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
     sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
 
+    /* "xlnx,xuartps" */
     cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hds[0]);
     cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hds[1]);
 
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 325642058b..38f038786c 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -155,7 +155,7 @@ static void xlnx_zynqmp_init(Object *obj)
     }
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_UARTS; i++) {
-        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
+        object_initialize(&s->uart[i], sizeof(s->uart[i]), "xlnx,zynqmp-uart");
         qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
     }
 
-- 
2.15.1

[Qemu-devel] [RFC PATCH 5/6] hw/net/cadence_gem: add FDT names as alias

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/cadence_gem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 3943187572..b7b4540d91 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1544,6 +1544,12 @@ static void gem_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo gem_info = {
     .name  = TYPE_CADENCE_GEM,
+    .aliases       = (const char * []) {
+                        "cdns,gem",
+                        "cdns,zynq-gem",    /* Zynq-7xxx SoC */
+                        "cdns,zynqmp-gem",  /* Zynq Ultrascale+ MPSoC */
+                        NULL
+                     },
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size  = sizeof(CadenceGEMState),
     .instance_init = gem_init,
-- 
2.15.1

[Qemu-devel] [RFC PATCH 6/6] hw/arm/xlnx-zynq: use FDT names for the Cadence GEM

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/xilinx_zynq.c | 1 +
 hw/arm/xlnx-zynqmp.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index c45c364583..5ae18921c1 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -245,6 +245,7 @@ static void zynq_init(MachineState *machine)
     sysbus_create_varargs("cadence_ttc", 0xF8002000,
             pic[69-IRQ_OFFSET], pic[70-IRQ_OFFSET], pic[71-IRQ_OFFSET], NULL);
 
+    /* cdns,gem */
     gem_init(&nd_table[0], 0xE000B000, pic[54-IRQ_OFFSET]);
     gem_init(&nd_table[1], 0xE000C000, pic[77-IRQ_OFFSET]);
 
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 38f038786c..1aed89707c 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -150,7 +150,7 @@ static void xlnx_zynqmp_init(Object *obj)
     qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
-        object_initialize(&s->gem[i], sizeof(s->gem[i]), TYPE_CADENCE_GEM);
+        object_initialize(&s->gem[i], sizeof(s->gem[i]), "cdns,zynqmp-gem");
         qdev_set_parent_bus(DEVICE(&s->gem[i]), sysbus_get_default());
     }
 
-- 
2.15.1

Re: [Qemu-devel] [RFC PATCH 2/6] hw/net/e1000: real device name is 'e1000-82540em', 'e1000' is an alias

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/net/e1000.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 05a00cba31..2280f7fdf9 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1648,6 +1648,7 @@ typedef struct E1000Info {
>      uint16_t   device_id;
>      uint8_t    revision;
>      uint16_t   phy_id2;
> +    const char **aliases;
>  } E1000Info;
>  
>  static void e1000_class_init(ObjectClass *klass, void *data)
> @@ -1695,10 +1696,11 @@ static const TypeInfo e1000_base_info = {
>  
>  static const E1000Info e1000_devices[] = {
>      {
> -        .name      = "e1000",
> +        .name      = "e1000-82540em",
>          .device_id = E1000_DEV_ID_82540EM,
>          .revision  = 0x03,
>          .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
> +        .aliases   = (const char * []) {"e1000", NULL},
>      },
>      {
>          .name      = "e1000-82544gc",
> @@ -1725,6 +1727,7 @@ static void e1000_register_types(void)
>  
>          type_info.name = info->name;
>          type_info.parent = TYPE_E1000_BASE;
> +        type_info.aliases = info->aliases;
>          type_info.class_data = (void *)info;
>          type_info.class_init = e1000_class_init;
>          type_info.instance_init = e1000_instance_init;

Can you just check that this doesn't break migration compatibility
either way please;  I think there's some alias code somewhere but I
can't remember the details.

One thing I do remember that broke when it previously changed was
entries in PC_COMPAT_* tables like the one in hw/i386/pc_piix.c's
PC_COMPAT_1_3 - so check with an 'info qtree' that the property is OK.

Dave

> -- 
> 2.15.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH 2/6] hw/net/e1000: real device name is 'e1000-82540em', 'e1000' is an alias

[Philippe Mathieu-Daudé]

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

Hi David,

On 01/04/2018 01:24 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/net/e1000.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index 05a00cba31..2280f7fdf9 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -1648,6 +1648,7 @@ typedef struct E1000Info {
>>      uint16_t   device_id;
>>      uint8_t    revision;
>>      uint16_t   phy_id2;
>> +    const char **aliases;
>>  } E1000Info;
>>  
>>  static void e1000_class_init(ObjectClass *klass, void *data)
>> @@ -1695,10 +1696,11 @@ static const TypeInfo e1000_base_info = {
>>  
>>  static const E1000Info e1000_devices[] = {
>>      {
>> -        .name      = "e1000",
>> +        .name      = "e1000-82540em",
>>          .device_id = E1000_DEV_ID_82540EM,
>>          .revision  = 0x03,
>>          .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
>> +        .aliases   = (const char * []) {"e1000", NULL},
>>      },
>>      {
>>          .name      = "e1000-82544gc",
>> @@ -1725,6 +1727,7 @@ static void e1000_register_types(void)
>>  
>>          type_info.name = info->name;
>>          type_info.parent = TYPE_E1000_BASE;
>> +        type_info.aliases = info->aliases;
>>          type_info.class_data = (void *)info;
>>          type_info.class_init = e1000_class_init;
>>          type_info.instance_init = e1000_instance_init;
> 
> Can you just check that this doesn't break migration compatibility
> either way please;  I think there's some alias code somewhere but I
> can't remember the details.

Good point.

Something related to qdev-monitor.c?

> One thing I do remember that broke when it previously changed was
> entries in PC_COMPAT_* tables like the one in hw/i386/pc_piix.c's
> PC_COMPAT_1_3 - so check with an 'info qtree' that the property is OK.

Ok, I'll verify and think about a qtest for that.

I took this device because I wanted to test another arch than ARM (which
strongly use FDT).
If there is an issue migrating this particular device we can drop this
patch for this only device, and the series still stands for the ARM/FDT.

Regards,

Phil.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [RFC PATCH 2/6] hw/net/e1000: real device name is 'e1000-82540em', 'e1000' is an alias

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> Hi David,
> 
> On 01/04/2018 01:24 PM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>  hw/net/e1000.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> >> index 05a00cba31..2280f7fdf9 100644
> >> --- a/hw/net/e1000.c
> >> +++ b/hw/net/e1000.c
> >> @@ -1648,6 +1648,7 @@ typedef struct E1000Info {
> >>      uint16_t   device_id;
> >>      uint8_t    revision;
> >>      uint16_t   phy_id2;
> >> +    const char **aliases;
> >>  } E1000Info;
> >>  
> >>  static void e1000_class_init(ObjectClass *klass, void *data)
> >> @@ -1695,10 +1696,11 @@ static const TypeInfo e1000_base_info = {
> >>  
> >>  static const E1000Info e1000_devices[] = {
> >>      {
> >> -        .name      = "e1000",
> >> +        .name      = "e1000-82540em",
> >>          .device_id = E1000_DEV_ID_82540EM,
> >>          .revision  = 0x03,
> >>          .phy_id2   = E1000_PHY_ID2_8254xx_DEFAULT,
> >> +        .aliases   = (const char * []) {"e1000", NULL},
> >>      },
> >>      {
> >>          .name      = "e1000-82544gc",
> >> @@ -1725,6 +1727,7 @@ static void e1000_register_types(void)
> >>  
> >>          type_info.name = info->name;
> >>          type_info.parent = TYPE_E1000_BASE;
> >> +        type_info.aliases = info->aliases;
> >>          type_info.class_data = (void *)info;
> >>          type_info.class_init = e1000_class_init;
> >>          type_info.instance_init = e1000_instance_init;
> > 
> > Can you just check that this doesn't break migration compatibility
> > either way please;  I think there's some alias code somewhere but I
> > can't remember the details.
> 
> Good point.
> 
> Something related to qdev-monitor.c?

The thing I was worrying about was when names end up in the migration
stream;  that can be a bit subtle so the only way is to just check it
doesn't break.

> > One thing I do remember that broke when it previously changed was
> > entries in PC_COMPAT_* tables like the one in hw/i386/pc_piix.c's
> > PC_COMPAT_1_3 - so check with an 'info qtree' that the property is OK.
> 
> Ok, I'll verify and think about a qtest for that.
> 
> I took this device because I wanted to test another arch than ARM (which
> strongly use FDT).
> If there is an issue migrating this particular device we can drop this
> patch for this only device, and the series still stands for the ARM/FDT.

Yes, although my concern was more general, I just had a previous case
where I had to fix e1000 downstream because of renaming; but just keep
in mind other places where names turn up, like the compatibility lists.

Dave

> Regards,
> 
> Phil.
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH 0/6] qom: introduce TypeInfo name aliases

[Eduardo Habkost]

On Thu, Jan 04, 2018 at 11:40:40AM -0300, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This RFC series is intended to simplify Flattened Device Tree support,
> in particular the 'compatible' FDT entry, when Linux names mismatches
> QEMU ones, but this is the same device modelled.
> 
> Eventually this might help to remove the QDevAlias qdev_alias_table[]
> in qdev-monitor.c.
> 

Didn't look closely at the patches yet, but this sounds like a
nice generic way to replace other alias systems.  We have at
least:

* qdev-monitor.c: qdev_alias_table[] (as mentioned above)
* chardev/char.c: chardev_alias_table[]
* target/alpha/cpu.c: alpha_cpu_aliases[]
* target/ppc/cpu-models.c: ppc_cpu_aliases[]
* include/hw/boards.h: MachineClass::alias

Probably there are others I couldn't find.


> So far this is only a 'proof of concept'.
> To see how the qtests perform, I only modified 3 devices, 2 used by the
> Xilinx Zynq machines (Cadence), and the e1000 (used by the PXE test).
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (6):
>   qom: introduce TypeInfo name aliases
>   hw/net/e1000: real device name is 'e1000-82540em', 'e1000' is an alias
>   hw/char/cadence_uart: add FDT aliases
>   arm/xlnx-zynq: use FDT names for the Cadence UART
>   hw/net/cadence_gem: add FDT names as alias
>   hw/arm/xlnx-zynq: use FDT names for the Cadence GEM
> 
>  include/qom/object.h   |  3 +++
>  hw/arm/xilinx_zynq.c   |  2 ++
>  hw/arm/xlnx-zynqmp.c   |  4 ++--
>  hw/char/cadence_uart.c |  7 +++++++
>  hw/net/cadence_gem.c   |  6 ++++++
>  hw/net/e1000.c         |  5 ++++-
>  qom/object.c           | 18 ++++++++++++++++--
>  7 files changed, 40 insertions(+), 5 deletions(-)
> 
> -- 
> 2.15.1
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 3/6] hw/char/cadence_uart: add FDT aliases

[Alistair Francis]

On Thu, Jan 4, 2018 at 6:40 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> ---
>  hw/char/cadence_uart.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 6143494060..6880827947 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -552,6 +552,13 @@ static void cadence_uart_class_init(ObjectClass *klass, void *data)
>
>  static const TypeInfo cadence_uart_info = {
>      .name          = TYPE_CADENCE_UART,
> +    .aliases       = (const char * []) {
> +                        "cdns,uart-r1p8",
> +                        "xlnx,xuartps",     /* Zynq-7xxx SoC */
> +                        "cdns,uart-r1p12",
> +                        "xlnx,zynqmp-uart", /* Zynq Ultrascale+ MPSoC */
> +                        NULL
> +                     },
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(CadenceUARTState),
>      .instance_init = cadence_uart_init,
> --
> 2.15.1
>
>

Re: [Qemu-devel] [RFC PATCH 4/6] arm/xlnx-zynq: use FDT names for the Cadence UART

[Alistair Francis]

On Thu, Jan 4, 2018 at 6:40 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> ---
>  hw/arm/xilinx_zynq.c | 1 +
>  hw/arm/xlnx-zynqmp.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 1836a4ed45..c45c364583 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -236,6 +236,7 @@ static void zynq_init(MachineState *machine)
>      sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
>      sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
>
> +    /* "xlnx,xuartps" */
>      cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hds[0]);
>      cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hds[1]);
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 325642058b..38f038786c 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -155,7 +155,7 @@ static void xlnx_zynqmp_init(Object *obj)
>      }
>
>      for (i = 0; i < XLNX_ZYNQMP_NUM_UARTS; i++) {
> -        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
> +        object_initialize(&s->uart[i], sizeof(s->uart[i]), "xlnx,zynqmp-uart");
>          qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
>      }
>
> --
> 2.15.1
>
>

Re: [Qemu-devel] [RFC PATCH 5/6] hw/net/cadence_gem: add FDT names as alias

[Alistair Francis]

On Thu, Jan 4, 2018 at 6:40 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> ---
>  hw/net/cadence_gem.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 3943187572..b7b4540d91 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -1544,6 +1544,12 @@ static void gem_class_init(ObjectClass *klass, void *data)
>
>  static const TypeInfo gem_info = {
>      .name  = TYPE_CADENCE_GEM,
> +    .aliases       = (const char * []) {
> +                        "cdns,gem",
> +                        "cdns,zynq-gem",    /* Zynq-7xxx SoC */
> +                        "cdns,zynqmp-gem",  /* Zynq Ultrascale+ MPSoC */
> +                        NULL
> +                     },
>      .parent = TYPE_SYS_BUS_DEVICE,
>      .instance_size  = sizeof(CadenceGEMState),
>      .instance_init = gem_init,
> --
> 2.15.1
>
>

Re: [Qemu-devel] [RFC PATCH 6/6] hw/arm/xlnx-zynq: use FDT names for the Cadence GEM

[Alistair Francis]

On Thu, Jan 4, 2018 at 6:40 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> ---
>  hw/arm/xilinx_zynq.c | 1 +
>  hw/arm/xlnx-zynqmp.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index c45c364583..5ae18921c1 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -245,6 +245,7 @@ static void zynq_init(MachineState *machine)
>      sysbus_create_varargs("cadence_ttc", 0xF8002000,
>              pic[69-IRQ_OFFSET], pic[70-IRQ_OFFSET], pic[71-IRQ_OFFSET], NULL);
>
> +    /* cdns,gem */
>      gem_init(&nd_table[0], 0xE000B000, pic[54-IRQ_OFFSET]);
>      gem_init(&nd_table[1], 0xE000C000, pic[77-IRQ_OFFSET]);
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 38f038786c..1aed89707c 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -150,7 +150,7 @@ static void xlnx_zynqmp_init(Object *obj)
>      qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
>
>      for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
> -        object_initialize(&s->gem[i], sizeof(s->gem[i]), TYPE_CADENCE_GEM);
> +        object_initialize(&s->gem[i], sizeof(s->gem[i]), "cdns,zynqmp-gem");
>          qdev_set_parent_bus(DEVICE(&s->gem[i]), sysbus_get_default());
>      }
>
> --
> 2.15.1
>
>

Re: [Qemu-devel] [RFC PATCH 0/6] qom: introduce TypeInfo name aliases

[Igor Mammedov]

On Thu, 4 Jan 2018 17:22:03 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jan 04, 2018 at 11:40:40AM -0300, Philippe Mathieu-Daudé wrote:
> > Hi,
> > 
> > This RFC series is intended to simplify Flattened Device Tree support,
> > in particular the 'compatible' FDT entry, when Linux names mismatches
> > QEMU ones, but this is the same device modelled.
> > 
> > Eventually this might help to remove the QDevAlias qdev_alias_table[]
> > in qdev-monitor.c.
> >   
> 
> Didn't look closely at the patches yet, but this sounds like a
> nice generic way to replace other alias systems.  We have at
> least:
Though it seems easy and trivial, I'm a bit concerned about using
QOM types for the task though.
Also see commit 6acbe4c6f which labels aliases as a bad idea
and says that they are there only for compatibility and shouldn't
be used.
So far I agree with that statement, because it introduces
ambiguity in code used internally and more worrying is that
this ambiguity will increase user visible ABI (think of '-device_add FOO_ALIAS')
that we would need to maintain afterwards.
It would be nice to have unified alias API, but I think it should
be separate one and limited to the same scope (i.e. compat stuff),
and even that won't be easy as different alias impl. we have now
have a different needs.

wrt this series targeted usage, I'd prefer that object_new/initialize
would use real type names when creating devices as it does currently

FDT linux guest specific names wouldn't seep into device model
itself. Firmware (FDT or ACPI) should be separate from device
implementation.

If really there is need to dynamically scan present devices
and build FDT from result, then probably we should introduce
interface that devices could implement if necessary.
(I was thinking about such possibility for ACPI). But so far
it looked to me as too much overhead for what we do now.


> * qdev-monitor.c: qdev_alias_table[] (as mentioned above)
> * chardev/char.c: chardev_alias_table[]
> * target/alpha/cpu.c: alpha_cpu_aliases[]
> * target/ppc/cpu-models.c: ppc_cpu_aliases[]
> * include/hw/boards.h: MachineClass::alias
> 
> Probably there are others I couldn't find.
> 
> 
> > So far this is only a 'proof of concept'.
> > To see how the qtests perform, I only modified 3 devices, 2 used by the
> > Xilinx Zynq machines (Cadence), and the e1000 (used by the PXE test).
> > 
> > Regards,
> > 
> > Phil.
> > 
> > Philippe Mathieu-Daudé (6):
> >   qom: introduce TypeInfo name aliases
> >   hw/net/e1000: real device name is 'e1000-82540em', 'e1000' is an alias
> >   hw/char/cadence_uart: add FDT aliases
> >   arm/xlnx-zynq: use FDT names for the Cadence UART
> >   hw/net/cadence_gem: add FDT names as alias
> >   hw/arm/xlnx-zynq: use FDT names for the Cadence GEM
> > 
> >  include/qom/object.h   |  3 +++
> >  hw/arm/xilinx_zynq.c   |  2 ++
> >  hw/arm/xlnx-zynqmp.c   |  4 ++--
> >  hw/char/cadence_uart.c |  7 +++++++
> >  hw/net/cadence_gem.c   |  6 ++++++
> >  hw/net/e1000.c         |  5 ++++-
> >  qom/object.c           | 18 ++++++++++++++++--
> >  7 files changed, 40 insertions(+), 5 deletions(-)
> > 
> > -- 
> > 2.15.1
> >   
> 

Re: [Qemu-devel] [RFC PATCH 4/6] arm/xlnx-zynq: use FDT names for the Cadence UART

[Igor Mammedov]

On Thu,  4 Jan 2018 11:40:44 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/xilinx_zynq.c | 1 +
>  hw/arm/xlnx-zynqmp.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 1836a4ed45..c45c364583 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -236,6 +236,7 @@ static void zynq_init(MachineState *machine)
>      sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
>      sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
>  
> +    /* "xlnx,xuartps" */
>      cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hds[0]);
>      cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hds[1]);
>  
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 325642058b..38f038786c 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -155,7 +155,7 @@ static void xlnx_zynqmp_init(Object *obj)
>      }
>  
>      for (i = 0; i < XLNX_ZYNQMP_NUM_UARTS; i++) {
> -        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
> +        object_initialize(&s->uart[i], sizeof(s->uart[i]), "xlnx,zynqmp-uart");
I don't think that commas are valid symbol in type names
(if I recall correctly it should be letters, numbers and '-')

>          qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
>      }
>  

Re: [Qemu-devel] [RFC PATCH 4/6] arm/xlnx-zynq: use FDT names for the Cadence UART

[Thomas Huth]

On 08.01.2018 13:54, Igor Mammedov wrote:
> On Thu,  4 Jan 2018 11:40:44 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/xilinx_zynq.c | 1 +
>>  hw/arm/xlnx-zynqmp.c | 2 +-
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>> index 1836a4ed45..c45c364583 100644
>> --- a/hw/arm/xilinx_zynq.c
>> +++ b/hw/arm/xilinx_zynq.c
>> @@ -236,6 +236,7 @@ static void zynq_init(MachineState *machine)
>>      sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
>>      sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
>>  
>> +    /* "xlnx,xuartps" */
>>      cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hds[0]);
>>      cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hds[1]);
>>  
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 325642058b..38f038786c 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -155,7 +155,7 @@ static void xlnx_zynqmp_init(Object *obj)
>>      }
>>  
>>      for (i = 0; i < XLNX_ZYNQMP_NUM_UARTS; i++) {
>> -        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
>> +        object_initialize(&s->uart[i], sizeof(s->uart[i]), "xlnx,zynqmp-uart");
>
> I don't think that commas are valid symbol in type names
> (if I recall correctly it should be letters, numbers and '-')

At least commas are a real PITA when you try to use such devices with
the "-device" CLI parameter. We should try to avoid this if possible, I
think.

 Thomas

Re: [Qemu-devel] [RFC PATCH 4/6] arm/xlnx-zynq: use FDT names for the Cadence UART

[Philippe Mathieu-Daudé]

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

On 01/08/2018 10:17 AM, Thomas Huth wrote:
> On 08.01.2018 13:54, Igor Mammedov wrote:
>> On Thu,  4 Jan 2018 11:40:44 -0300
>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/arm/xilinx_zynq.c | 1 +
>>>  hw/arm/xlnx-zynqmp.c | 2 +-
>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>>> index 1836a4ed45..c45c364583 100644
>>> --- a/hw/arm/xilinx_zynq.c
>>> +++ b/hw/arm/xilinx_zynq.c
>>> @@ -236,6 +236,7 @@ static void zynq_init(MachineState *machine)
>>>      sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
>>>      sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
>>>  
>>> +    /* "xlnx,xuartps" */
>>>      cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hds[0]);
>>>      cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hds[1]);
>>>  
>>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>>> index 325642058b..38f038786c 100644
>>> --- a/hw/arm/xlnx-zynqmp.c
>>> +++ b/hw/arm/xlnx-zynqmp.c
>>> @@ -155,7 +155,7 @@ static void xlnx_zynqmp_init(Object *obj)
>>>      }
>>>  
>>>      for (i = 0; i < XLNX_ZYNQMP_NUM_UARTS; i++) {
>>> -        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
>>> +        object_initialize(&s->uart[i], sizeof(s->uart[i]), "xlnx,zynqmp-uart");
>>
>> I don't think that commas are valid symbol in type names
>> (if I recall correctly it should be letters, numbers and '-')
> 
> At least commas are a real PITA when you try to use such devices with
> the "-device" CLI parameter. We should try to avoid this if possible, I
> think.

Ok, good to know.

I was following Linux Device Tree names [1] to avoid handling some
fdt_qemu_to_linux[] & fdt_linux_to_qemu conversion arrays.

What about keeping using the QEMU default name for -device CLI param and
allow aliases for FDT parsing?
With this series the cadence_uart_info is now:

static const TypeInfo cadence_uart_info = {
    .name          = TYPE_CADENCE_UART,
    .aliases       = (const char * []) {
                        "cdns,uart-r1p8",
                        "xlnx,xuartps",
                        "cdns,uart-r1p12",
                        "xlnx,zynqmp-uart",
                        NULL
                     },

[1]:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/serial/cdns,uart.txt


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [RFC PATCH 4/6] arm/xlnx-zynq: use FDT names for the Cadence UART

[Igor Mammedov]

On Mon, 8 Jan 2018 10:51:38 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 01/08/2018 10:17 AM, Thomas Huth wrote:
> > On 08.01.2018 13:54, Igor Mammedov wrote:  
> >> On Thu,  4 Jan 2018 11:40:44 -0300
> >> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>  
> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> ---
> >>>  hw/arm/xilinx_zynq.c | 1 +
> >>>  hw/arm/xlnx-zynqmp.c | 2 +-
> >>>  2 files changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> >>> index 1836a4ed45..c45c364583 100644
> >>> --- a/hw/arm/xilinx_zynq.c
> >>> +++ b/hw/arm/xilinx_zynq.c
> >>> @@ -236,6 +236,7 @@ static void zynq_init(MachineState *machine)
> >>>      sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
> >>>      sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
> >>>  
> >>> +    /* "xlnx,xuartps" */
> >>>      cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hds[0]);
> >>>      cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hds[1]);
> >>>  
> >>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> >>> index 325642058b..38f038786c 100644
> >>> --- a/hw/arm/xlnx-zynqmp.c
> >>> +++ b/hw/arm/xlnx-zynqmp.c
> >>> @@ -155,7 +155,7 @@ static void xlnx_zynqmp_init(Object *obj)
> >>>      }
> >>>  
> >>>      for (i = 0; i < XLNX_ZYNQMP_NUM_UARTS; i++) {
> >>> -        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
> >>> +        object_initialize(&s->uart[i], sizeof(s->uart[i]), "xlnx,zynqmp-uart");  
> >>
> >> I don't think that commas are valid symbol in type names
> >> (if I recall correctly it should be letters, numbers and '-')  
> > 
> > At least commas are a real PITA when you try to use such devices with
> > the "-device" CLI parameter. We should try to avoid this if possible, I
> > think.  
> 
> Ok, good to know.
> 
> I was following Linux Device Tree names [1] to avoid handling some
> fdt_qemu_to_linux[] & fdt_linux_to_qemu conversion arrays.
> 
> What about keeping using the QEMU default name for -device CLI param and
> allow aliases for FDT parsing?
> With this series the cadence_uart_info is now:
> 
> static const TypeInfo cadence_uart_info = {
>     .name          = TYPE_CADENCE_UART,
>     .aliases       = (const char * []) {
>                         "cdns,uart-r1p8",
>                         "xlnx,xuartps",
>                         "cdns,uart-r1p12",
>                         "xlnx,zynqmp-uart",
>                         NULL
>                      },
No need to pollute Object for this,
It might be better if you make it interface and type will implement it.
But honestly putting desired name in place at the place FDT binding is created
is much simpler and one can adapt it to a specific board as needed.


> 
> [1]:
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/serial/cdns,uart.txt
> 

Re: [Qemu-devel] [RFC PATCH 0/6] qom: introduce TypeInfo name aliases

[Philippe Mathieu-Daudé]

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

Hi Igor,

On 01/08/2018 09:51 AM, Igor Mammedov wrote:
[...]
> Though it seems easy and trivial, I'm a bit concerned about using
> QOM types for the task though.
> Also see commit 6acbe4c6f which labels aliases as a bad idea
> and says that they are there only for compatibility and shouldn't
> be used.
> So far I agree with that statement, because it introduces
> ambiguity in code used internally and more worrying is that
> this ambiguity will increase user visible ABI (think of '-device_add FOO_ALIAS')
> that we would need to maintain afterwards.
> It would be nice to have unified alias API, but I think it should
> be separate one and limited to the same scope (i.e. compat stuff),
> and even that won't be easy as different alias impl. we have now
> have a different needs.
> 
> wrt this series targeted usage, I'd prefer that object_new/initialize
> would use real type names when creating devices as it does currently
> 
> FDT linux guest specific names wouldn't seep into device model
> itself. Firmware (FDT or ACPI) should be separate from device
> implementation.

Good point.

> If really there is need to dynamically scan present devices
> and build FDT from result, then probably we should introduce
> interface that devices could implement if necessary.
> (I was thinking about such possibility for ACPI). But so far
> it looked to me as too much overhead for what we do now.

I see, I thought about something similar but TypeInfo.aliases was way
too simple to not try this series first.

What about adding a INTERFACE_FDT_DEVICE type (InterfaceInfo) and let
the FDT devices implement something such:

typedef struct {
    /*< private >*/
    InterfaceClass parent_class;
    DeviceClass parent_class;
    /*< public >*/
    bool (*is_alias)(FDTDeviceIf *dev, const char *name);
    bool (*set_prop...)(FDTDeviceIf *dev, const char *property, ...);
    const void *(*get_prop)(FDTDeviceIf *dev, const char *property);
};

Regards,

Phil.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/6] qom: introduce TypeInfo name aliases

[Eduardo Habkost]

On Mon, Jan 08, 2018 at 01:51:22PM +0100, Igor Mammedov wrote:
> On Thu, 4 Jan 2018 17:22:03 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Jan 04, 2018 at 11:40:40AM -0300, Philippe Mathieu-Daudé wrote:
> > > Hi,
> > > 
> > > This RFC series is intended to simplify Flattened Device Tree support,
> > > in particular the 'compatible' FDT entry, when Linux names mismatches
> > > QEMU ones, but this is the same device modelled.
> > > 
> > > Eventually this might help to remove the QDevAlias qdev_alias_table[]
> > > in qdev-monitor.c.
> > >   
> > 
> > Didn't look closely at the patches yet, but this sounds like a
> > nice generic way to replace other alias systems.  We have at
> > least:
> Though it seems easy and trivial, I'm a bit concerned about using
> QOM types for the task though.
> Also see commit 6acbe4c6f which labels aliases as a bad idea
> and says that they are there only for compatibility and shouldn't
> be used.

It's true that they are there only for compatibility.  But I
don't think commit 6acbe4c6f label aliases in general as a bad
idea.  It just removes a very limited mechanism and make it
specific to the only place where it was needed.  Now we have at
least 5 different places where we do the same thing, so now it
might be worth it.

But:

> So far I agree with that statement, because it introduces
> ambiguity in code used internally and more worrying is that
> this ambiguity will increase user visible ABI (think of '-device_add FOO_ALIAS')
> that we would need to maintain afterwards.

This is a reasonable worry.  If aliases exist only for
compatibility, they should be restricted to the places where
compatibility is really needed.

For example: aliases that affect -cpu shouldn't necessarily
affect -device.


> It would be nice to have unified alias API, but I think it should
> be separate one and limited to the same scope (i.e. compat stuff),
> and even that won't be easy as different alias impl. we have now
> have a different needs.

Starting with a separate API would be a good way to understand
what are our real needs, before deciding if we really want
aliases that affect all object_new() calls.

> 
> wrt this series targeted usage, I'd prefer that object_new/initialize
> would use real type names when creating devices as it does currently
> 
> FDT linux guest specific names wouldn't seep into device model
> itself. Firmware (FDT or ACPI) should be separate from device
> implementation.

I agree.  The QOM names and FDT names have different expectations
and assumptions (the commas added to some type names in this
series are one example).  Having the QOM type and FDT name match
on most cases is nice, but this shouldn't be a requirement.

I don't think we should rename user-visible QOM types and change
the command-line interface (and require additional compatibility
cruft) just to make it more convenient for internal FDT code.


> 
> If really there is need to dynamically scan present devices
> and build FDT from result, then probably we should introduce
> interface that devices could implement if necessary.
> (I was thinking about such possibility for ACPI). But so far
> it looked to me as too much overhead for what we do now.
> 
> 
> > * qdev-monitor.c: qdev_alias_table[] (as mentioned above)
> > * chardev/char.c: chardev_alias_table[]
> > * target/alpha/cpu.c: alpha_cpu_aliases[]
> > * target/ppc/cpu-models.c: ppc_cpu_aliases[]
> > * include/hw/boards.h: MachineClass::alias
> > 
> > Probably there are others I couldn't find.
> > 
> > 
> > > So far this is only a 'proof of concept'.
> > > To see how the qtests perform, I only modified 3 devices, 2 used by the
> > > Xilinx Zynq machines (Cadence), and the e1000 (used by the PXE test).
> > > 
> > > Regards,
> > > 
> > > Phil.
> > > 
> > > Philippe Mathieu-Daudé (6):
> > >   qom: introduce TypeInfo name aliases
> > >   hw/net/e1000: real device name is 'e1000-82540em', 'e1000' is an alias
> > >   hw/char/cadence_uart: add FDT aliases
> > >   arm/xlnx-zynq: use FDT names for the Cadence UART
> > >   hw/net/cadence_gem: add FDT names as alias
> > >   hw/arm/xlnx-zynq: use FDT names for the Cadence GEM
> > > 
> > >  include/qom/object.h   |  3 +++
> > >  hw/arm/xilinx_zynq.c   |  2 ++
> > >  hw/arm/xlnx-zynqmp.c   |  4 ++--
> > >  hw/char/cadence_uart.c |  7 +++++++
> > >  hw/net/cadence_gem.c   |  6 ++++++
> > >  hw/net/e1000.c         |  5 ++++-
> > >  qom/object.c           | 18 ++++++++++++++++--
> > >  7 files changed, 40 insertions(+), 5 deletions(-)
> > > 
> > > -- 
> > > 2.15.1
> > >   
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 0/6] qom: introduce TypeInfo name aliases

[Eduardo Habkost]

On Mon, Jan 08, 2018 at 11:10:37AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Igor,
> 
> On 01/08/2018 09:51 AM, Igor Mammedov wrote:
> [...]
> > Though it seems easy and trivial, I'm a bit concerned about using
> > QOM types for the task though.
> > Also see commit 6acbe4c6f which labels aliases as a bad idea
> > and says that they are there only for compatibility and shouldn't
> > be used.
> > So far I agree with that statement, because it introduces
> > ambiguity in code used internally and more worrying is that
> > this ambiguity will increase user visible ABI (think of '-device_add FOO_ALIAS')
> > that we would need to maintain afterwards.
> > It would be nice to have unified alias API, but I think it should
> > be separate one and limited to the same scope (i.e. compat stuff),
> > and even that won't be easy as different alias impl. we have now
> > have a different needs.
> > 
> > wrt this series targeted usage, I'd prefer that object_new/initialize
> > would use real type names when creating devices as it does currently
> > 
> > FDT linux guest specific names wouldn't seep into device model
> > itself. Firmware (FDT or ACPI) should be separate from device
> > implementation.
> 
> Good point.
> 
> > If really there is need to dynamically scan present devices
> > and build FDT from result, then probably we should introduce
> > interface that devices could implement if necessary.
> > (I was thinking about such possibility for ACPI). But so far
> > it looked to me as too much overhead for what we do now.
> 
> I see, I thought about something similar but TypeInfo.aliases was way
> too simple to not try this series first.
> 
> What about adding a INTERFACE_FDT_DEVICE type (InterfaceInfo) and let
> the FDT devices implement something such:
> 
> typedef struct {
>     /*< private >*/
>     InterfaceClass parent_class;
>     DeviceClass parent_class;
>     /*< public >*/
>     bool (*is_alias)(FDTDeviceIf *dev, const char *name);
>     bool (*set_prop...)(FDTDeviceIf *dev, const char *property, ...);
>     const void *(*get_prop)(FDTDeviceIf *dev, const char *property);

An interface common to FDT devices probably make sense, but I
don't know if the one you suggest above makes sense or not (a
description of each method would be useful to understand what
exactly they would do).

-- 
Eduardo