[PATCH] hw/riscv/virt.c: re-insert and deprecate 'riscv,delegate'

[PATCH] hw/riscv/virt.c: re-insert and deprecate 'riscv,delegate'

[Daniel Henrique Barboza]

Commit b1f1e9dcfa renamed 'riscv,delegate' to 'riscv,delegation' since
it is the correct name as per dt-bindings, and the absence of the
correct name will result in validation fails when dumping the dtb and
using dt-validate.

But this change has a side-effect: every other firmware available that
is AIA capable is using 'riscv,delegate', and it will fault/misbehave if
this property isn't present. The property was added back in QEMU 7.0,
meaning we have 2 years of firmware development using the wrong
property.

Re-introducing 'riscv,delegate' while keeping 'riscv,delegation' will
make current firmwares to keep booting with the 'virt' machine and
dt-validate won't complain about it since we're still using the expected
property 'riscv,delegation'. 'riscv,delegate' is then marked for future
deprecation and its use is being discouraged.

Cc: Conor Dooley <conor@kernel.org>
Cc: Anup Patel <apatel@ventanamicro.com>
Fixes: b1f1e9dcfa ("hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation'")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 docs/about/deprecated.rst | 11 +++++++++++
 hw/riscv/virt.c           |  9 +++++++++
 2 files changed, 20 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 20b7a17cf0..88f0f03786 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -479,6 +479,17 @@ versions, aliases will point to newer CPU model versions
 depending on the machine type, so management software must
 resolve CPU model aliases before starting a virtual machine.
 
+RISC-V "virt" board "riscv,delegate" DT property (since 9.1)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+The "riscv,delegate" DT property was added in QEMU 7.0 as part of
+the AIA APLIC support.  The property changed name during the review
+process in Linux and the correct name ended up being
+"riscv,delegation".  Changing the DT property name will break all
+available firmwares that are using the current (wrong) name.  The
+property is kept as is in 9.1, together with "riscv,delegation", to
+give more time for firmware developers to change their code.
+
 Migration
 ---------
 
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index bc0893e087..9981e0f6c9 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -651,6 +651,15 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
         qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegation",
                                aplic_child_phandle, 0x1,
                                VIRT_IRQCHIP_NUM_SOURCES);
+        /*
+         * DEPRECATED_9.1: Compat property kept temporarily
+         * to allow old firmwares to work with AIA. Do *not*
+         * use 'riscv,delegate' in new code: use
+         * 'riscv,delegation' instead.
+         */
+        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
+                               aplic_child_phandle, 0x1,
+                               VIRT_IRQCHIP_NUM_SOURCES);
     }
 
     riscv_socket_fdt_write_id(ms, aplic_name, socket);
-- 
2.45.2

Re: [PATCH] hw/riscv/virt.c: re-insert and deprecate 'riscv, delegate'

[Conor Dooley]

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

On Sat, Jul 13, 2024 at 02:43:25PM -0300, Daniel Henrique Barboza wrote:
> Commit b1f1e9dcfa renamed 'riscv,delegate' to 'riscv,delegation' since
> it is the correct name as per dt-bindings, and the absence of the
> correct name will result in validation fails when dumping the dtb and
> using dt-validate.
> 
> But this change has a side-effect: every other firmware available that
> is AIA capable is using 'riscv,delegate', and it will fault/misbehave if
> this property isn't present. The property was added back in QEMU 7.0,
> meaning we have 2 years of firmware development using the wrong
> property.
> 
> Re-introducing 'riscv,delegate' while keeping 'riscv,delegation' will
> make current firmwares to keep booting with the 'virt' machine and
> dt-validate won't complain about it since we're still using the expected
> property 'riscv,delegation'. 'riscv,delegate' is then marked for future
> deprecation and its use is being discouraged.

dt-validate /should/ complain about it - does yours not? It's probably a
bug if it doesn't. Whether dt-validate complains or not here I don't
think is relevant though, only the impact on firmware of removing it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] hw/riscv/virt.c: re-insert and deprecate 'riscv, delegate'

[Alistair Francis]

On Sun, Jul 14, 2024 at 3:44 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Commit b1f1e9dcfa renamed 'riscv,delegate' to 'riscv,delegation' since
> it is the correct name as per dt-bindings, and the absence of the
> correct name will result in validation fails when dumping the dtb and
> using dt-validate.
>
> But this change has a side-effect: every other firmware available that
> is AIA capable is using 'riscv,delegate', and it will fault/misbehave if
> this property isn't present. The property was added back in QEMU 7.0,
> meaning we have 2 years of firmware development using the wrong
> property.
>
> Re-introducing 'riscv,delegate' while keeping 'riscv,delegation' will
> make current firmwares to keep booting with the 'virt' machine and
> dt-validate won't complain about it since we're still using the expected
> property 'riscv,delegation'. 'riscv,delegate' is then marked for future
> deprecation and its use is being discouraged.
>
> Cc: Conor Dooley <conor@kernel.org>
> Cc: Anup Patel <apatel@ventanamicro.com>
> Fixes: b1f1e9dcfa ("hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation'")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  docs/about/deprecated.rst | 11 +++++++++++
>  hw/riscv/virt.c           |  9 +++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 20b7a17cf0..88f0f03786 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -479,6 +479,17 @@ versions, aliases will point to newer CPU model versions
>  depending on the machine type, so management software must
>  resolve CPU model aliases before starting a virtual machine.
>
> +RISC-V "virt" board "riscv,delegate" DT property (since 9.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The "riscv,delegate" DT property was added in QEMU 7.0 as part of
> +the AIA APLIC support.  The property changed name during the review
> +process in Linux and the correct name ended up being
> +"riscv,delegation".  Changing the DT property name will break all
> +available firmwares that are using the current (wrong) name.  The
> +property is kept as is in 9.1, together with "riscv,delegation", to
> +give more time for firmware developers to change their code.
> +
>  Migration
>  ---------
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index bc0893e087..9981e0f6c9 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -651,6 +651,15 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>          qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegation",
>                                 aplic_child_phandle, 0x1,
>                                 VIRT_IRQCHIP_NUM_SOURCES);
> +        /*
> +         * DEPRECATED_9.1: Compat property kept temporarily
> +         * to allow old firmwares to work with AIA. Do *not*
> +         * use 'riscv,delegate' in new code: use
> +         * 'riscv,delegation' instead.
> +         */
> +        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
> +                               aplic_child_phandle, 0x1,
> +                               VIRT_IRQCHIP_NUM_SOURCES);
>      }
>
>      riscv_socket_fdt_write_id(ms, aplic_name, socket);
> --
> 2.45.2
>
>

Re: [PATCH] hw/riscv/virt.c: re-insert and deprecate 'riscv, delegate'

[Alistair Francis]

On Sun, Jul 14, 2024 at 3:44 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Commit b1f1e9dcfa renamed 'riscv,delegate' to 'riscv,delegation' since
> it is the correct name as per dt-bindings, and the absence of the
> correct name will result in validation fails when dumping the dtb and
> using dt-validate.
>
> But this change has a side-effect: every other firmware available that
> is AIA capable is using 'riscv,delegate', and it will fault/misbehave if
> this property isn't present. The property was added back in QEMU 7.0,
> meaning we have 2 years of firmware development using the wrong
> property.
>
> Re-introducing 'riscv,delegate' while keeping 'riscv,delegation' will
> make current firmwares to keep booting with the 'virt' machine and
> dt-validate won't complain about it since we're still using the expected
> property 'riscv,delegation'. 'riscv,delegate' is then marked for future
> deprecation and its use is being discouraged.
>
> Cc: Conor Dooley <conor@kernel.org>
> Cc: Anup Patel <apatel@ventanamicro.com>
> Fixes: b1f1e9dcfa ("hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation'")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  docs/about/deprecated.rst | 11 +++++++++++
>  hw/riscv/virt.c           |  9 +++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 20b7a17cf0..88f0f03786 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -479,6 +479,17 @@ versions, aliases will point to newer CPU model versions
>  depending on the machine type, so management software must
>  resolve CPU model aliases before starting a virtual machine.
>
> +RISC-V "virt" board "riscv,delegate" DT property (since 9.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The "riscv,delegate" DT property was added in QEMU 7.0 as part of
> +the AIA APLIC support.  The property changed name during the review
> +process in Linux and the correct name ended up being
> +"riscv,delegation".  Changing the DT property name will break all
> +available firmwares that are using the current (wrong) name.  The
> +property is kept as is in 9.1, together with "riscv,delegation", to
> +give more time for firmware developers to change their code.
> +
>  Migration
>  ---------
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index bc0893e087..9981e0f6c9 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -651,6 +651,15 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>          qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegation",
>                                 aplic_child_phandle, 0x1,
>                                 VIRT_IRQCHIP_NUM_SOURCES);
> +        /*
> +         * DEPRECATED_9.1: Compat property kept temporarily
> +         * to allow old firmwares to work with AIA. Do *not*
> +         * use 'riscv,delegate' in new code: use
> +         * 'riscv,delegation' instead.
> +         */
> +        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
> +                               aplic_child_phandle, 0x1,
> +                               VIRT_IRQCHIP_NUM_SOURCES);
>      }
>
>      riscv_socket_fdt_write_id(ms, aplic_name, socket);
> --
> 2.45.2
>
>

Re: [PATCH] hw/riscv/virt.c: re-insert and deprecate 'riscv, delegate'

[Daniel Henrique Barboza]

On 7/13/24 3:57 PM, Conor Dooley wrote:
> On Sat, Jul 13, 2024 at 02:43:25PM -0300, Daniel Henrique Barboza wrote:
>> Commit b1f1e9dcfa renamed 'riscv,delegate' to 'riscv,delegation' since
>> it is the correct name as per dt-bindings, and the absence of the
>> correct name will result in validation fails when dumping the dtb and
>> using dt-validate.
>>
>> But this change has a side-effect: every other firmware available that
>> is AIA capable is using 'riscv,delegate', and it will fault/misbehave if
>> this property isn't present. The property was added back in QEMU 7.0,
>> meaning we have 2 years of firmware development using the wrong
>> property.
>>
>> Re-introducing 'riscv,delegate' while keeping 'riscv,delegation' will
>> make current firmwares to keep booting with the 'virt' machine and
>> dt-validate won't complain about it since we're still using the expected
>> property 'riscv,delegation'. 'riscv,delegate' is then marked for future
>> deprecation and its use is being discouraged.
> 
> dt-validate /should/ complain about it - does yours not? It's probably a
> bug if it doesn't. Whether dt-validate complains or not here I don't
> think is relevant though, only the impact on firmware of removing it.

We'll keep the property as a temporary documented band-aid that we'll remove
a few releases from now. OpenSBI was also updated to use the right property
recently so we shouldn't have any problems.

Oh, and dt-validate will complain about it if we use the right command line that
creates the controller ... something that I failed to do in v1 :(  I'll re-send
the patch with an updated commit msg.


Thanks,

Daniel

>