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

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

Hi Phil,

Apologies, I missed this and I just found it when preparing
now the v3 for ivshmem-flat.


On 12/13/23 6:15 AM, Philippe Mathieu-Daudé wrote:
> 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?

I prefer 'toggle' (as you suggested initially). However, for v2,
I didn't add an API function to detect a pulse/trigger/toggle.
Instead, for v2, there are only qtest_get_irq_raised_counter() and
qtest_get_irq_lowered_counter(), as you suggested. The user can
then use these functions to create such a detector:

https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg03176.html

... as we do in the ivshmem-flat qtest (test_ivshmem_flat_irq_positive_pulse).

If you're ok with v2, could you please bless it? :)


> 
>> 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.

oh! cool, totally missed that. I'm using it for the ivshmem-flat v3 series!


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