On Fri, Oct 11, 2024 at 9:19 PM Daniel Henrique Barboza
<dbarb...@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
> > <dbarb...@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 <dbarb...@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
