* [PATCH] test/qtest: Add an API function to capture IRQ toggling
@ 2023-11-12 1:38 Gustavo Romero
2023-11-13 6:59 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo Romero @ 2023-11-12 1:38 UTC (permalink / raw)
To: qemu-devel, thuth; +Cc: lvivier, pbonzini, philmd, gustavo.romero
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]++;
} else {
s->irq_level[irq] = false;
}
@@ -980,6 +983,14 @@ bool qtest_get_irq(QTestState *s, int num)
return s->irq_level[num];
}
+uint64_t qtest_get_irq_trigger_counter(QTestState *s, int irq_num)
+{
+ /* dummy operation in order to make sure irq is up to date */
+ qtest_inb(s, 0);
+
+ return s->irq_trigger_counter[irq_num];
+}
+
void qtest_module_load(QTestState *s, const char *prefix, const char *libname)
{
qtest_sendf(s, "module_load %s %s\n", prefix, libname);
@@ -1799,6 +1810,7 @@ QTestState *qtest_inproc_init(QTestState **s, bool log, const char* arch,
qts->wstatus = 0;
for (int i = 0; i < MAX_IRQ; i++) {
qts->irq_level[i] = false;
+ qts->irq_trigger_counter[i] = 0;
}
qtest_client_set_rx_handler(qts, qtest_client_inproc_recv_line);
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 6e3d3525bf..d1c525aea0 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -364,6 +364,15 @@ void qtest_module_load(QTestState *s, const char *prefix, const char *libname);
*/
bool qtest_get_irq(QTestState *s, int num);
+/**
+ * qtest_get_irq_counter:
+ * @s: #QTestState instance to operate on.
+ * @num: Interrupt to observe.
+ *
+ * Returns: The number of times IRQ @num got triggered (raised).
+ */
+uint64_t qtest_get_irq_trigger_counter(QTestState *s, int num);
+
/**
* qtest_irq_intercept_in:
* @s: #QTestState instance to operate on.
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] test/qtest: Add an API function to capture IRQ toggling
2023-11-12 1:38 [PATCH] test/qtest: Add an API function to capture IRQ toggling Gustavo Romero
@ 2023-11-13 6:59 ` Thomas Huth
2023-11-13 10:14 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2023-11-13 6:59 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel; +Cc: lvivier, pbonzini, philmd
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]++;
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?
> } else {
> s->irq_level[irq] = false;
> }
Anyway:
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] test/qtest: Add an API function to capture IRQ toggling
2023-11-13 6:59 ` Thomas Huth
@ 2023-11-13 10:14 ` Philippe Mathieu-Daudé
2023-11-13 17:33 ` Gustavo Romero
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-11-13 10:14 UTC (permalink / raw)
To: Thomas Huth, Gustavo Romero, qemu-devel; +Cc: lvivier, pbonzini
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).
Per Gustavo's description, he indeed wants irq_pulsed_counter (or
irq_toggled_counter'.
>
>> } else {
>> s->irq_level[irq] = false;
>> }
>
> Anyway:
> Acked-by: Thomas Huth <thuth@redhat.com>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] test/qtest: Add an API function to capture IRQ toggling
2023-11-13 10:14 ` Philippe Mathieu-Daudé
@ 2023-11-13 17:33 ` Gustavo Romero
2023-12-13 9:15 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo Romero @ 2023-11-13 17:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Thomas Huth, qemu-devel; +Cc: lvivier, pbonzini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] test/qtest: Add an API function to capture IRQ toggling
2023-11-13 17:33 ` Gustavo Romero
@ 2023-12-13 9:15 ` Philippe Mathieu-Daudé
2024-02-21 15:47 ` Gustavo Romero
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-12-13 9:15 UTC (permalink / raw)
To: Gustavo Romero, Thomas Huth, qemu-devel; +Cc: lvivier, pbonzini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] test/qtest: Add an API function to capture IRQ toggling
2023-12-13 9:15 ` Philippe Mathieu-Daudé
@ 2024-02-21 15:47 ` Gustavo Romero
0 siblings, 0 replies; 6+ messages in thread
From: Gustavo Romero @ 2024-02-21 15:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Thomas Huth, qemu-devel; +Cc: lvivier, pbonzini
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-21 15:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-12 1:38 [PATCH] test/qtest: Add an API function to capture IRQ toggling Gustavo Romero
2023-11-13 6:59 ` Thomas Huth
2023-11-13 10:14 ` Philippe Mathieu-Daudé
2023-11-13 17:33 ` Gustavo Romero
2023-12-13 9:15 ` Philippe Mathieu-Daudé
2024-02-21 15:47 ` Gustavo Romero
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).