| Message ID | 20240924124407.766495-4-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv/kvm: add riscv-aia bool props | expand |
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
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,
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(+)