[PATCH 0/2] system/vl.c: parse all '-accel' opts

[PATCH 0/2] system/vl.c: parse all '-accel' opts

[Daniel Henrique Barboza]

Hi,

In the current handling of  '-accel' only the first instance is parsed.
All other instances (aside from a 'helper' command that triggers the
help text and exits) is ignored.

This also means that we can mix different accelerators in the same
command line. In fact we can do whatever we want as long as the first
instance of '-accel' is valid. E.g. this command line will boot a x86_64
KVM guest without problems:

qemu-system-x86_64 -accel kvm -accel tcg -accel not_an_accel (...)

And this will boot a KVM guest with kernel-irqchip=off because it will
ignore the second '-accel' that sets its back to 'on':

qemu-system-x86_64 -accel kvm,kernel-irqchip=off \
    -accel kvm,kernel-irqchip=on (...)

My initial intention was to fix a problem we're having with libvirt and
RISC-V where we can't set 'riscv-aia' by appending '-accel kvm,riscv-aia=val'
via <qemu:cmdline> in the domain XML. libvirt will add a leading
'-accel kvm' in the regular command line and ignore the second. But to
fix that (patch 2) we must first guarantee that we're not mixing different
accelerators.

Both patches can be squashed in a single patch if preferrable. I'm
sending in separate because I'm not fully confident in patch 1.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>

Daniel Henrique Barboza (2):
  system/vl.c: do not allow mixed -accel opts
  system/vl.c: parse all -accel options

 system/vl.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

-- 
2.45.2

[PATCH 1/2] system/vl.c: do not allow mixed -accel opts

[Daniel Henrique Barboza]

We're allowing multiple -accel options to be used with different
accelerators, even though we don't have any machine that supports mixed
acceleration.

In fact, it will only parse the first occurence of -accel and, aside
from a help option (e.g. -accel help) that will always cause the process
to print the help text, it will accept every other accel option
regardless of being correct or not. E.g. this:

qemu-system-x86_64 -accel kvm -accel tcg -accel IamNotAnAccel (...)

will happily boot a x86_64 KVM guest.

Do not allow for different accelerators to be used when multiple
instances of -accel are present.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 system/vl.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/system/vl.c b/system/vl.c
index bdd2f6ecf6..32602e68b7 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -3318,10 +3318,22 @@ void qemu_init(int argc, char **argv)
                     }
                     break;
                 }
-            case QEMU_OPTION_accel:
+            case QEMU_OPTION_accel: {
+                QemuOptsList *list = qemu_find_opts("accel");
+                const char *prev_accel = qemu_opt_get(QTAILQ_LAST(&list->head),
+                                                      "accel");
+
                 accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
                                                      optarg, true);
                 optarg = qemu_opt_get(accel_opts, "accel");
+
+                if (prev_accel && optarg && strcmp(prev_accel, optarg)) {
+                    printf("Unable to mix accelerators with multiple "
+                           "-accel options (have: '%s' and '%s')\n",
+                           prev_accel, optarg);
+                    exit(1);
+                }
+
                 if (!optarg || is_help_option(optarg)) {
                     printf("Accelerators supported in QEMU binary:\n");
                     GSList *el, *accel_list = object_class_get_list(TYPE_ACCEL,
@@ -3343,6 +3355,7 @@ void qemu_init(int argc, char **argv)
                     exit(0);
                 }
                 break;
+            }
             case QEMU_OPTION_usb:
                 qdict_put_str(machine_opts_dict, "usb", "on");
                 break;
-- 
2.45.2

Re: [PATCH 1/2] system/vl.c: do not allow mixed -accel opts

[Richard Henderson]

On 7/1/24 06:30, Daniel Henrique Barboza wrote:
> We're allowing multiple -accel options to be used with different
> accelerators, even though we don't have any machine that supports mixed
> acceleration.
> 
> In fact, it will only parse the first occurence of -accel and, aside
> from a help option (e.g. -accel help) that will always cause the process
> to print the help text, it will accept every other accel option
> regardless of being correct or not. E.g. this:
> 
> qemu-system-x86_64 -accel kvm -accel tcg -accel IamNotAnAccel (...)
> 
> will happily boot a x86_64 KVM guest.
> 
> Do not allow for different accelerators to be used when multiple
> instances of -accel are present.
> 
> Cc: Paolo Bonzini<pbonzini@redhat.com>
> Cc: Thomas Huth<thuth@redhat.com>
> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
> ---
>   system/vl.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)

We use '-accel kvm -accel tcg' to allow kvm to fail (e.g. no /dev/kvm permission) and 
proceed with tcg.

This patch will cause testsuite failures.


r~

Re: [PATCH 1/2] system/vl.c: do not allow mixed -accel opts

[Daniel Henrique Barboza]

On 7/1/24 12:23 PM, Richard Henderson wrote:
> On 7/1/24 06:30, Daniel Henrique Barboza wrote:
>> We're allowing multiple -accel options to be used with different
>> accelerators, even though we don't have any machine that supports mixed
>> acceleration.
>>
>> In fact, it will only parse the first occurence of -accel and, aside
>> from a help option (e.g. -accel help) that will always cause the process
>> to print the help text, it will accept every other accel option
>> regardless of being correct or not. E.g. this:
>>
>> qemu-system-x86_64 -accel kvm -accel tcg -accel IamNotAnAccel (...)
>>
>> will happily boot a x86_64 KVM guest.
>>
>> Do not allow for different accelerators to be used when multiple
>> instances of -accel are present.
>>
>> Cc: Paolo Bonzini<pbonzini@redhat.com>
>> Cc: Thomas Huth<thuth@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
>> ---
>>   system/vl.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> We use '-accel kvm -accel tcg' to allow kvm to fail (e.g. no /dev/kvm permission) and proceed with tcg.
> 
> This patch will cause testsuite failures.

For the issue I want to fix patch 2 alone is enough. I'll re-send.


Thanks,

Daniel


> 
> 
> r~

Re: [PATCH 1/2] system/vl.c: do not allow mixed -accel opts

[Paolo Bonzini]

On Mon, Jul 1, 2024 at 5:53 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
> > We use '-accel kvm -accel tcg' to allow kvm to fail (e.g. no /dev/kvm permission) and proceed with tcg.
> >
> > This patch will cause testsuite failures.
>
> For the issue I want to fix patch 2 alone is enough. I'll re-send.

It doesn't; it effectively changes '-accel kvm -accel tcg' to just
'-accel tcg'.  This is why you didn't see any failures, I think.

Paolo

[PATCH 2/2] system/vl.c: parse all -accel options

[Daniel Henrique Barboza]

We're not honouring KVM options that are provided by any -accel option
aside from the first. In this example:

qemu-system-riscv64 -accel kvm,riscv-aia=emul (...) \ -accel
kvm,riscv-aia=hwaccel

'riscv-aia' will be set to 'emul', ignoring the last occurrence of the
option that set 'riscv-aia' to 'hwaccel'.

The previous change guarantees that we'll not have mixed accelerators in
the command line, and now it's safe to activate 'merge_lists' for
'qemu_accel_opts'. This will merge all accel options in the same list,
allowing the 'qemu_opt_foreach()' callback in do_configure_accelerator()
to apply each one of them in the Accel class.

Reported-by: Andrew Jones <ajones@ventanamicro.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 system/vl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/system/vl.c b/system/vl.c
index 32602e68b7..5ed9a9229f 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -259,6 +259,7 @@ static QemuOptsList qemu_accel_opts = {
     .name = "accel",
     .implied_opt_name = "accel",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
+    .merge_lists = true,
     .desc = {
         /*
          * no elements => accept any
-- 
2.45.2

Re: [PATCH 2/2] system/vl.c: parse all -accel options

[Philippe Mathieu-Daudé]

On 1/7/24 15:30, Daniel Henrique Barboza wrote:
> We're not honouring KVM options that are provided by any -accel option
> aside from the first. In this example:
> 
> qemu-system-riscv64 -accel kvm,riscv-aia=emul (...) \ -accel
> kvm,riscv-aia=hwaccel
> 
> 'riscv-aia' will be set to 'emul', ignoring the last occurrence of the
> option that set 'riscv-aia' to 'hwaccel'.
> 
> The previous change guarantees that we'll not have mixed accelerators in
> the command line, and now it's safe to activate 'merge_lists' for
> 'qemu_accel_opts'. This will merge all accel options in the same list,
> allowing the 'qemu_opt_foreach()' callback in do_configure_accelerator()
> to apply each one of them in the Accel class.
> 
> Reported-by: Andrew Jones <ajones@ventanamicro.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   system/vl.c | 1 +
>   1 file changed, 1 insertion(+)

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

Re: [PATCH 2/2] system/vl.c: parse all -accel options

[Paolo Bonzini]

On Mon, Jul 1, 2024 at 4:34 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

In principle, a Reviewed-by tag is just stating that you don't know of
any issues that would prevent the patch being included. However, as a
frequent participant to the project, your Reviewed-by tag carries some
weight and, to some extent, it is also a statement that you understand
the area being modified.  A Reviewed-by from an experienced
contributor may even imply that you could take the patch in one of
your pull requests. (*) That makes it even more important to
understand the area.

I would expect that anyone with an understanding of command line
parsing would know 1) what -accel kvm -accel tcg does, and 2) what
.merge_lists does; and this would be enough to flag an issue
preventing the patch from being included.

To be clear, I don't expect reviews to be perfect. But in this case
I'm speaking up because the patch is literally a one line declarative
change, and the only way to say "I've reviewed it" is by understanding
the deeper effects of that line.

Also, I think it's fair that the submitter didn't spot the problem;
it's okay to send out broken patches, that's part of the learning
experience. :)

Paolo

(*) as opposed to Acked-by, where your review probably has been more
conceptual than technical, and that you don't really want to take the
patch in a pull request.


Paolo

Re: [PATCH 2/2] system/vl.c: parse all -accel options

[Philippe Mathieu-Daudé]

On 1/7/24 18:43, Paolo Bonzini wrote:
> On Mon, Jul 1, 2024 at 4:34 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> In principle, a Reviewed-by tag is just stating that you don't know of
> any issues that would prevent the patch being included. However, as a
> frequent participant to the project, your Reviewed-by tag carries some
> weight and, to some extent, it is also a statement that you understand
> the area being modified.  A Reviewed-by from an experienced
> contributor may even imply that you could take the patch in one of
> your pull requests. (*) That makes it even more important to
> understand the area.
> 
> I would expect that anyone with an understanding of command line
> parsing would know 1) what -accel kvm -accel tcg does, and 2) what
> .merge_lists does; and this would be enough to flag an issue
> preventing the patch from being included.

I admit I haven't reviewed what .merge_lists does but went to look
at its use cases (I see 'git grep -wW merge_lists' in my history)
and mis-read:

util/keyval.c-370- * - lists are concatenated
util/keyval.c-371- *
util/keyval.c-372- * - dictionaries are merged recursively
util/keyval.c-373- *
util/keyval.c-374- * - for scalar values, @merged wins
util/keyval.c-375- *
util/keyval.c-376- * In case an error is reported, @dest may already 
have been modified.
util/keyval.c-377- *
util/keyval.c-378- * This function can be used to implement semantics 
analogous to QemuOpts's
util/keyval.c:379: * .merge_lists = true case, or to implement -set for 
options backed by QDicts.

which made me confident enough with the patch description:

  > and now it's safe to activate 'merge_lists' for 'qemu_accel_opts'.
  > This will merge all accel options in the same list

OTOH I wasn't clear about the first patch and knew this one depends
on it, so if the first is wrong, this one is automatically discarded.

> To be clear, I don't expect reviews to be perfect. But in this case
> I'm speaking up because the patch is literally a one line declarative
> change, and the only way to say "I've reviewed it" is by understanding
> the deeper effects of that line.

Don't blame the review but the reviewer :) Reviews aim to be
perfect, unfortunately the human beings sending them aren't
(at least I am not, as I just proved).

Thankfully maintainers are gatekeepers on their areas and can
catch issues like that. I duly noted "my Reviewed-by tag carries
some weigh" and could confuse other maintainers, and will think
it twice before posting it on topics I'm unsure. Thanks for
taking the time to warn me.

Regards,

Phil.

> Also, I think it's fair that the submitter didn't spot the problem;
> it's okay to send out broken patches, that's part of the learning
> experience. :)
> 
> Paolo
> 
> (*) as opposed to Acked-by, where your review probably has been more
> conceptual than technical, and that you don't really want to take the
> patch in a pull request.

Re: [PATCH 2/2] system/vl.c: parse all -accel options

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On Mon, Jul 1, 2024 at 4:34 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> In principle, a Reviewed-by tag is just stating that you don't know of
> any issues that would prevent the patch being included. However, as a
> frequent participant to the project, your Reviewed-by tag carries some
> weight and, to some extent, it is also a statement that you understand
> the area being modified.  A Reviewed-by from an experienced
> contributor may even imply that you could take the patch in one of
> your pull requests. (*) That makes it even more important to
> understand the area.

I think you are attaching a little too much weight to a r-b tag here as
no one was suggesting the patch go in via a different tree. Ultimately
the maintainer can always NACK a reviewed patch. 

> I would expect that anyone with an understanding of command line
> parsing would know 1) what -accel kvm -accel tcg does, and 2) what
> .merge_lists does; and this would be enough to flag an issue
> preventing the patch from being included.

Maybe more useful would be re-wording the comment:

  /* Merge multiple uses of option into a single list? */

to be explicit about its behaviour. 

> To be clear, I don't expect reviews to be perfect. But in this case
> I'm speaking up because the patch is literally a one line declarative
> change, and the only way to say "I've reviewed it" is by understanding
> the deeper effects of that line.

I think that's a fairly subjective requirement for something that
generally we can always use more of. I encourage people to review all
around the code base to get familiar with new sub-systems. I don't think
we should be dissuading people from exploring outside their silos. That
simple one liners can trip people up says more about the code than the
reviewer.

I sympathise with Philippe here who's current brief takes him around our
large and interconnected code base more than most.

>
> Also, I think it's fair that the submitter didn't spot the problem;
> it's okay to send out broken patches, that's part of the learning
> experience. :)
>
> Paolo
>
> (*) as opposed to Acked-by, where your review probably has been more
> conceptual than technical, and that you don't really want to take the
> patch in a pull request.
>
>
> Paolo

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/2] system/vl.c: parse all '-accel' opts

[Paolo Bonzini]

On Mon, Jul 1, 2024 at 3:30 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
> My initial intention was to fix a problem we're having with libvirt and
> RISC-V where we can't set 'riscv-aia' by appending '-accel kvm,riscv-aia=val'
> via <qemu:cmdline> in the domain XML. libvirt will add a leading
> '-accel kvm' in the regular command line and ignore the second. But to
> fix that (patch 2) we must first guarantee that we're not mixing different
> accelerators.

I think you can use -global, if you tweak qdev_prop_check_globals() to
also allow descendents of TYPE_ACCEL.

For example:

   ./qemu-system-x86_64 -accel kvm -global kvm-accel.kernel-irqchip=off

Paolo

Re: [PATCH 0/2] system/vl.c: parse all '-accel' opts

[Daniel Henrique Barboza]

On 7/1/24 1:31 PM, Paolo Bonzini wrote:
> On Mon, Jul 1, 2024 at 3:30 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>> My initial intention was to fix a problem we're having with libvirt and
>> RISC-V where we can't set 'riscv-aia' by appending '-accel kvm,riscv-aia=val'
>> via <qemu:cmdline> in the domain XML. libvirt will add a leading
>> '-accel kvm' in the regular command line and ignore the second. But to
>> fix that (patch 2) we must first guarantee that we're not mixing different
>> accelerators.
> 
> I think you can use -global, if you tweak qdev_prop_check_globals() to
> also allow descendents of TYPE_ACCEL.
> 
> For example:
> 
>     ./qemu-system-x86_64 -accel kvm -global kvm-accel.kernel-irqchip=off

I'll try it out! Thanks,


Daniel


> 
> Paolo
>