| Message ID | 20240701133038.1489043-3-dbarboza@ventanamicro.com |
|---|---|
| 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.
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
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(+)