| Message ID | 20241213002602.4181289-1-nabihestefan@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest/sse-timer-test: Add watchdog reset to sse-timer test | expand |
On 13/12/2024 01.26, Nabih Estefan wrote: > Recent CDMSK Watchdog changes (eff9dc5660fad3a610171c56a5ec3fada245e519) > updated the CDMSK APB Watchdog to not free run out of reset. That led to > this test failing since it never triggers the watchdog to start running. > No watchdog running means that the timer and counter in the test cannot > start, leading to failures in the assert statements throughout the test. > Adding a reset and enable of the watchdog to the reset function solves > this problem by enabling the watchdog and thus letting the timer and > counter run as expected > > Also renaming the reset_counter_and_timer function since it now also > affects the watchdog. > > To reproduce the failure at HEAD: > ./configure --target-list=arm-softmmu > make -j check-report-qtest-arm.junit.xml > > Signed-off-by: Nabih Estefan <nabihestefan@google.com> I think this fixes the problem that I was seeing on Travis-CI: Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2702 Tested-by: Thomas Huth <thuth@redhat.com> > > diff --git a/scripts/meson-buildoptions.sh.tmp b/scripts/meson-buildoptions.sh.tmp > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qtest/sse-timer-test.c b/tests/qtest/sse-timer-test.c > index fd5635d4c9..d7a53ac23a 100644 > --- a/tests/qtest/sse-timer-test.c > +++ b/tests/qtest/sse-timer-test.c > @@ -29,6 +29,13 @@ > /* Base of the System Counter control frame */ > #define COUNTER_BASE 0x58100000 > > +/* Base of the MSSDK APB Watchdog Device */ > +#define WDOG_BASE 0x4802e000 > + > +/* CMSDK Watchdog offsets */ > +#define WDOGLOAD 0 > +#define WDOGCONTROL 8 > + > /* SSE counter register offsets in the control frame */ > #define CNTCR 0 > #define CNTSR 0x4 > @@ -63,24 +70,26 @@ static void clock_step_ticks(uint64_t ticks) > clock_step(FOUR_TICKS * (ticks >> 2)); > } > > -static void reset_counter_and_timer(void) > +static void reset_watchdog_counter_and_timer(void) > { > /* > - * Reset the system counter and the timer between tests. This > + * Reset the system watchdog, counter and the timer between tests. This > * isn't a full reset, but it's sufficient for what the tests check. > */ > + writel(WDOG_BASE + WDOGCONTROL, 0); > writel(COUNTER_BASE + CNTCR, 0); > writel(TIMER_BASE + CNTP_CTL, 0); > writel(TIMER_BASE + CNTP_AIVAL_CTL, 0); > writel(COUNTER_BASE + CNTCV_LO, 0); > writel(COUNTER_BASE + CNTCV_HI, 0); > + writel(WDOG_BASE + WDOGCONTROL, 1); > } > > static void test_counter(void) > { > /* Basic counter functionality test */ > > - reset_counter_and_timer(); > + reset_watchdog_counter_and_timer(); > /* The counter should start disabled: check that it doesn't move */ > clock_step_ticks(100); > g_assert_cmpuint(readl(COUNTER_BASE + CNTCV_LO), ==, 0); > @@ -103,7 +112,7 @@ static void test_timer(void) > { > /* Basic timer functionality test */ > > - reset_counter_and_timer(); > + reset_watchdog_counter_and_timer(); > /* > * The timer is behind a Peripheral Protection Controller, and > * qtest accesses are always non-secure (no memory attributes), > @@ -195,7 +204,7 @@ static void test_timer_scale_change(void) > * Test that the timer responds correctly to counter > * scaling changes while it has an active timer. > */ > - reset_counter_and_timer(); > + reset_watchdog_counter_and_timer(); > /* Give ourselves access to the timer, and enable the counter and timer */ > writel(PERIPHNSPPC0, 1); > writel(COUNTER_BASE + CNTCR, 1);
Thomas Huth <thuth@redhat.com> writes: > On 13/12/2024 01.26, Nabih Estefan wrote: >> Recent CDMSK Watchdog changes (eff9dc5660fad3a610171c56a5ec3fada245e519) >> updated the CDMSK APB Watchdog to not free run out of reset. That led to >> this test failing since it never triggers the watchdog to start running. >> No watchdog running means that the timer and counter in the test cannot >> start, leading to failures in the assert statements throughout the test. >> Adding a reset and enable of the watchdog to the reset function solves >> this problem by enabling the watchdog and thus letting the timer and >> counter run as expected >> >> Also renaming the reset_counter_and_timer function since it now also >> affects the watchdog. >> >> To reproduce the failure at HEAD: >> ./configure --target-list=arm-softmmu >> make -j check-report-qtest-arm.junit.xml >> >> Signed-off-by: Nabih Estefan <nabihestefan@google.com> I don't see any failure with the sse-timer-test on master (83aaec1d5a). Is it supposed to be intermittent? Nabih, can you confirm whether this is the same issue Thomas has seen? > I think this fixes the problem that I was seeing on Travis-CI: > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2702 I did see this^ issue, however, with Philippe's series [1] (and this patch doesn't fix it). Here's a CI job with both series applied: https://gitlab.com/farosas/qemu/-/jobs/8638126011 1- https://lore.kernel.org/r/20241211233727.98923-2-philmd@linaro.org
From what I can tell this is the same issue Thomas was looking at yes. I saw the failure on the master branch at the v9.2.0 tag (ae35f033) and just re-tested it against (83aaec1d) and still see it. I haven't seen it be an intermittent failure, it has failed 100% of the time that I have tested it when targeting arm-softmmu. However, you are right that with Phillippe's series on top of my change the bug is somehow re-introduced. Not sure what the protocol is here since neither patch has landed
Nabih Estefan <nabihestefan@google.com> writes: > From what I can tell this is the same issue Thomas was looking at yes. > > I saw the failure on the master branch at the v9.2.0 tag (ae35f033) and just > re-tested it against (83aaec1d) and still see it. I haven't seen it be an > intermittent failure, it has failed 100% of the time that I have tested it when > targeting arm-softmmu. Could you please post the output of: QTEST_LOG=1 QTEST_TRACE="sse_*" QTEST_QEMU_BINARY=./qemu-system-arm ./tests/qtest/sse-timer-test Also your compiler version. > > However, you are right that with Phillippe's series on top of my change the > bug is somehow re-introduced. Not sure what the protocol is here since > neither patch has landed I would like to figure out why this reproduces 100% for you (and some CI job) and 0% for me and others. Once this is clear we can merge this patch. As for the other series let's leave the issues there to be discussed in its own thread.
Nabih Estefan <nabihestefan@google.com> writes: > Recent CDMSK Watchdog changes (eff9dc5660fad3a610171c56a5ec3fada245e519) > updated the CDMSK APB Watchdog to not free run out of reset. That led to > this test failing since it never triggers the watchdog to start running. > No watchdog running means that the timer and counter in the test cannot > start, leading to failures in the assert statements throughout the test. > Adding a reset and enable of the watchdog to the reset function solves > this problem by enabling the watchdog and thus letting the timer and > counter run as expected > > Also renaming the reset_counter_and_timer function since it now also > affects the watchdog. > > To reproduce the failure at HEAD: > ./configure --target-list=arm-softmmu > make -j check-report-qtest-arm.junit.xml > > Signed-off-by: Nabih Estefan <nabihestefan@google.com> > --- > scripts/meson-buildoptions.sh.tmp | 0 > tests/qtest/sse-timer-test.c | 19 ++++++++++++++----- > 2 files changed, 14 insertions(+), 5 deletions(-) > create mode 100644 scripts/meson-buildoptions.sh.tmp > > diff --git a/scripts/meson-buildoptions.sh.tmp b/scripts/meson-buildoptions.sh.tmp > new file mode 100644 > index 0000000000..e69de29bb2 By the way, there's an extra file being created here that we don't want. > diff --git a/tests/qtest/sse-timer-test.c b/tests/qtest/sse-timer-test.c > index fd5635d4c9..d7a53ac23a 100644 > --- a/tests/qtest/sse-timer-test.c > +++ b/tests/qtest/sse-timer-test.c > @@ -29,6 +29,13 @@ > /* Base of the System Counter control frame */ > #define COUNTER_BASE 0x58100000 > > +/* Base of the MSSDK APB Watchdog Device */ > +#define WDOG_BASE 0x4802e000 > + > +/* CMSDK Watchdog offsets */ > +#define WDOGLOAD 0 > +#define WDOGCONTROL 8 > + > /* SSE counter register offsets in the control frame */ > #define CNTCR 0 > #define CNTSR 0x4 > @@ -63,24 +70,26 @@ static void clock_step_ticks(uint64_t ticks) > clock_step(FOUR_TICKS * (ticks >> 2)); > } > > -static void reset_counter_and_timer(void) > +static void reset_watchdog_counter_and_timer(void) > { > /* > - * Reset the system counter and the timer between tests. This > + * Reset the system watchdog, counter and the timer between tests. This > * isn't a full reset, but it's sufficient for what the tests check. > */ > + writel(WDOG_BASE + WDOGCONTROL, 0); > writel(COUNTER_BASE + CNTCR, 0); > writel(TIMER_BASE + CNTP_CTL, 0); > writel(TIMER_BASE + CNTP_AIVAL_CTL, 0); > writel(COUNTER_BASE + CNTCV_LO, 0); > writel(COUNTER_BASE + CNTCV_HI, 0); > + writel(WDOG_BASE + WDOGCONTROL, 1); > } > > static void test_counter(void) > { > /* Basic counter functionality test */ > > - reset_counter_and_timer(); > + reset_watchdog_counter_and_timer(); > /* The counter should start disabled: check that it doesn't move */ > clock_step_ticks(100); > g_assert_cmpuint(readl(COUNTER_BASE + CNTCV_LO), ==, 0); > @@ -103,7 +112,7 @@ static void test_timer(void) > { > /* Basic timer functionality test */ > > - reset_counter_and_timer(); > + reset_watchdog_counter_and_timer(); > /* > * The timer is behind a Peripheral Protection Controller, and > * qtest accesses are always non-secure (no memory attributes), > @@ -195,7 +204,7 @@ static void test_timer_scale_change(void) > * Test that the timer responds correctly to counter > * scaling changes while it has an active timer. > */ > - reset_counter_and_timer(); > + reset_watchdog_counter_and_timer(); > /* Give ourselves access to the timer, and enable the counter and timer */ > writel(PERIPHNSPPC0, 1); > writel(COUNTER_BASE + CNTCR, 1);
diff --git a/scripts/meson-buildoptions.sh.tmp b/scripts/meson-buildoptions.sh.tmp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qtest/sse-timer-test.c b/tests/qtest/sse-timer-test.c index fd5635d4c9..d7a53ac23a 100644 --- a/tests/qtest/sse-timer-test.c +++ b/tests/qtest/sse-timer-test.c @@ -29,6 +29,13 @@ /* Base of the System Counter control frame */ #define COUNTER_BASE 0x58100000 +/* Base of the MSSDK APB Watchdog Device */ +#define WDOG_BASE 0x4802e000 + +/* CMSDK Watchdog offsets */ +#define WDOGLOAD 0 +#define WDOGCONTROL 8 + /* SSE counter register offsets in the control frame */ #define CNTCR 0 #define CNTSR 0x4 @@ -63,24 +70,26 @@ static void clock_step_ticks(uint64_t ticks) clock_step(FOUR_TICKS * (ticks >> 2)); } -static void reset_counter_and_timer(void) +static void reset_watchdog_counter_and_timer(void) { /* - * Reset the system counter and the timer between tests. This + * Reset the system watchdog, counter and the timer between tests. This * isn't a full reset, but it's sufficient for what the tests check. */ + writel(WDOG_BASE + WDOGCONTROL, 0); writel(COUNTER_BASE + CNTCR, 0); writel(TIMER_BASE + CNTP_CTL, 0); writel(TIMER_BASE + CNTP_AIVAL_CTL, 0); writel(COUNTER_BASE + CNTCV_LO, 0); writel(COUNTER_BASE + CNTCV_HI, 0); + writel(WDOG_BASE + WDOGCONTROL, 1); } static void test_counter(void) { /* Basic counter functionality test */ - reset_counter_and_timer(); + reset_watchdog_counter_and_timer(); /* The counter should start disabled: check that it doesn't move */ clock_step_ticks(100); g_assert_cmpuint(readl(COUNTER_BASE + CNTCV_LO), ==, 0); @@ -103,7 +112,7 @@ static void test_timer(void) { /* Basic timer functionality test */ - reset_counter_and_timer(); + reset_watchdog_counter_and_timer(); /* * The timer is behind a Peripheral Protection Controller, and * qtest accesses are always non-secure (no memory attributes), @@ -195,7 +204,7 @@ static void test_timer_scale_change(void) * Test that the timer responds correctly to counter * scaling changes while it has an active timer. */ - reset_counter_and_timer(); + reset_watchdog_counter_and_timer(); /* Give ourselves access to the timer, and enable the counter and timer */ writel(PERIPHNSPPC0, 1); writel(COUNTER_BASE + CNTCR, 1);
Recent CDMSK Watchdog changes (eff9dc5660fad3a610171c56a5ec3fada245e519) updated the CDMSK APB Watchdog to not free run out of reset. That led to this test failing since it never triggers the watchdog to start running. No watchdog running means that the timer and counter in the test cannot start, leading to failures in the assert statements throughout the test. Adding a reset and enable of the watchdog to the reset function solves this problem by enabling the watchdog and thus letting the timer and counter run as expected Also renaming the reset_counter_and_timer function since it now also affects the watchdog. To reproduce the failure at HEAD: ./configure --target-list=arm-softmmu make -j check-report-qtest-arm.junit.xml Signed-off-by: Nabih Estefan <nabihestefan@google.com> --- scripts/meson-buildoptions.sh.tmp | 0 tests/qtest/sse-timer-test.c | 19 ++++++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) create mode 100644 scripts/meson-buildoptions.sh.tmp