| Message ID | 20231112013801.293970-1-gustavo.romero@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | test/qtest: Add an API function to capture IRQ toggling | expand |
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>
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> >
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
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
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 >
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.
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(+)