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

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(+)

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
>
>

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
>>
>>

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

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