Re: [PATCH 2/3] system/qtest: Check ngl->name for irq_intercept_in

[Fabiano Rosas <farosas@suse.de>]

Peter Maydell <peter.maydell@linaro.org> writes:

> Currently we check the name of the NamedGPIOList when doing
> irq_intercept_out, but not for irq_intercept_in.  Even though we
> don't currently support irq_intercept_in on named GPIO lines, we
> still need to check the name, because otherwise we will install the
> observer on every set of GPIO inputs the device has, rather than only
> for the unnamed GPIO inputs.
>
> This doesn't currently affect any test cases, because of all the
> tests which use irq_intercept_in only tests/qtest-l2vic-test.c uses
> it on a device that creates named GPIO inputs.  That device has both
> unnamed GPIO inputs and a named GPIO input "reset".  The effect is
> that a change in the "reset" line state looks like a change in
> interrupt input line 0; but since the test doesn't cause "reset" to
> change this doesn't happen and there are no observable effects of the
> bug.
>
> Check the ngl->name against the specified one for both input
> and output interception. (Currently this will be equivalent to
> a check against NULL for input intercepts.) As we're now checking
> for an exact name match unconditionally, we can break out of the
> loop as soon as we've found it.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The forbidding of intercept_in for named GPIOs is done before
> the loop. I think the only reason we forbid it is that we don't
> have a mechanism for the "IRQ raise"/"IRQ lower" messages to
> say what GPIO they're for. Similarly I don't think supporting
> intercepts of multiple devices simultaneously would be too
> hard; but I'd rather wait til we have a test that actually wants
> that functionality.
> ---
>  system/qtest.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/system/qtest.c b/system/qtest.c
> index a3de815ce65..5ff3f9f27d8 100644
> --- a/system/qtest.c
> +++ b/system/qtest.c
> @@ -422,23 +422,24 @@ static void qtest_process_command(CharFrontend *chr, gchar **words)
>          }
>  
>          QLIST_FOREACH(ngl, &dev->gpios, node) {
> -            /* We don't support inbound interception of named GPIOs yet */
> +            /* NULL is valid and matchable, for "unnamed GPIO" */
> +            if (g_strcmp0(ngl->name, words[2]) != 0) {
> +                continue;
> +            }
> +
>              if (is_outbound) {
> -                /* NULL is valid and matchable, for "unnamed GPIO" */
> -                if (g_strcmp0(ngl->name, words[2]) == 0) {
> -                    int i;
> -                    qtest_init_irq_intercept(ngl->num_out);
> -                    for (i = 0; i < ngl->num_out; ++i) {
> -                        qtest_install_gpio_out_intercept(dev, ngl->name, i);
> -                    }
> -                    interception_succeeded = true;
> +                int i;
> +                qtest_init_irq_intercept(ngl->num_out);
> +                for (i = 0; i < ngl->num_out; ++i) {
> +                    qtest_install_gpio_out_intercept(dev, ngl->name, i);
>                  }
>              } else {
>                  qtest_init_irq_intercept(ngl->num_in);
>                  qemu_irq_set_observer(ngl->in, qtest_irq_handler,
>                                        ngl->num_in);
> -                interception_succeeded = true;
>              }
> +            interception_succeeded = true;
> +            break;
>          }
>  
>          if (interception_succeeded) {

Reviewed-by: Fabiano Rosas <farosas@suse.de>