| Message ID | 20240701133038.1489043-3-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | system/vl.c: parse all '-accel' opts | expand |
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>
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
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.
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
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(+)