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

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Thomas and Phil,


On 11/13/23 7:14 AM, Philippe Mathieu-Daudé wrote:
> On 13/11/23 07:59, Thomas Huth wrote:
>> On 12/11/2023 02.38, 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'.

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);
}

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