[PATCH 0/2] Replace anti-social QOM type names (again)

[PATCH 0/2] Replace anti-social QOM type names (again)

[Markus Armbruster]

We got rid of QOM type names containing ',' in 6.0, but some have
crept back in.  Replace them just like we did in 6.0.

Cover letter of 6.0's replacement:
https://lore.kernel.org/qemu-devel/20210304140229.575481-1-armbru@redhat.com/

Markus Armbruster (2):
  docs/system/arm: Fix for rename of type "xlnx.bbram-ctrl"
  hw: Replace anti-social QOM type names (again)

 docs/system/arm/xlnx-versal-virt.rst     | 4 ++--
 include/hw/misc/xlnx-versal-cframe-reg.h | 2 +-
 include/hw/misc/xlnx-versal-cfu.h        | 6 +++---
 include/hw/misc/xlnx-versal-crl.h        | 2 +-
 include/hw/nvram/xlnx-efuse.h            | 2 +-
 include/hw/nvram/xlnx-versal-efuse.h     | 4 ++--
 include/hw/nvram/xlnx-zynqmp-efuse.h     | 2 +-
 7 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.41.0

[PATCH 1/2] docs/system/arm: Fix for rename of type "xlnx.bbram-ctrl"

[Markus Armbruster]

Fixes: b65b4b7ae3c8 (xlnx-bbram: hw/nvram: Use dot in device type name)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/system/arm/xlnx-versal-virt.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/arm/xlnx-versal-virt.rst b/docs/system/arm/xlnx-versal-virt.rst
index d2d1b26692..a6a77b3799 100644
--- a/docs/system/arm/xlnx-versal-virt.rst
+++ b/docs/system/arm/xlnx-versal-virt.rst
@@ -194,7 +194,7 @@ To use a different index value, N, from default of 0, add:
 
 .. code-block:: bash
 
-  -global xlnx,bbram-ctrl.drive-index=N
+  -global xlnx.bbram-ctrl.drive-index=N
 
 eFUSE File Backend
 """"""""""""""""""
-- 
2.41.0

[PATCH 2/2] hw: Replace anti-social QOM type names (again)

[Markus Armbruster]

QOM type names containing ',' result in awful UI.  We got rid of them
in v6.0.0 (commit e178113ff64 hw: Replace anti-social QOM type names).
A few have crept back since:

    xlnx,cframe-reg
    xlnx,efuse
    xlnx,pmc-efuse-cache
    xlnx,versal-cfu-apb
    xlnx,versal-cfu-fdro
    xlnx,versal-cfu-sfr
    xlnx,versal-crl
    xlnx,versal-efuse
    xlnx,zynqmp-efuse

These are all device types.  They can't be plugged with -device /
device_add, except for "xlnx,efuse" (I'm not sure that one is
intentional).

They *can* be used with -device / device_add to request help.
Usability is poor, though: you have to double the comma, like this:

    $ qemu-system-aarch64 -device xlnx,,pmc-efuse-cache,help

They can also be used with -global, where you must *not* double the
comma:

    $ qemu-system-aarch64 -global xlnx,efuse.drive-index=2

Trap for the unwary.

"xlnx,efuse", "xlnx,versal-efuse", "xlnx,pmc-efuse-cache",
"xlnx-zynqmp-efuse" are from v6.2.0, "xlnx,versal-crl" is from v7.1.0,
and the remainder are new.

Rename them all to "xlnx-FOO", like commit e178113ff64 did.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/system/arm/xlnx-versal-virt.rst     | 2 +-
 include/hw/misc/xlnx-versal-cframe-reg.h | 2 +-
 include/hw/misc/xlnx-versal-cfu.h        | 6 +++---
 include/hw/misc/xlnx-versal-crl.h        | 2 +-
 include/hw/nvram/xlnx-efuse.h            | 2 +-
 include/hw/nvram/xlnx-versal-efuse.h     | 4 ++--
 include/hw/nvram/xlnx-zynqmp-efuse.h     | 2 +-
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/docs/system/arm/xlnx-versal-virt.rst b/docs/system/arm/xlnx-versal-virt.rst
index a6a77b3799..edf2e48109 100644
--- a/docs/system/arm/xlnx-versal-virt.rst
+++ b/docs/system/arm/xlnx-versal-virt.rst
@@ -212,7 +212,7 @@ To use a different index value, N, from default of 1, add:
 
 .. code-block:: bash
 
-  -global xlnx,efuse.drive-index=N
+  -global xlnx-efuse.drive-index=N
 
 .. warning::
   In actual physical Versal, BBRAM and eFUSE contain sensitive data.
diff --git a/include/hw/misc/xlnx-versal-cframe-reg.h b/include/hw/misc/xlnx-versal-cframe-reg.h
index a14fbd7fe4..f403b00e31 100644
--- a/include/hw/misc/xlnx-versal-cframe-reg.h
+++ b/include/hw/misc/xlnx-versal-cframe-reg.h
@@ -23,7 +23,7 @@
 #include "hw/misc/xlnx-versal-cfu.h"
 #include "qemu/fifo32.h"
 
-#define TYPE_XLNX_VERSAL_CFRAME_REG "xlnx,cframe-reg"
+#define TYPE_XLNX_VERSAL_CFRAME_REG "xlnx-cframe-reg"
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFrameReg, XLNX_VERSAL_CFRAME_REG)
 
 #define TYPE_XLNX_VERSAL_CFRAME_BCAST_REG "xlnx.cframe-bcast-reg"
diff --git a/include/hw/misc/xlnx-versal-cfu.h b/include/hw/misc/xlnx-versal-cfu.h
index 86fb841053..8c581c0797 100644
--- a/include/hw/misc/xlnx-versal-cfu.h
+++ b/include/hw/misc/xlnx-versal-cfu.h
@@ -22,13 +22,13 @@
 #include "hw/misc/xlnx-cfi-if.h"
 #include "qemu/fifo32.h"
 
-#define TYPE_XLNX_VERSAL_CFU_APB "xlnx,versal-cfu-apb"
+#define TYPE_XLNX_VERSAL_CFU_APB "xlnx-versal-cfu-apb"
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFUAPB, XLNX_VERSAL_CFU_APB)
 
-#define TYPE_XLNX_VERSAL_CFU_FDRO "xlnx,versal-cfu-fdro"
+#define TYPE_XLNX_VERSAL_CFU_FDRO "xlnx-versal-cfu-fdro"
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFUFDRO, XLNX_VERSAL_CFU_FDRO)
 
-#define TYPE_XLNX_VERSAL_CFU_SFR "xlnx,versal-cfu-sfr"
+#define TYPE_XLNX_VERSAL_CFU_SFR "xlnx-versal-cfu-sfr"
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFUSFR, XLNX_VERSAL_CFU_SFR)
 
 REG32(CFU_ISR, 0x0)
diff --git a/include/hw/misc/xlnx-versal-crl.h b/include/hw/misc/xlnx-versal-crl.h
index 2857f4169a..dfb8dff197 100644
--- a/include/hw/misc/xlnx-versal-crl.h
+++ b/include/hw/misc/xlnx-versal-crl.h
@@ -13,7 +13,7 @@
 #include "hw/register.h"
 #include "target/arm/cpu.h"
 
-#define TYPE_XLNX_VERSAL_CRL "xlnx,versal-crl"
+#define TYPE_XLNX_VERSAL_CRL "xlnx-versal-crl"
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCRL, XLNX_VERSAL_CRL)
 
 REG32(ERR_CTRL, 0x0)
diff --git a/include/hw/nvram/xlnx-efuse.h b/include/hw/nvram/xlnx-efuse.h
index 58414e468b..cff7924106 100644
--- a/include/hw/nvram/xlnx-efuse.h
+++ b/include/hw/nvram/xlnx-efuse.h
@@ -30,7 +30,7 @@
 #include "sysemu/block-backend.h"
 #include "hw/qdev-core.h"
 
-#define TYPE_XLNX_EFUSE "xlnx,efuse"
+#define TYPE_XLNX_EFUSE "xlnx-efuse"
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxEFuse, XLNX_EFUSE);
 
 struct XlnxEFuse {
diff --git a/include/hw/nvram/xlnx-versal-efuse.h b/include/hw/nvram/xlnx-versal-efuse.h
index a873dc5cb0..86e2261b9a 100644
--- a/include/hw/nvram/xlnx-versal-efuse.h
+++ b/include/hw/nvram/xlnx-versal-efuse.h
@@ -29,8 +29,8 @@
 
 #define XLNX_VERSAL_EFUSE_CTRL_R_MAX ((0x100 / 4) + 1)
 
-#define TYPE_XLNX_VERSAL_EFUSE_CTRL  "xlnx,versal-efuse"
-#define TYPE_XLNX_VERSAL_EFUSE_CACHE "xlnx,pmc-efuse-cache"
+#define TYPE_XLNX_VERSAL_EFUSE_CTRL  "xlnx-versal-efuse"
+#define TYPE_XLNX_VERSAL_EFUSE_CACHE "xlnx-pmc-efuse-cache"
 
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalEFuseCtrl, XLNX_VERSAL_EFUSE_CTRL);
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalEFuseCache, XLNX_VERSAL_EFUSE_CACHE);
diff --git a/include/hw/nvram/xlnx-zynqmp-efuse.h b/include/hw/nvram/xlnx-zynqmp-efuse.h
index 6b051ec4f1..f5beacc2e6 100644
--- a/include/hw/nvram/xlnx-zynqmp-efuse.h
+++ b/include/hw/nvram/xlnx-zynqmp-efuse.h
@@ -29,7 +29,7 @@
 
 #define XLNX_ZYNQMP_EFUSE_R_MAX ((0x10fc / 4) + 1)
 
-#define TYPE_XLNX_ZYNQMP_EFUSE "xlnx,zynqmp-efuse"
+#define TYPE_XLNX_ZYNQMP_EFUSE "xlnx-zynqmp-efuse"
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPEFuse, XLNX_ZYNQMP_EFUSE);
 
 struct XlnxZynqMPEFuse {
-- 
2.41.0

Re: [PATCH 0/2] Replace anti-social QOM type names (again)

[Daniel P. Berrangé]

On Mon, Nov 13, 2023 at 02:43:42PM +0100, Markus Armbruster wrote:
> We got rid of QOM type names containing ',' in 6.0, but some have
> crept back in.  Replace them just like we did in 6.0.

It is practical to add

   assert(strchr(name, ',') == NULL)

to some place in QOM to stop them coming back yet again ?

> 
> Cover letter of 6.0's replacement:
> https://lore.kernel.org/qemu-devel/20210304140229.575481-1-armbru@redhat.com/
> 
> Markus Armbruster (2):
>   docs/system/arm: Fix for rename of type "xlnx.bbram-ctrl"
>   hw: Replace anti-social QOM type names (again)
> 
>  docs/system/arm/xlnx-versal-virt.rst     | 4 ++--
>  include/hw/misc/xlnx-versal-cframe-reg.h | 2 +-
>  include/hw/misc/xlnx-versal-cfu.h        | 6 +++---
>  include/hw/misc/xlnx-versal-crl.h        | 2 +-
>  include/hw/nvram/xlnx-efuse.h            | 2 +-
>  include/hw/nvram/xlnx-versal-efuse.h     | 4 ++--
>  include/hw/nvram/xlnx-zynqmp-efuse.h     | 2 +-
>  7 files changed, 11 insertions(+), 11 deletions(-)
> 
> -- 
> 2.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] docs/system/arm: Fix for rename of type "xlnx.bbram-ctrl"

[Francisco Iglesias]

On 2023-11-13 14:43, Markus Armbruster wrote:
> Fixes: b65b4b7ae3c8 (xlnx-bbram: hw/nvram: Use dot in device type name)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>


> ---
>   docs/system/arm/xlnx-versal-virt.rst | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/system/arm/xlnx-versal-virt.rst b/docs/system/arm/xlnx-versal-virt.rst
> index d2d1b26692..a6a77b3799 100644
> --- a/docs/system/arm/xlnx-versal-virt.rst
> +++ b/docs/system/arm/xlnx-versal-virt.rst
> @@ -194,7 +194,7 @@ To use a different index value, N, from default of 0, add:
>   
>   .. code-block:: bash
>   
> -  -global xlnx,bbram-ctrl.drive-index=N
> +  -global xlnx.bbram-ctrl.drive-index=N
>   
>   eFUSE File Backend
>   """"""""""""""""""

Re: [PATCH 2/2] hw: Replace anti-social QOM type names (again)

[Francisco Iglesias]

On 2023-11-13 14:43, Markus Armbruster wrote:
> QOM type names containing ',' result in awful UI.  We got rid of them
> in v6.0.0 (commit e178113ff64 hw: Replace anti-social QOM type names).
> A few have crept back since:
> 
>      xlnx,cframe-reg
>      xlnx,efuse
>      xlnx,pmc-efuse-cache
>      xlnx,versal-cfu-apb
>      xlnx,versal-cfu-fdro
>      xlnx,versal-cfu-sfr
>      xlnx,versal-crl
>      xlnx,versal-efuse
>      xlnx,zynqmp-efuse
> 
> These are all device types.  They can't be plugged with -device /
> device_add, except for "xlnx,efuse" (I'm not sure that one is
> intentional).
> 
> They *can* be used with -device / device_add to request help.
> Usability is poor, though: you have to double the comma, like this:
> 
>      $ qemu-system-aarch64 -device xlnx,,pmc-efuse-cache,help
> 
> They can also be used with -global, where you must *not* double the
> comma:
> 
>      $ qemu-system-aarch64 -global xlnx,efuse.drive-index=2
> 
> Trap for the unwary.
> 
> "xlnx,efuse", "xlnx,versal-efuse", "xlnx,pmc-efuse-cache",
> "xlnx-zynqmp-efuse" are from v6.2.0, "xlnx,versal-crl" is from v7.1.0,
> and the remainder are new.
> 
> Rename them all to "xlnx-FOO", like commit e178113ff64 did.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

> ---
>   docs/system/arm/xlnx-versal-virt.rst     | 2 +-
>   include/hw/misc/xlnx-versal-cframe-reg.h | 2 +-
>   include/hw/misc/xlnx-versal-cfu.h        | 6 +++---
>   include/hw/misc/xlnx-versal-crl.h        | 2 +-
>   include/hw/nvram/xlnx-efuse.h            | 2 +-
>   include/hw/nvram/xlnx-versal-efuse.h     | 4 ++--
>   include/hw/nvram/xlnx-zynqmp-efuse.h     | 2 +-
>   7 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/system/arm/xlnx-versal-virt.rst b/docs/system/arm/xlnx-versal-virt.rst
> index a6a77b3799..edf2e48109 100644
> --- a/docs/system/arm/xlnx-versal-virt.rst
> +++ b/docs/system/arm/xlnx-versal-virt.rst
> @@ -212,7 +212,7 @@ To use a different index value, N, from default of 1, add:
>   
>   .. code-block:: bash
>   
> -  -global xlnx,efuse.drive-index=N
> +  -global xlnx-efuse.drive-index=N
>   
>   .. warning::
>     In actual physical Versal, BBRAM and eFUSE contain sensitive data.
> diff --git a/include/hw/misc/xlnx-versal-cframe-reg.h b/include/hw/misc/xlnx-versal-cframe-reg.h
> index a14fbd7fe4..f403b00e31 100644
> --- a/include/hw/misc/xlnx-versal-cframe-reg.h
> +++ b/include/hw/misc/xlnx-versal-cframe-reg.h
> @@ -23,7 +23,7 @@
>   #include "hw/misc/xlnx-versal-cfu.h"
>   #include "qemu/fifo32.h"
>   
> -#define TYPE_XLNX_VERSAL_CFRAME_REG "xlnx,cframe-reg"
> +#define TYPE_XLNX_VERSAL_CFRAME_REG "xlnx-cframe-reg"
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFrameReg, XLNX_VERSAL_CFRAME_REG)
>   
>   #define TYPE_XLNX_VERSAL_CFRAME_BCAST_REG "xlnx.cframe-bcast-reg"
> diff --git a/include/hw/misc/xlnx-versal-cfu.h b/include/hw/misc/xlnx-versal-cfu.h
> index 86fb841053..8c581c0797 100644
> --- a/include/hw/misc/xlnx-versal-cfu.h
> +++ b/include/hw/misc/xlnx-versal-cfu.h
> @@ -22,13 +22,13 @@
>   #include "hw/misc/xlnx-cfi-if.h"
>   #include "qemu/fifo32.h"
>   
> -#define TYPE_XLNX_VERSAL_CFU_APB "xlnx,versal-cfu-apb"
> +#define TYPE_XLNX_VERSAL_CFU_APB "xlnx-versal-cfu-apb"
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFUAPB, XLNX_VERSAL_CFU_APB)
>   
> -#define TYPE_XLNX_VERSAL_CFU_FDRO "xlnx,versal-cfu-fdro"
> +#define TYPE_XLNX_VERSAL_CFU_FDRO "xlnx-versal-cfu-fdro"
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFUFDRO, XLNX_VERSAL_CFU_FDRO)
>   
> -#define TYPE_XLNX_VERSAL_CFU_SFR "xlnx,versal-cfu-sfr"
> +#define TYPE_XLNX_VERSAL_CFU_SFR "xlnx-versal-cfu-sfr"
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFUSFR, XLNX_VERSAL_CFU_SFR)
>   
>   REG32(CFU_ISR, 0x0)
> diff --git a/include/hw/misc/xlnx-versal-crl.h b/include/hw/misc/xlnx-versal-crl.h
> index 2857f4169a..dfb8dff197 100644
> --- a/include/hw/misc/xlnx-versal-crl.h
> +++ b/include/hw/misc/xlnx-versal-crl.h
> @@ -13,7 +13,7 @@
>   #include "hw/register.h"
>   #include "target/arm/cpu.h"
>   
> -#define TYPE_XLNX_VERSAL_CRL "xlnx,versal-crl"
> +#define TYPE_XLNX_VERSAL_CRL "xlnx-versal-crl"
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCRL, XLNX_VERSAL_CRL)
>   
>   REG32(ERR_CTRL, 0x0)
> diff --git a/include/hw/nvram/xlnx-efuse.h b/include/hw/nvram/xlnx-efuse.h
> index 58414e468b..cff7924106 100644
> --- a/include/hw/nvram/xlnx-efuse.h
> +++ b/include/hw/nvram/xlnx-efuse.h
> @@ -30,7 +30,7 @@
>   #include "sysemu/block-backend.h"
>   #include "hw/qdev-core.h"
>   
> -#define TYPE_XLNX_EFUSE "xlnx,efuse"
> +#define TYPE_XLNX_EFUSE "xlnx-efuse"
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxEFuse, XLNX_EFUSE);
>   
>   struct XlnxEFuse {
> diff --git a/include/hw/nvram/xlnx-versal-efuse.h b/include/hw/nvram/xlnx-versal-efuse.h
> index a873dc5cb0..86e2261b9a 100644
> --- a/include/hw/nvram/xlnx-versal-efuse.h
> +++ b/include/hw/nvram/xlnx-versal-efuse.h
> @@ -29,8 +29,8 @@
>   
>   #define XLNX_VERSAL_EFUSE_CTRL_R_MAX ((0x100 / 4) + 1)
>   
> -#define TYPE_XLNX_VERSAL_EFUSE_CTRL  "xlnx,versal-efuse"
> -#define TYPE_XLNX_VERSAL_EFUSE_CACHE "xlnx,pmc-efuse-cache"
> +#define TYPE_XLNX_VERSAL_EFUSE_CTRL  "xlnx-versal-efuse"
> +#define TYPE_XLNX_VERSAL_EFUSE_CACHE "xlnx-pmc-efuse-cache"
>   
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalEFuseCtrl, XLNX_VERSAL_EFUSE_CTRL);
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalEFuseCache, XLNX_VERSAL_EFUSE_CACHE);
> diff --git a/include/hw/nvram/xlnx-zynqmp-efuse.h b/include/hw/nvram/xlnx-zynqmp-efuse.h
> index 6b051ec4f1..f5beacc2e6 100644
> --- a/include/hw/nvram/xlnx-zynqmp-efuse.h
> +++ b/include/hw/nvram/xlnx-zynqmp-efuse.h
> @@ -29,7 +29,7 @@
>   
>   #define XLNX_ZYNQMP_EFUSE_R_MAX ((0x10fc / 4) + 1)
>   
> -#define TYPE_XLNX_ZYNQMP_EFUSE "xlnx,zynqmp-efuse"
> +#define TYPE_XLNX_ZYNQMP_EFUSE "xlnx-zynqmp-efuse"
>   OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPEFuse, XLNX_ZYNQMP_EFUSE);
>   
>   struct XlnxZynqMPEFuse {

Re: [PATCH 1/2] docs/system/arm: Fix for rename of type "xlnx.bbram-ctrl"

[Philippe Mathieu-Daudé]

On 13/11/23 14:43, Markus Armbruster wrote:
> Fixes: b65b4b7ae3c8 (xlnx-bbram: hw/nvram: Use dot in device type name)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   docs/system/arm/xlnx-versal-virt.rst | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH 1/2] docs/system/arm: Fix for rename of type "xlnx.bbram-ctrl"

[Thomas Huth]

On 13/11/2023 14.43, Markus Armbruster wrote:
> Fixes: b65b4b7ae3c8 (xlnx-bbram: hw/nvram: Use dot in device type name)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   docs/system/arm/xlnx-versal-virt.rst | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/system/arm/xlnx-versal-virt.rst b/docs/system/arm/xlnx-versal-virt.rst
> index d2d1b26692..a6a77b3799 100644
> --- a/docs/system/arm/xlnx-versal-virt.rst
> +++ b/docs/system/arm/xlnx-versal-virt.rst
> @@ -194,7 +194,7 @@ To use a different index value, N, from default of 0, add:
>   
>   .. code-block:: bash
>   
> -  -global xlnx,bbram-ctrl.drive-index=N
> +  -global xlnx.bbram-ctrl.drive-index=N

Ouch, that's now ugly, too. Imagine that we have a device called "xlnx" one 
day, how's the reader supposed to distinguish between the "xlnx" and the 
"xlnx.bbram-ctrl" device here?

It feels like we should forbid both, "," and "." in device names...

Anyway, for the current state:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/2] hw: Replace anti-social QOM type names (again)

[Thomas Huth]

On 13/11/2023 14.43, Markus Armbruster wrote:
> QOM type names containing ',' result in awful UI.  We got rid of them
> in v6.0.0 (commit e178113ff64 hw: Replace anti-social QOM type names).
> A few have crept back since:
> 
>      xlnx,cframe-reg
>      xlnx,efuse
>      xlnx,pmc-efuse-cache
>      xlnx,versal-cfu-apb
>      xlnx,versal-cfu-fdro
>      xlnx,versal-cfu-sfr
>      xlnx,versal-crl
>      xlnx,versal-efuse
>      xlnx,zynqmp-efuse
> 
> These are all device types.  They can't be plugged with -device /
> device_add, except for "xlnx,efuse" (I'm not sure that one is
> intentional).
> 
> They *can* be used with -device / device_add to request help.
> Usability is poor, though: you have to double the comma, like this:
> 
>      $ qemu-system-aarch64 -device xlnx,,pmc-efuse-cache,help
> 
> They can also be used with -global, where you must *not* double the
> comma:
> 
>      $ qemu-system-aarch64 -global xlnx,efuse.drive-index=2
> 
> Trap for the unwary.
> 
> "xlnx,efuse", "xlnx,versal-efuse", "xlnx,pmc-efuse-cache",
> "xlnx-zynqmp-efuse" are from v6.2.0, "xlnx,versal-crl" is from v7.1.0,
> and the remainder are new.
> 
> Rename them all to "xlnx-FOO", like commit e178113ff64 did.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   docs/system/arm/xlnx-versal-virt.rst     | 2 +-
>   include/hw/misc/xlnx-versal-cframe-reg.h | 2 +-
>   include/hw/misc/xlnx-versal-cfu.h        | 6 +++---
>   include/hw/misc/xlnx-versal-crl.h        | 2 +-
>   include/hw/nvram/xlnx-efuse.h            | 2 +-
>   include/hw/nvram/xlnx-versal-efuse.h     | 4 ++--
>   include/hw/nvram/xlnx-zynqmp-efuse.h     | 2 +-
>   7 files changed, 10 insertions(+), 10 deletions(-)
>
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/2] docs/system/arm: Fix for rename of type "xlnx.bbram-ctrl"

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 13/11/2023 14.43, Markus Armbruster wrote:
>> Fixes: b65b4b7ae3c8 (xlnx-bbram: hw/nvram: Use dot in device type name)
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   docs/system/arm/xlnx-versal-virt.rst | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/docs/system/arm/xlnx-versal-virt.rst b/docs/system/arm/xlnx-versal-virt.rst
>> index d2d1b26692..a6a77b3799 100644
>> --- a/docs/system/arm/xlnx-versal-virt.rst
>> +++ b/docs/system/arm/xlnx-versal-virt.rst
>> @@ -194,7 +194,7 @@ To use a different index value, N, from default of 0, add:
>>     .. code-block:: bash
>>   -  -global xlnx,bbram-ctrl.drive-index=N
>> +  -global xlnx.bbram-ctrl.drive-index=N
>
> Ouch, that's now ugly, too. Imagine that we have a device called "xlnx" one day, how's the reader supposed to distinguish between the "xlnx" and the "xlnx.bbram-ctrl" device here?

Hmm, it's actually worse than ugly: it doesn't work.

    $ qemu-system-aarch64 -nodefaults -S -display none -M none -global xlnx.bbram-ctrl.drive-index=2
    qemu-system-aarch64: warning: global xlnx.bbram-ctrl.drive-index has invalid class name

That's because xlnx.bbram-ctrl.drive-index=N is syntactically ambiguous: it
could be sugar for

    driver=xlnx.bbram-ctrl,property=drive-index,value=N

or for

    driver=xlnx,property=bbram-ctrl.drive-index,value=N

Our parser picks the latter.

I'll respin the patch to use longhand syntax.

> It feels like we should forbid both, "," and "." in device names...

Yes, '.' is a bad idea, too.  But there are many names with them, and
I'm not ready to tackle them.

> Anyway, for the current state:
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!

Re: [PATCH 0/2] Replace anti-social QOM type names (again)

[Markus Armbruster]

Cc: the other QOM maintainers

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Nov 13, 2023 at 02:43:42PM +0100, Markus Armbruster wrote:
>> We got rid of QOM type names containing ',' in 6.0, but some have
>> crept back in.  Replace them just like we did in 6.0.
>
> It is practical to add
>
>    assert(strchr(name, ',') == NULL)
>
> to some place in QOM to stop them coming back yet again ?

This adds a naming rule to QOM.  Right now, QOM has none whatsoever,
which I've long called out as a mistake.

I'm all for correcting that mistake, but I'd go further than just
outlawing ','.

Discussed in more depth here:

>> Cover letter of 6.0's replacement:
>> https://lore.kernel.org/qemu-devel/20210304140229.575481-1-armbru@redhat.com/

Let me copy the text for convenience.

QAPI has naming rules.  docs/devel/qapi-code-gen.txt:

    === Naming rules and reserved names ===

    All names must begin with a letter, and contain only ASCII letters,
    digits, hyphen, and underscore.  There are two exceptions: enum values
    may start with a digit, and names that are downstream extensions (see
    section Downstream extensions) start with underscore.

    [More on reserved names, upper vs. lower case, '-' vs. '_'...]

The generator enforces the rules.

Naming rules help in at least three ways:

1. They help with keeping names in interfaces consistent and
   predictable.

2. They make avoiding collisions with the users' names in the
   generator simpler.

3. They enable quote-less, evolvable syntax.

   For instance, keyval_parse() syntax consists of names, values, and
   special characters ',', '=', '.'

   Since names cannot contain special characters, there is no need for
   quoting[*].  Simple.

   Values are unrestricted, but only ',' is special there.  We quote
   it by doubling.

   Together, we get exactly the same quoting as in QemuOpts.  This is
   a feature.

   If we ever decice to extend key syntax, we have plenty of special
   characters to choose from.  This is also a feature.

   Both features rely on naming rules.

QOM has no naming rules whatsoever.  Actual names aren't nearly as bad
as they could be.  Still, there are plenty of "funny" names.  This may
become a problem when we

* Switch from QemuOpts to keyval_parse()

  Compared to QemuOpts, keyval_parse() restricts *keys*, but not
  *values*.

  "Funny" type names occuring as values are no worse than before:
  quoting issues, described below.

  Type names occuring in keys must be valid QAPI names.  Should be
  avoidable.

* QAPIfy (the compile-time static parts of) QOM

  QOM type names become QAPI enum values.  They must conform to QAPI
  enum naming rules.

[...]

One more thing on relaxing QAPI naming rules.  QAPI names get mapped
to (parts of) C identifiers.  These mappings are not injective.  The
basic mapping is simple: replace characters other than letters and
digits by '_'.

This means names distinct QAPI names can clash in C.  Fairly harmless
when the only "other" characters are '-' and '_'.  The more "others" we
permit, the more likely confusing clashes become.  Not a show stopper,
"merely" an issue of ergonomics.

Re: [PATCH 0/2] Replace anti-social QOM type names (again)

[Thomas Huth]

On 14/11/2023 08.41, Markus Armbruster wrote:
> Cc: the other QOM maintainers
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Mon, Nov 13, 2023 at 02:43:42PM +0100, Markus Armbruster wrote:
>>> We got rid of QOM type names containing ',' in 6.0, but some have
>>> crept back in.  Replace them just like we did in 6.0.
>>
>> It is practical to add
>>
>>     assert(strchr(name, ',') == NULL)
>>
>> to some place in QOM to stop them coming back yet again ?
> 
> This adds a naming rule to QOM.  Right now, QOM has none whatsoever,
> which I've long called out as a mistake.
> 
> I'm all for correcting that mistake, but I'd go further than just
> outlawing ','.

What prevents us from fixing this "mistake"? Is there any compelling reason 
for keeping the current lax naming rules of QOM? Would there be migration 
issues if we'd rename the current offenders? (and even if so, couldn't we 
simply fix that issue by curating an allowlist of old names?)

  Thomas

Re: [PATCH 0/2] Replace anti-social QOM type names (again)

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 14/11/2023 08.41, Markus Armbruster wrote:
>> Cc: the other QOM maintainers
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>>> On Mon, Nov 13, 2023 at 02:43:42PM +0100, Markus Armbruster wrote:
>>>> We got rid of QOM type names containing ',' in 6.0, but some have
>>>> crept back in.  Replace them just like we did in 6.0.
>>>
>>> It is practical to add
>>>
>>>     assert(strchr(name, ',') == NULL)
>>>
>>> to some place in QOM to stop them coming back yet again ?
>>
>> This adds a naming rule to QOM.  Right now, QOM has none whatsoever,
>> which I've long called out as a mistake.
>>
>> I'm all for correcting that mistake, but I'd go further than just
>> outlawing ','.
>
> What prevents us from fixing this "mistake"?

1. Having to clean up the naming messes we made.  This involves backward
compatibility arguments and work-arounds.

2. Inertia.

> Is there any compelling reason for keeping the current lax naming rules of QOM?

Can't think of any but avoiding 1.

> Would there be migration issues if we'd rename the current offenders? (and even if so, couldn't we simply fix that issue by curating an allowlist of old names?)

I believe migration should not be affected, since migration section
names are entirely separate.  Mind, I'm no migration expert.