Re: [PATCH] test/qtest: Add an API function to capture IRQ toggling

[Philippe Mathieu-Daudé]

On 13/11/23 18:33, Gustavo Romero wrote:
> > > > Currently the QTest API does not provide a function to allow capturing
> > > > when an IRQ line is toggled (raised then lowered). Functions like
> > > > qtest_get_irq() read the current state of the intercepted IRQ lines,
> > > > which is already low when the function is called, since the line is
> > > > toggled.
> > > >
> > > > This commit introduces a new function, qtest_get_irq_trigger_counter(),
> > > > which returns the number of times a given intercepted IRQ line was
> > > > triggered (raised), hence allowing to capture when an IRQ line was
> > > > toggled.
> > > >
> > > > Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> > > > ---
> > > >   tests/qtest/libqtest.c | 12 ++++++++++++
> > > >   tests/qtest/libqtest.h |  9 +++++++++
> > > >   2 files changed, 21 insertions(+)
> > > >
> > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > index f33a210861..21891b52f1 100644
> > > > --- a/tests/qtest/libqtest.c
> > > > +++ b/tests/qtest/libqtest.c
> > > > @@ -82,6 +82,7 @@ struct QTestState
> > > >       int expected_status;
> > > >       bool big_endian;
> > > >       bool irq_level[MAX_IRQ];
> > > > +    uint64_t irq_trigger_counter[MAX_IRQ];
> > > >       GString *rx;
> > > >       QTestTransportOps ops;
> > > >       GList *pending_events;
> > > > @@ -498,6 +499,7 @@ static QTestState *qtest_init_internal(const 
> > > > char *qemu_bin,
> > > >       s->rx = g_string_new("");
> > > >       for (i = 0; i < MAX_IRQ; i++) {
> > > >           s->irq_level[i] = false;
> > > > +        s->irq_trigger_counter[i] = 0;
> > > >       }
> > > >       /*
> > > > @@ -690,6 +692,7 @@ redo:
> > > >           if (strcmp(words[1], "raise") == 0) {
> > > >               s->irq_level[irq] = true;
> > > > +            s->irq_trigger_counter[irq]++;
> >
> > This is 'irq_raised_counter',
> >
> > > Not sure whether you can get some "raise" events in a row without 
> > > some "lower" events in between ... but just in case, I wonder whether 
> > > it would make sense to check whether it is really a rising edge, i.e.:
> > >
> > >             if (strcmp(words[1], "raise") == 0) {
> > >                 if (!s->irq_level[irq]) {
> > >                     s->irq_trigger_counter[irq]++;
> > >                 }
> > >                 s->irq_level[irq] = true;
> > >
> > > What do you think?
> >
> > This is 'irq_pulsed_counter'. 'irq_lowered_counter' could also be
> > useful (at least for completeness).
>
> I understand that the code provided by Thomas actually has the exactly same
> effect as my code. This happens because a "raise" (or "low) message is
> not sent by the "server" side unless a transition state low -> high 
> happens,
> so the check 'if (!s->irq_level[irq]) { ... }' is always true. So it's 
> still
> capturing the raising state transition (as a side note, when one intercepts
> an IRQ the current state of the IRQ line is saved -- so the old IRQ 
> state is
> always valid). Therefore, also as a consequence, like Thomas said, it's not
> possible to get a 'raise' event in a row without a 'lower' event in 
> between.
>
> Based on your comments, I gave a second thought on 'trigger' meaning. I 
> think
> 'trigger' refers to an event or action that automatically initiates a
> procedure. Since raising an IRQ line does not always mean to generate an 
> IRQ,
> since the like can be active low in a device, maybe I should avoid using
> trigger and synonymous for raising.
>
> I think since 'toggle' means to switch back and forth between two modes or
> states, yep, it seems ok to me to use it as a synonymous for 'pulse'.

One definition of "to toggle" is:
'''
switch from one effect, feature, or state to another by using a toggle.
'''

"pulsate":
'''
expand and contract with strong regular movements.
'''

Maybe 'pulse' is simpler to understand?

> Hence, I removed the word 'trigger' from the API function name and 
> elsewhere.
>
> Right, I think having a qtest_get_irq_lowered_counter() is better and also,
> when used together with qtest_get_irq_raised_counter(), it's possible for a
> test to check pulses on the IRQ lines.
>
>
> > Per Gustavo's description, he indeed wants irq_pulsed_counter (or
> > irq_toggled_counter'.
>
> That's a good point. So far I was testing just the high -> low transition,
> but in fact the most correct way to test the device is also check for
> a high -> low transition (so, yeah, indeed test a pulse).
>
> In the device I have:
>
> [...]
>      /*
>       * Toggle device's output line, which is connected to interrupt 
> controller,
>       * generating an interrupt request to the CPU.
>       */
>      qemu_set_irq(s->irq, true);
>      qemu_set_irq(s->irq, false);

This is qemu_irq_pulse() from include/hw/irq.h.

> }
>
> Thus having both qtest_get_irq_{lowered,raised}_counter() allows capturing
> an IRQ toggle, for instance, as the following, where exactly 1 pulse is 
> tested:
>
>      uint64_t num_raises;
>      uint64_t num_lows;
>
>      while (1) {
>          if ((num_raises = qtest_get_irq_raised_counter(qts, 0))) {
>              num_lows = qtest_get_irq_lowered_counter(qts, 0);
>              if (num_raises == num_lows && num_lows == 1) {
>                  printf("Detected correct number of pulses.\n");
>                  return 0;
>              } else {
>                  printf("Detected incorrect number of pulses.\n");
>                  return 1;
>              }
>          }
>      }
>
> > > >           } else {
> > > >               s->irq_level[irq] = false;
> > > >           }
> > >
> > > Anyway:
> > > Acked-by: Thomas Huth <thuth@redhat.com>
> I'm sending a v2 with qtest_get_irq_lowered_counter().
>
> Thanks!
>
>
> Cheers,
> Gustavo