[PATCH for-8.1 0/1] hw/riscv/virt.c: fix 'aclint' prop regression

[PATCH for-8.1 0/1] hw/riscv/virt.c: fix 'aclint' prop regression

[Daniel Henrique Barboza]

Richard, Alistair,

I came across this gitlab bug earlier today. The bug itself was opened
yesterday:

https://gitlab.com/qemu-project/qemu/-/issues/1823

And turns out that this is a regression in the 'aclint' option that was
introduced in 8.1.

I'm aware that we're already in rc3 and kind of late, but even so I
marked this patch as 8.1 to let you decide whether it's worth spinning
rc4 or not.

Daniel Henrique Barboza (1):
  hw/riscv/virt.c: change 'aclint' TCG check

 hw/riscv/virt.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

-- 
2.41.0

[PATCH for-8.1 1/1] hw/riscv/virt.c: change 'aclint' TCG check

[Daniel Henrique Barboza]

The 'aclint' property is being conditioned with tcg acceleration in
virt_machine_class_init(). But acceleration code starts later than the
class init of the board, meaning that tcg_enabled() will be always be
false during class_init(), and the option is never being declared even
when declaring TCG accel:

$ ./build/qemu-system-riscv64 -M virt,accel=tcg,aclint=on
qemu-system-riscv64: Property 'virt-machine.aclint' not found

Fix it by moving the check from class_init() to machine_init(). Tune the
description to mention that the option is TCG only.

Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Fixes: c0716c81b ("hw/riscv/virt: Restrict ACLINT to TCG")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1823
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d90286dc46..99c4e6314b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1350,6 +1350,11 @@ static void virt_machine_init(MachineState *machine)
         exit(1);
     }
 
+    if (!tcg_enabled() && s->have_aclint) {
+        error_report("'aclint' is only available with TCG acceleration");
+        exit(1);
+    }
+
     /* Initialize sockets */
     mmio_irqchip = virtio_irqchip = pcie_irqchip = NULL;
     for (i = 0; i < socket_count; i++) {
@@ -1683,13 +1688,14 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
 #endif
 
-    if (tcg_enabled()) {
-        object_class_property_add_bool(oc, "aclint", virt_get_aclint,
-                                       virt_set_aclint);
-        object_class_property_set_description(oc, "aclint",
-                                              "Set on/off to enable/disable "
-                                              "emulating ACLINT devices");
-    }
+
+    object_class_property_add_bool(oc, "aclint", virt_get_aclint,
+                                   virt_set_aclint);
+    object_class_property_set_description(oc, "aclint",
+                                          "(TCG only) Set on/off to "
+                                          "enable/disable emulating "
+                                          "ACLINT devices");
+
     object_class_property_add_str(oc, "aia", virt_get_aia,
                                   virt_set_aia);
     object_class_property_set_description(oc, "aia",
-- 
2.41.0

Re: [PATCH for-8.1 1/1] hw/riscv/virt.c: change 'aclint' TCG check

[Philippe Mathieu-Daudé]

On 11/8/23 18:02, Daniel Henrique Barboza wrote:
> The 'aclint' property is being conditioned with tcg acceleration in
> virt_machine_class_init(). But acceleration code starts later than the
> class init of the board, meaning that tcg_enabled() will be always be
> false during class_init(), and the option is never being declared even
> when declaring TCG accel:
> 
> $ ./build/qemu-system-riscv64 -M virt,accel=tcg,aclint=on
> qemu-system-riscv64: Property 'virt-machine.aclint' not found
> 
> Fix it by moving the check from class_init() to machine_init(). Tune the
> description to mention that the option is TCG only.
> 
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Fixes: c0716c81b ("hw/riscv/virt: Restrict ACLINT to TCG")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1823
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/virt.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)

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

Re: [PATCH for-8.1 1/1] hw/riscv/virt.c: change 'aclint' TCG check

[Alistair Francis]

On Fri, Aug 11, 2023 at 12:03 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The 'aclint' property is being conditioned with tcg acceleration in
> virt_machine_class_init(). But acceleration code starts later than the
> class init of the board, meaning that tcg_enabled() will be always be
> false during class_init(), and the option is never being declared even
> when declaring TCG accel:
>
> $ ./build/qemu-system-riscv64 -M virt,accel=tcg,aclint=on
> qemu-system-riscv64: Property 'virt-machine.aclint' not found
>
> Fix it by moving the check from class_init() to machine_init(). Tune the
> description to mention that the option is TCG only.
>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Fixes: c0716c81b ("hw/riscv/virt: Restrict ACLINT to TCG")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1823
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d90286dc46..99c4e6314b 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1350,6 +1350,11 @@ static void virt_machine_init(MachineState *machine)
>          exit(1);
>      }
>
> +    if (!tcg_enabled() && s->have_aclint) {
> +        error_report("'aclint' is only available with TCG acceleration");
> +        exit(1);
> +    }
> +
>      /* Initialize sockets */
>      mmio_irqchip = virtio_irqchip = pcie_irqchip = NULL;
>      for (i = 0; i < socket_count; i++) {
> @@ -1683,13 +1688,14 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
>  #endif
>
> -    if (tcg_enabled()) {
> -        object_class_property_add_bool(oc, "aclint", virt_get_aclint,
> -                                       virt_set_aclint);
> -        object_class_property_set_description(oc, "aclint",
> -                                              "Set on/off to enable/disable "
> -                                              "emulating ACLINT devices");
> -    }
> +
> +    object_class_property_add_bool(oc, "aclint", virt_get_aclint,
> +                                   virt_set_aclint);
> +    object_class_property_set_description(oc, "aclint",
> +                                          "(TCG only) Set on/off to "
> +                                          "enable/disable emulating "
> +                                          "ACLINT devices");
> +
>      object_class_property_add_str(oc, "aia", virt_get_aia,
>                                    virt_set_aia);
>      object_class_property_set_description(oc, "aia",
> --
> 2.41.0
>
>

Re: [PATCH for-8.1 0/1] hw/riscv/virt.c: fix 'aclint' prop regression

[Alistair Francis]

On Fri, Aug 11, 2023 at 12:03 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Richard, Alistair,
>
> I came across this gitlab bug earlier today. The bug itself was opened
> yesterday:
>
> https://gitlab.com/qemu-project/qemu/-/issues/1823
>
> And turns out that this is a regression in the 'aclint' option that was
> introduced in 8.1.
>
> I'm aware that we're already in rc3 and kind of late, but even so I
> marked this patch as 8.1 to let you decide whether it's worth spinning
> rc4 or not.

Urgh! What a pain!

I'll fire off a PR now as this is worth fixing

Alistair

>
> Daniel Henrique Barboza (1):
>   hw/riscv/virt.c: change 'aclint' TCG check
>
>  hw/riscv/virt.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> --
> 2.41.0
>
>