[PATCH 0/4] target/riscv/kvm: add riscv-aia bool props

[PATCH 0/4] target/riscv/kvm: add riscv-aia bool props

[Daniel Henrique Barboza]

Hi,

Boolean properties are easier to deal with in the protocol side (e.g.
QMP) since they don't require string parsing. We should always use them
when applicable.

This series adds 3 new riscv-aia bool options for the KVM accel driver,
each one representing the possible values (emul, hwaccel and auto).
We're also deprecating the existing 'riscv-aia' string option. 

The idea is to use the new properties to enable AIA support in libvirt.

Patches based on riscv-to-apply.next.

Daniel Henrique Barboza (4):
  target/riscv/kvm: set 'aia_mode' to default in error path
  target/riscv/kvm: clarify how 'riscv-aia' default works
  target/riscv/kvm: add kvm-aia bools props
  target/riscv/kvm: deprecate riscv-aia string prop

 docs/about/deprecated.rst  |   8 +++
 target/riscv/kvm/kvm-cpu.c | 100 +++++++++++++++++++++++++++++++++----
 2 files changed, 98 insertions(+), 10 deletions(-)

-- 
2.45.2

[PATCH 1/4] target/riscv/kvm: set 'aia_mode' to default in error path

[Daniel Henrique Barboza]

When failing to set the selected AIA mode, 'aia_mode' is left untouched.
This means that 'aia_mode' will not reflect the actual AIA mode,
retrieved in 'default_aia_mode',

This is benign for now, but it will impact QMP query commands that will
expose the 'aia_mode' value, retrieving the wrong value.

Set 'aia_mode' to 'default_aia_mode' if we fail to change the AIA mode
in KVM.

While we're at it, rework the log/warning messages to be a bit less
verbose. Instead of:

KVM AIA: default mode is emul
qemu-system-riscv64: warning: KVM AIA: failed to set KVM AIA mode

We can use a single warning message:

qemu-system-riscv64: warning: KVM AIA: failed to set KVM AIA mode 'auto', using default host mode 'emul'

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 341af901c5..970a7ab2f1 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1711,18 +1711,26 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
         error_report("KVM AIA: failed to get current KVM AIA mode");
         exit(1);
     }
-    qemu_log("KVM AIA: default mode is %s\n",
-             kvm_aia_mode_str(default_aia_mode));
 
-    if (default_aia_mode != aia_mode) {
+    if (default_aia_mode == aia_mode) {
+        qemu_log("KVM AIA: using default host mode '%s'\n",
+                  kvm_aia_mode_str(default_aia_mode));
+    } else {
         ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
                                 KVM_DEV_RISCV_AIA_CONFIG_MODE,
                                 &aia_mode, true, NULL);
-        if (ret < 0)
-            warn_report("KVM AIA: failed to set KVM AIA mode");
-        else
-            qemu_log("KVM AIA: set current mode to %s\n",
+        if (ret < 0) {
+            warn_report("KVM AIA: failed to set KVM AIA mode '%s', using "
+                        "default host mode '%s'",
+                        kvm_aia_mode_str(aia_mode),
+                        kvm_aia_mode_str(default_aia_mode));
+
+            /* failed to change AIA mode, use default */
+            aia_mode = default_aia_mode;
+        } else {
+            qemu_log("KVM AIA: setting current mode to %s\n",
                      kvm_aia_mode_str(aia_mode));
+        }
     }
 
     ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
-- 
2.45.2

[PATCH 2/4] target/riscv/kvm: clarify how 'riscv-aia' default works

[Daniel Henrique Barboza]

We do not have control in the default 'riscv-aia' default value. We can
try to set it to a specific value, in this case 'auto', but there's no
guarantee that the host will accept it.

Couple with this we're always doing a 'qemu_log' to inform whether we're
ended up using the host default or if we managed to set the AIA mode to
the QEMU default we wanted to set.

Change the 'riscv-aia' description to better reflect how the option
works, and remove the two informative 'qemu_log' that are now unneeded:
if no message shows, riscv-aia was set to the default or uset-set value.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 970a7ab2f1..32f3dd6a43 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1676,9 +1676,9 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
     object_class_property_add_str(oc, "riscv-aia", riscv_get_kvm_aia,
                                   riscv_set_kvm_aia);
     object_class_property_set_description(oc, "riscv-aia",
-                                          "Set KVM AIA mode. Valid values are "
-                                          "emul, hwaccel, and auto. Default "
-                                          "is auto.");
+        "Set KVM AIA mode. Valid values are 'emul', 'hwaccel' and 'auto'. "
+        "Changing KVM AIA modes relies on host support. Defaults to 'auto' "
+        "if the host supports it");
     object_property_set_default_str(object_class_property_find(oc, "riscv-aia"),
                                     "auto");
 }
@@ -1712,10 +1712,7 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
         exit(1);
     }
 
-    if (default_aia_mode == aia_mode) {
-        qemu_log("KVM AIA: using default host mode '%s'\n",
-                  kvm_aia_mode_str(default_aia_mode));
-    } else {
+    if (default_aia_mode != aia_mode) {
         ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
                                 KVM_DEV_RISCV_AIA_CONFIG_MODE,
                                 &aia_mode, true, NULL);
@@ -1727,9 +1724,6 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
 
             /* failed to change AIA mode, use default */
             aia_mode = default_aia_mode;
-        } else {
-            qemu_log("KVM AIA: setting current mode to %s\n",
-                     kvm_aia_mode_str(aia_mode));
         }
     }
 
-- 
2.45.2

[PATCH 3/4] target/riscv/kvm: add kvm-aia bools props

[Daniel Henrique Barboza]

Boolean properties are preferrable in comparision to string properties
since they don't require a string parsing.

Add three bools that represents the available kvm-aia mode:
riscv-aia-emul, riscv-aia-hwaccel, riscv-aia-auto. They work like the
existing riscv-aia string property, i.e. if no bool is set we'll default
to riscv-aia-auto, if the host supports it.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 77 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 32f3dd6a43..e256e3fc48 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1671,6 +1671,62 @@ static void riscv_set_kvm_aia(Object *obj, const char *val, Error **errp)
     }
 }
 
+static void riscv_set_kvm_aia_bool(uint32_t aia_bool, bool val)
+{
+    bool default_aia_mode = KVM_DEV_RISCV_AIA_MODE_AUTO;
+
+    g_assert(aia_bool <= KVM_DEV_RISCV_AIA_MODE_AUTO);
+
+    if (val) {
+        aia_mode = aia_bool;
+        return;
+    }
+
+    /*
+     * Setting an aia_bool to 'false' does nothing if
+     * aia_mode isn't set to aia_bool.
+     */
+    if (aia_mode != aia_bool) {
+        return;
+    }
+
+    /*
+     * Return to default value if we're disabling the
+     * current set aia_mode.
+     */
+    aia_mode = default_aia_mode;
+}
+
+static bool riscv_get_kvm_aia_emul(Object *obj, Error **errp)
+{
+    return aia_mode == KVM_DEV_RISCV_AIA_MODE_EMUL;
+}
+
+static void riscv_set_kvm_aia_emul(Object *obj,  bool val, Error **errp)
+{
+    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_EMUL, val);
+}
+
+static bool riscv_get_kvm_aia_hwaccel(Object *obj, Error **errp)
+{
+    return aia_mode == KVM_DEV_RISCV_AIA_MODE_HWACCEL;
+}
+
+static void riscv_set_kvm_aia_hwaccel(Object *obj,  bool val, Error **errp)
+{
+    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_HWACCEL, val);
+}
+
+static bool riscv_get_kvm_aia_auto(Object *obj, Error **errp)
+{
+    return aia_mode == KVM_DEV_RISCV_AIA_MODE_AUTO;
+}
+
+static void riscv_set_kvm_aia_auto(Object *obj,  bool val, Error **errp)
+{
+    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_AUTO, val);
+}
+
 void kvm_arch_accel_class_init(ObjectClass *oc)
 {
     object_class_property_add_str(oc, "riscv-aia", riscv_get_kvm_aia,
@@ -1681,6 +1737,27 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
         "if the host supports it");
     object_property_set_default_str(object_class_property_find(oc, "riscv-aia"),
                                     "auto");
+
+    object_class_property_add_bool(oc, "riscv-aia-emul",
+                                   riscv_get_kvm_aia_emul,
+                                   riscv_set_kvm_aia_emul);
+    object_class_property_set_description(oc, "riscv-aia-emul",
+        "Set KVM AIA mode to 'emul'. Changing KVM AIA modes relies on host "
+        "support. Default mode is 'auto' if the host supports it");
+
+    object_class_property_add_bool(oc, "riscv-aia-hwaccel",
+                                   riscv_get_kvm_aia_hwaccel,
+                                   riscv_set_kvm_aia_hwaccel);
+    object_class_property_set_description(oc, "riscv-aia-hwaccel",
+        "Set KVM AIA mode to 'hwaccel'. Changing KVM AIA modes relies on host "
+        "support. Default mode is 'auto' if the host supports it");
+
+    object_class_property_add_bool(oc, "riscv-aia-auto",
+                                   riscv_get_kvm_aia_auto,
+                                   riscv_set_kvm_aia_auto);
+    object_class_property_set_description(oc, "riscv-aia-auto",
+        "Set KVM AIA mode to 'auto'. Changing KVM AIA modes "
+        "relies on host support");
 }
 
 void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
-- 
2.45.2

[PATCH 4/4] target/riscv/kvm: deprecate riscv-aia string prop

[Daniel Henrique Barboza]

We want to use the new boolean properties instead: riscv-aia-emul,
riscv-aia-hwaccel and riscv-aia-auto.

Mark the string prop 'riscv-aia' for deprecation.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 docs/about/deprecated.rst  | 8 ++++++++
 target/riscv/kvm/kvm-cpu.c | 1 +
 2 files changed, 9 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index ed31d4b0b2..d3e0e3e303 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -463,6 +463,14 @@ 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.
 
+RISC-V "riscv-aia" KVM property (since 9.2)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+This property was deprecated in favor of using the boolean properties
+'riscv-aia-emul', 'riscv-aia-hwaccel' and 'riscv-aia-auto'.  The
+motivation behind it is to make it easier to expose the internal
+state of the KVM accelerator in APIs such as query-cpu-model-expansion.
+
 Migration
 ---------
 
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index e256e3fc48..610057870f 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1732,6 +1732,7 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
     object_class_property_add_str(oc, "riscv-aia", riscv_get_kvm_aia,
                                   riscv_set_kvm_aia);
     object_class_property_set_description(oc, "riscv-aia",
+        "DEPRECATED: use riscv-aia-<mode> properties instead. "
         "Set KVM AIA mode. Valid values are 'emul', 'hwaccel' and 'auto'. "
         "Changing KVM AIA modes relies on host support. Defaults to 'auto' "
         "if the host supports it");
-- 
2.45.2

Re: [PATCH 1/4] target/riscv/kvm: set 'aia_mode' to default in error path

[Alistair Francis]

On Tue, Sep 24, 2024 at 10:46 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> When failing to set the selected AIA mode, 'aia_mode' is left untouched.
> This means that 'aia_mode' will not reflect the actual AIA mode,
> retrieved in 'default_aia_mode',
>
> This is benign for now, but it will impact QMP query commands that will
> expose the 'aia_mode' value, retrieving the wrong value.
>
> Set 'aia_mode' to 'default_aia_mode' if we fail to change the AIA mode
> in KVM.
>
> While we're at it, rework the log/warning messages to be a bit less
> verbose. Instead of:
>
> KVM AIA: default mode is emul
> qemu-system-riscv64: warning: KVM AIA: failed to set KVM AIA mode
>
> We can use a single warning message:
>
> qemu-system-riscv64: warning: KVM AIA: failed to set KVM AIA mode 'auto', using default host mode 'emul'
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/kvm/kvm-cpu.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 341af901c5..970a7ab2f1 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1711,18 +1711,26 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>          error_report("KVM AIA: failed to get current KVM AIA mode");
>          exit(1);
>      }
> -    qemu_log("KVM AIA: default mode is %s\n",
> -             kvm_aia_mode_str(default_aia_mode));
>
> -    if (default_aia_mode != aia_mode) {
> +    if (default_aia_mode == aia_mode) {
> +        qemu_log("KVM AIA: using default host mode '%s'\n",
> +                  kvm_aia_mode_str(default_aia_mode));
> +    } else {
>          ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
>                                  KVM_DEV_RISCV_AIA_CONFIG_MODE,
>                                  &aia_mode, true, NULL);
> -        if (ret < 0)
> -            warn_report("KVM AIA: failed to set KVM AIA mode");
> -        else
> -            qemu_log("KVM AIA: set current mode to %s\n",
> +        if (ret < 0) {
> +            warn_report("KVM AIA: failed to set KVM AIA mode '%s', using "
> +                        "default host mode '%s'",
> +                        kvm_aia_mode_str(aia_mode),
> +                        kvm_aia_mode_str(default_aia_mode));
> +
> +            /* failed to change AIA mode, use default */
> +            aia_mode = default_aia_mode;
> +        } else {
> +            qemu_log("KVM AIA: setting current mode to %s\n",
>                       kvm_aia_mode_str(aia_mode));
> +        }
>      }
>
>      ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
> --
> 2.45.2
>
>

Re: [PATCH 2/4] target/riscv/kvm: clarify how 'riscv-aia' default works

[Alistair Francis]

On Tue, Sep 24, 2024 at 10:46 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We do not have control in the default 'riscv-aia' default value. We can
> try to set it to a specific value, in this case 'auto', but there's no
> guarantee that the host will accept it.
>
> Couple with this we're always doing a 'qemu_log' to inform whether we're
> ended up using the host default or if we managed to set the AIA mode to
> the QEMU default we wanted to set.
>
> Change the 'riscv-aia' description to better reflect how the option
> works, and remove the two informative 'qemu_log' that are now unneeded:
> if no message shows, riscv-aia was set to the default or uset-set value.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/kvm/kvm-cpu.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 970a7ab2f1..32f3dd6a43 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1676,9 +1676,9 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
>      object_class_property_add_str(oc, "riscv-aia", riscv_get_kvm_aia,
>                                    riscv_set_kvm_aia);
>      object_class_property_set_description(oc, "riscv-aia",
> -                                          "Set KVM AIA mode. Valid values are "
> -                                          "emul, hwaccel, and auto. Default "
> -                                          "is auto.");
> +        "Set KVM AIA mode. Valid values are 'emul', 'hwaccel' and 'auto'. "
> +        "Changing KVM AIA modes relies on host support. Defaults to 'auto' "
> +        "if the host supports it");
>      object_property_set_default_str(object_class_property_find(oc, "riscv-aia"),
>                                      "auto");
>  }
> @@ -1712,10 +1712,7 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>          exit(1);
>      }
>
> -    if (default_aia_mode == aia_mode) {
> -        qemu_log("KVM AIA: using default host mode '%s'\n",
> -                  kvm_aia_mode_str(default_aia_mode));
> -    } else {
> +    if (default_aia_mode != aia_mode) {
>          ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
>                                  KVM_DEV_RISCV_AIA_CONFIG_MODE,
>                                  &aia_mode, true, NULL);
> @@ -1727,9 +1724,6 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>
>              /* failed to change AIA mode, use default */
>              aia_mode = default_aia_mode;
> -        } else {
> -            qemu_log("KVM AIA: setting current mode to %s\n",
> -                     kvm_aia_mode_str(aia_mode));
>          }
>      }
>
> --
> 2.45.2
>
>

Re: [PATCH 3/4] target/riscv/kvm: add kvm-aia bools props

[Alistair Francis]

On Tue, Sep 24, 2024 at 10:46 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Boolean properties are preferrable in comparision to string properties
> since they don't require a string parsing.
>
> Add three bools that represents the available kvm-aia mode:
> riscv-aia-emul, riscv-aia-hwaccel, riscv-aia-auto. They work like the
> existing riscv-aia string property, i.e. if no bool is set we'll default
> to riscv-aia-auto, if the host supports it.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm/kvm-cpu.c | 77 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 32f3dd6a43..e256e3fc48 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1671,6 +1671,62 @@ static void riscv_set_kvm_aia(Object *obj, const char *val, Error **errp)
>      }
>  }
>
> +static void riscv_set_kvm_aia_bool(uint32_t aia_bool, bool val)
> +{
> +    bool default_aia_mode = KVM_DEV_RISCV_AIA_MODE_AUTO;
> +
> +    g_assert(aia_bool <= KVM_DEV_RISCV_AIA_MODE_AUTO);
> +
> +    if (val) {
> +        aia_mode = aia_bool;
> +        return;
> +    }
> +
> +    /*
> +     * Setting an aia_bool to 'false' does nothing if
> +     * aia_mode isn't set to aia_bool.
> +     */
> +    if (aia_mode != aia_bool) {
> +        return;
> +    }
> +
> +    /*
> +     * Return to default value if we're disabling the
> +     * current set aia_mode.
> +     */
> +    aia_mode = default_aia_mode;
> +}
> +
> +static bool riscv_get_kvm_aia_emul(Object *obj, Error **errp)
> +{
> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_EMUL;
> +}
> +
> +static void riscv_set_kvm_aia_emul(Object *obj,  bool val, Error **errp)
> +{
> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_EMUL, val);
> +}
> +
> +static bool riscv_get_kvm_aia_hwaccel(Object *obj, Error **errp)
> +{
> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_HWACCEL;
> +}
> +
> +static void riscv_set_kvm_aia_hwaccel(Object *obj,  bool val, Error **errp)
> +{
> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_HWACCEL, val);
> +}
> +
> +static bool riscv_get_kvm_aia_auto(Object *obj, Error **errp)
> +{
> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_AUTO;
> +}
> +
> +static void riscv_set_kvm_aia_auto(Object *obj,  bool val, Error **errp)
> +{
> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_AUTO, val);
> +}
> +
>  void kvm_arch_accel_class_init(ObjectClass *oc)
>  {
>      object_class_property_add_str(oc, "riscv-aia", riscv_get_kvm_aia,
> @@ -1681,6 +1737,27 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
>          "if the host supports it");
>      object_property_set_default_str(object_class_property_find(oc, "riscv-aia"),
>                                      "auto");
> +
> +    object_class_property_add_bool(oc, "riscv-aia-emul",
> +                                   riscv_get_kvm_aia_emul,
> +                                   riscv_set_kvm_aia_emul);
> +    object_class_property_set_description(oc, "riscv-aia-emul",
> +        "Set KVM AIA mode to 'emul'. Changing KVM AIA modes relies on host "
> +        "support. Default mode is 'auto' if the host supports it");
> +
> +    object_class_property_add_bool(oc, "riscv-aia-hwaccel",
> +                                   riscv_get_kvm_aia_hwaccel,
> +                                   riscv_set_kvm_aia_hwaccel);
> +    object_class_property_set_description(oc, "riscv-aia-hwaccel",
> +        "Set KVM AIA mode to 'hwaccel'. Changing KVM AIA modes relies on host "
> +        "support. Default mode is 'auto' if the host supports it");
> +
> +    object_class_property_add_bool(oc, "riscv-aia-auto",
> +                                   riscv_get_kvm_aia_auto,
> +                                   riscv_set_kvm_aia_auto);
> +    object_class_property_set_description(oc, "riscv-aia-auto",
> +        "Set KVM AIA mode to 'auto'. Changing KVM AIA modes "
> +        "relies on host support");

This seems more confusing. What should happen if a user sets multiple to true?

Alistair

>  }
>
>  void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
> --
> 2.45.2
>
>

Re: [PATCH 3/4] target/riscv/kvm: add kvm-aia bools props

[Daniel Henrique Barboza]

On 10/10/24 10:57 PM, Alistair Francis wrote:
> On Tue, Sep 24, 2024 at 10:46 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> Boolean properties are preferrable in comparision to string properties
>> since they don't require a string parsing.
>>
>> Add three bools that represents the available kvm-aia mode:
>> riscv-aia-emul, riscv-aia-hwaccel, riscv-aia-auto. They work like the
>> existing riscv-aia string property, i.e. if no bool is set we'll default
>> to riscv-aia-auto, if the host supports it.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/kvm/kvm-cpu.c | 77 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 77 insertions(+)
>>
>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>> index 32f3dd6a43..e256e3fc48 100644
>> --- a/target/riscv/kvm/kvm-cpu.c
>> +++ b/target/riscv/kvm/kvm-cpu.c
>> @@ -1671,6 +1671,62 @@ static void riscv_set_kvm_aia(Object *obj, const char *val, Error **errp)
>>       }
>>   }
>>
>> +static void riscv_set_kvm_aia_bool(uint32_t aia_bool, bool val)
>> +{
>> +    bool default_aia_mode = KVM_DEV_RISCV_AIA_MODE_AUTO;
>> +
>> +    g_assert(aia_bool <= KVM_DEV_RISCV_AIA_MODE_AUTO);
>> +
>> +    if (val) {
>> +        aia_mode = aia_bool;
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Setting an aia_bool to 'false' does nothing if
>> +     * aia_mode isn't set to aia_bool.
>> +     */
>> +    if (aia_mode != aia_bool) {
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Return to default value if we're disabling the
>> +     * current set aia_mode.
>> +     */
>> +    aia_mode = default_aia_mode;
>> +}
>> +
>> +static bool riscv_get_kvm_aia_emul(Object *obj, Error **errp)
>> +{
>> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_EMUL;
>> +}
>> +
>> +static void riscv_set_kvm_aia_emul(Object *obj,  bool val, Error **errp)
>> +{
>> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_EMUL, val);
>> +}
>> +
>> +static bool riscv_get_kvm_aia_hwaccel(Object *obj, Error **errp)
>> +{
>> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_HWACCEL;
>> +}
>> +
>> +static void riscv_set_kvm_aia_hwaccel(Object *obj,  bool val, Error **errp)
>> +{
>> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_HWACCEL, val);
>> +}
>> +
>> +static bool riscv_get_kvm_aia_auto(Object *obj, Error **errp)
>> +{
>> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_AUTO;
>> +}
>> +
>> +static void riscv_set_kvm_aia_auto(Object *obj,  bool val, Error **errp)
>> +{
>> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_AUTO, val);
>> +}
>> +
>>   void kvm_arch_accel_class_init(ObjectClass *oc)
>>   {
>>       object_class_property_add_str(oc, "riscv-aia", riscv_get_kvm_aia,
>> @@ -1681,6 +1737,27 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
>>           "if the host supports it");
>>       object_property_set_default_str(object_class_property_find(oc, "riscv-aia"),
>>                                       "auto");
>> +
>> +    object_class_property_add_bool(oc, "riscv-aia-emul",
>> +                                   riscv_get_kvm_aia_emul,
>> +                                   riscv_set_kvm_aia_emul);
>> +    object_class_property_set_description(oc, "riscv-aia-emul",
>> +        "Set KVM AIA mode to 'emul'. Changing KVM AIA modes relies on host "
>> +        "support. Default mode is 'auto' if the host supports it");
>> +
>> +    object_class_property_add_bool(oc, "riscv-aia-hwaccel",
>> +                                   riscv_get_kvm_aia_hwaccel,
>> +                                   riscv_set_kvm_aia_hwaccel);
>> +    object_class_property_set_description(oc, "riscv-aia-hwaccel",
>> +        "Set KVM AIA mode to 'hwaccel'. Changing KVM AIA modes relies on host "
>> +        "support. Default mode is 'auto' if the host supports it");
>> +
>> +    object_class_property_add_bool(oc, "riscv-aia-auto",
>> +                                   riscv_get_kvm_aia_auto,
>> +                                   riscv_set_kvm_aia_auto);
>> +    object_class_property_set_description(oc, "riscv-aia-auto",
>> +        "Set KVM AIA mode to 'auto'. Changing KVM AIA modes "
>> +        "relies on host support");
> 
> This seems more confusing. What should happen if a user sets multiple to true?

It'll work like most options in QEMU: the last setting will overwrite the previous
ones. "-accel kvm,riscv-aia-hwaccel=true,riscv-aia-emul=true" will set the mode
to 'emul'. This is the same behavior that we have with the existing 'riscv-aia'
string option.

In case someone tries it out with multiple -accel options, this doesn't work. Only
the first '-accel <type>' are parsed. This happens due to a known command line
parsing/accel globals issue that I tried to fix in [1] and [2].

For now, using the existing 'riscv-aia' string option:

-accel kvm,riscv-aia=emul -accel kvm,riscv-aia=hwaccel -accel kvm,riscv-aia=auto

This will set riscv-aia to "emul" because all other "-accel kvm" options aren't
being parsed. You can do silly stuff like:

-accel kvm,riscv-aia=emul -accel kvm,riscv-aia=this_is_not_an_option

And the guest will boot normally, setting riscv-aia to 'emul'.


Thanks,

Daniel


[1] "[PATCH 0/2] system/vl.c: parse all '-accel' opts"
     https://lore.kernel.org/qemu-devel/20240701133038.1489043-1-dbarboza@ventanamicro.com/
[2] "[PATCH v2 0/2] object,accel-system: allow Accel type globals"
     https://lore.kernel.org/qemu-devel/20240703204149.1957136-1-dbarboza@ventanamicro.com/


> 
> Alistair
> 
>>   }
>>
>>   void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>> --
>> 2.45.2
>>
>>

Re: [PATCH 0/4] target/riscv/kvm: add riscv-aia bool props

[Daniel Henrique Barboza]

Hi,

I had a change of heart w.r.t this work. I still believe that the boolean properties
are better to deal with since we don't have to deal with string parsing, and that we
should avoid creating new string props in the future.

But as far as the user API goes it doesn't matter that much. Having to do

-accel kvm,riscv-aia=emul

or

-accel kvm,riscv-aia-emul=on

is basically the same thing. Deprecate properties always creates some form of hassle
for existing scripts and whatnot and we should avoid it.

String properties aren't that great to report to APIs though, so what we can do is to
create internal bools to track the string value and then use it for QMP.


Long story short, I'll re-send this series with only patches 1 and 2. Thanks,


Daniel



On 9/24/24 9:44 AM, Daniel Henrique Barboza wrote:
> Hi,
> 
> Boolean properties are easier to deal with in the protocol side (e.g.
> QMP) since they don't require string parsing. We should always use them
> when applicable.
> 
> This series adds 3 new riscv-aia bool options for the KVM accel driver,
> each one representing the possible values (emul, hwaccel and auto).
> We're also deprecating the existing 'riscv-aia' string option.
> 
> The idea is to use the new properties to enable AIA support in libvirt.
> 
> Patches based on riscv-to-apply.next.
> 
> Daniel Henrique Barboza (4):
>    target/riscv/kvm: set 'aia_mode' to default in error path
>    target/riscv/kvm: clarify how 'riscv-aia' default works
>    target/riscv/kvm: add kvm-aia bools props
>    target/riscv/kvm: deprecate riscv-aia string prop
> 
>   docs/about/deprecated.rst  |   8 +++
>   target/riscv/kvm/kvm-cpu.c | 100 +++++++++++++++++++++++++++++++++----
>   2 files changed, 98 insertions(+), 10 deletions(-)
> 

Re: [PATCH 3/4] target/riscv/kvm: add kvm-aia bools props

[Alistair Francis]

On Fri, Oct 11, 2024 at 9:19 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/10/24 10:57 PM, Alistair Francis wrote:
> > On Tue, Sep 24, 2024 at 10:46 PM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> Boolean properties are preferrable in comparision to string properties
> >> since they don't require a string parsing.
> >>
> >> Add three bools that represents the available kvm-aia mode:
> >> riscv-aia-emul, riscv-aia-hwaccel, riscv-aia-auto. They work like the
> >> existing riscv-aia string property, i.e. if no bool is set we'll default
> >> to riscv-aia-auto, if the host supports it.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> ---
> >>   target/riscv/kvm/kvm-cpu.c | 77 ++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 77 insertions(+)
> >>
> >> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> >> index 32f3dd6a43..e256e3fc48 100644
> >> --- a/target/riscv/kvm/kvm-cpu.c
> >> +++ b/target/riscv/kvm/kvm-cpu.c
> >> @@ -1671,6 +1671,62 @@ static void riscv_set_kvm_aia(Object *obj, const char *val, Error **errp)
> >>       }
> >>   }
> >>
> >> +static void riscv_set_kvm_aia_bool(uint32_t aia_bool, bool val)
> >> +{
> >> +    bool default_aia_mode = KVM_DEV_RISCV_AIA_MODE_AUTO;
> >> +
> >> +    g_assert(aia_bool <= KVM_DEV_RISCV_AIA_MODE_AUTO);
> >> +
> >> +    if (val) {
> >> +        aia_mode = aia_bool;
> >> +        return;
> >> +    }
> >> +
> >> +    /*
> >> +     * Setting an aia_bool to 'false' does nothing if
> >> +     * aia_mode isn't set to aia_bool.
> >> +     */
> >> +    if (aia_mode != aia_bool) {
> >> +        return;
> >> +    }
> >> +
> >> +    /*
> >> +     * Return to default value if we're disabling the
> >> +     * current set aia_mode.
> >> +     */
> >> +    aia_mode = default_aia_mode;
> >> +}
> >> +
> >> +static bool riscv_get_kvm_aia_emul(Object *obj, Error **errp)
> >> +{
> >> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_EMUL;
> >> +}
> >> +
> >> +static void riscv_set_kvm_aia_emul(Object *obj,  bool val, Error **errp)
> >> +{
> >> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_EMUL, val);
> >> +}
> >> +
> >> +static bool riscv_get_kvm_aia_hwaccel(Object *obj, Error **errp)
> >> +{
> >> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_HWACCEL;
> >> +}
> >> +
> >> +static void riscv_set_kvm_aia_hwaccel(Object *obj,  bool val, Error **errp)
> >> +{
> >> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_HWACCEL, val);
> >> +}
> >> +
> >> +static bool riscv_get_kvm_aia_auto(Object *obj, Error **errp)
> >> +{
> >> +    return aia_mode == KVM_DEV_RISCV_AIA_MODE_AUTO;
> >> +}
> >> +
> >> +static void riscv_set_kvm_aia_auto(Object *obj,  bool val, Error **errp)
> >> +{
> >> +    riscv_set_kvm_aia_bool(KVM_DEV_RISCV_AIA_MODE_AUTO, val);
> >> +}
> >> +
> >>   void kvm_arch_accel_class_init(ObjectClass *oc)
> >>   {
> >>       object_class_property_add_str(oc, "riscv-aia", riscv_get_kvm_aia,
> >> @@ -1681,6 +1737,27 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
> >>           "if the host supports it");
> >>       object_property_set_default_str(object_class_property_find(oc, "riscv-aia"),
> >>                                       "auto");
> >> +
> >> +    object_class_property_add_bool(oc, "riscv-aia-emul",
> >> +                                   riscv_get_kvm_aia_emul,
> >> +                                   riscv_set_kvm_aia_emul);
> >> +    object_class_property_set_description(oc, "riscv-aia-emul",
> >> +        "Set KVM AIA mode to 'emul'. Changing KVM AIA modes relies on host "
> >> +        "support. Default mode is 'auto' if the host supports it");
> >> +
> >> +    object_class_property_add_bool(oc, "riscv-aia-hwaccel",
> >> +                                   riscv_get_kvm_aia_hwaccel,
> >> +                                   riscv_set_kvm_aia_hwaccel);
> >> +    object_class_property_set_description(oc, "riscv-aia-hwaccel",
> >> +        "Set KVM AIA mode to 'hwaccel'. Changing KVM AIA modes relies on host "
> >> +        "support. Default mode is 'auto' if the host supports it");
> >> +
> >> +    object_class_property_add_bool(oc, "riscv-aia-auto",
> >> +                                   riscv_get_kvm_aia_auto,
> >> +                                   riscv_set_kvm_aia_auto);
> >> +    object_class_property_set_description(oc, "riscv-aia-auto",
> >> +        "Set KVM AIA mode to 'auto'. Changing KVM AIA modes "
> >> +        "relies on host support");
> >
> > This seems more confusing. What should happen if a user sets multiple to true?
>
> It'll work like most options in QEMU: the last setting will overwrite the previous
> ones. "-accel kvm,riscv-aia-hwaccel=true,riscv-aia-emul=true" will set the mode
> to 'emul'. This is the same behavior that we have with the existing 'riscv-aia'
> string option.

To me, reading "-accel kvm,riscv-aia-hwaccel=true,riscv-aia-emul=true"
means that the `riscv-aia-hwaccel` and the `riscv-aia-emul` features
are enabled.

Converting a single multi-option property to a range of bools just
feels strange. It seems more confusing to users.

I agree that not requiring string parsing is nice, but this doesn't
really seem worth it

>
> In case someone tries it out with multiple -accel options, this doesn't work. Only
> the first '-accel <type>' are parsed. This happens due to a known command line
> parsing/accel globals issue that I tried to fix in [1] and [2].
>
> For now, using the existing 'riscv-aia' string option:
>
> -accel kvm,riscv-aia=emul -accel kvm,riscv-aia=hwaccel -accel kvm,riscv-aia=auto
>
> This will set riscv-aia to "emul" because all other "-accel kvm" options aren't
> being parsed. You can do silly stuff like:
>
> -accel kvm,riscv-aia=emul -accel kvm,riscv-aia=this_is_not_an_option
>
> And the guest will boot normally, setting riscv-aia to 'emul'.

Both of those are unfortunate, but I do at least feel that reading
them it's clear that something is wrong as the user has listed `-accel
kvm...` multiple times.

Alistair

Re: [PATCH 0/4] target/riscv/kvm: add riscv-aia bool props

[Alistair Francis]

On Tue, Oct 29, 2024 at 4:01 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> I had a change of heart w.r.t this work. I still believe that the boolean properties
> are better to deal with since we don't have to deal with string parsing, and that we
> should avoid creating new string props in the future.
>
> But as far as the user API goes it doesn't matter that much. Having to do
>
> -accel kvm,riscv-aia=emul
>
> or
>
> -accel kvm,riscv-aia-emul=on
>
> is basically the same thing. Deprecate properties always creates some form of hassle
> for existing scripts and whatnot and we should avoid it.
>
> String properties aren't that great to report to APIs though, so what we can do is to
> create internal bools to track the string value and then use it for QMP.
>
>
> Long story short, I'll re-send this series with only patches 1 and 2. Thanks,

Ah, I should have read this before responding to your other patch.

Sounds good to me. Although I don't have the same dislike of string
properties as you, but I guess I'm also not using APIs :)

Alistair

>
>
> Daniel
>
>
>
> On 9/24/24 9:44 AM, Daniel Henrique Barboza wrote:
> > Hi,
> >
> > Boolean properties are easier to deal with in the protocol side (e.g.
> > QMP) since they don't require string parsing. We should always use them
> > when applicable.
> >
> > This series adds 3 new riscv-aia bool options for the KVM accel driver,
> > each one representing the possible values (emul, hwaccel and auto).
> > We're also deprecating the existing 'riscv-aia' string option.
> >
> > The idea is to use the new properties to enable AIA support in libvirt.
> >
> > Patches based on riscv-to-apply.next.
> >
> > Daniel Henrique Barboza (4):
> >    target/riscv/kvm: set 'aia_mode' to default in error path
> >    target/riscv/kvm: clarify how 'riscv-aia' default works
> >    target/riscv/kvm: add kvm-aia bools props
> >    target/riscv/kvm: deprecate riscv-aia string prop
> >
> >   docs/about/deprecated.rst  |   8 +++
> >   target/riscv/kvm/kvm-cpu.c | 100 +++++++++++++++++++++++++++++++++----
> >   2 files changed, 98 insertions(+), 10 deletions(-)
> >
>

Re: [PATCH 3/4] target/riscv/kvm: add kvm-aia bools props

[Andrew Jones]

On Wed, Oct 30, 2024 at 11:40:57AM +1000, Alistair Francis wrote:
> On Fri, Oct 11, 2024 at 9:19 PM Daniel Henrique Barboza
...
> > In case someone tries it out with multiple -accel options, this doesn't work. Only
> > the first '-accel <type>' are parsed. This happens due to a known command line
> > parsing/accel globals issue that I tried to fix in [1] and [2].
> >
> > For now, using the existing 'riscv-aia' string option:
> >
> > -accel kvm,riscv-aia=emul -accel kvm,riscv-aia=hwaccel -accel kvm,riscv-aia=auto
> >
> > This will set riscv-aia to "emul" because all other "-accel kvm" options aren't
> > being parsed. You can do silly stuff like:
> >
> > -accel kvm,riscv-aia=emul -accel kvm,riscv-aia=this_is_not_an_option
> >
> > And the guest will boot normally, setting riscv-aia to 'emul'.
> 
> Both of those are unfortunate, but I do at least feel that reading
> them it's clear that something is wrong as the user has listed `-accel
> kvm...` multiple times.
>

I wish that multiple '-accel kvm...' would work since scripts can be
written to set reasonable defaults, e.g.

 qemu-script.sh:
 qemu-system-riscv64 -cpu max -accel kvm,riscv-aia=auto ... $@

but then anything the user tacks on through $@ would override those
defaults. For example, 'qemu-script.sh -cpu max,some-ext=off' overrides
the '-cpu max' in the script, disabling an extension. However,
'qemu-script.sh -accel kvm,riscv-aia=emul' will not force KVM to use
use AIA=emul mode since the tacked on '-accel' is ignored.

Thanks,
drew

Re: [PATCH 0/4] target/riscv/kvm: add riscv-aia bool props

[Andrew Jones]

On Wed, Oct 30, 2024 at 11:44:19AM +1000, Alistair Francis wrote:
> On Tue, Oct 29, 2024 at 4:01 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > Hi,
> >
> > I had a change of heart w.r.t this work. I still believe that the boolean properties
> > are better to deal with since we don't have to deal with string parsing, and that we
> > should avoid creating new string props in the future.
> >
> > But as far as the user API goes it doesn't matter that much. Having to do
> >
> > -accel kvm,riscv-aia=emul
> >
> > or
> >
> > -accel kvm,riscv-aia-emul=on
> >
> > is basically the same thing. Deprecate properties always creates some form of hassle
> > for existing scripts and whatnot and we should avoid it.
> >
> > String properties aren't that great to report to APIs though, so what we can do is to
> > create internal bools to track the string value and then use it for QMP.
> >
> >
> > Long story short, I'll re-send this series with only patches 1 and 2. Thanks,
> 
> Ah, I should have read this before responding to your other patch.
> 
> Sounds good to me. Although I don't have the same dislike of string
> properties as you, but I guess I'm also not using APIs :)

libvirt and other upper layers which use qmp would need to learn about
each property's possible values, possibly requiring QEMU to provide
different APIs for each different property type. With only boolean
properties, all an object's properties can be queried and modified in the
same way, which also allows immediately knowing how to enable and disable
new properties which QEMU adds without the need to update the upper layers
at all.

Thanks,
drew