[PATCH 0/3] tests/qtest: npcm7xx test fixes

[PATCH 0/3] tests/qtest: npcm7xx test fixes

[Havard Skinnemoen via]

This series contains a fix for the randomness calculation in npcm7xx_rng-test.
It also makes test failures fatal. The last patch would have dumped the random
data to stderr if the randomness test fails, except now that failures are
fatal, it never actually gets a chance to do that.

It may not make sense to apply all three, but I'd definitely take (1), and I'll
leave it up to you whether to apply (2), (3) or both.

Havard Skinnemoen (3):
  tests/qtest/npcm7xx_rng-test: count runs properly
  tests/qtest/npcm7xx: Don't call g_test_set_nonfatal_assertions
  tests/qtest/npcm7xx_rng-test: dump random data on failure

 tests/qtest/npcm7xx_gpio-test.c           |  1 -
 tests/qtest/npcm7xx_rng-test.c            | 15 +++++++++++++--
 tests/qtest/npcm7xx_timer-test.c          |  1 -
 tests/qtest/npcm7xx_watchdog_timer-test.c |  1 -
 4 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 1/3] tests/qtest/npcm7xx_rng-test: count runs properly

[Havard Skinnemoen via]

The number of runs is equal to the number of 0-1 and 1-0 transitions,
plus one. Currently, it's counting the number of times these transitions
do _not_ happen, plus one.

Source:
https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-22r1a.pdf
section 2.3.4 point (3).

Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
---
 tests/qtest/npcm7xx_rng-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/npcm7xx_rng-test.c b/tests/qtest/npcm7xx_rng-test.c
index da6e639bf6..57787c5ffc 100644
--- a/tests/qtest/npcm7xx_rng-test.c
+++ b/tests/qtest/npcm7xx_rng-test.c
@@ -126,7 +126,7 @@ static double calc_runs_p(const unsigned long *buf, unsigned int nr_bits)
     pi = (double)nr_ones / nr_bits;
 
     for (k = 0; k < nr_bits - 1; k++) {
-        vn_obs += !(test_bit(k, buf) ^ test_bit(k + 1, buf));
+        vn_obs += (test_bit(k, buf) ^ test_bit(k + 1, buf));
     }
     vn_obs += 1;
 
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 2/3] tests/qtest/npcm7xx: Don't call g_test_set_nonfatal_assertions

[Havard Skinnemoen via]

Even though g_test_set_nonfatal_assertions() makes test failure
reporting a lot better, no other tests currently do this so we'll turn
it off as well.

Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
---
 tests/qtest/npcm7xx_gpio-test.c           | 1 -
 tests/qtest/npcm7xx_rng-test.c            | 1 -
 tests/qtest/npcm7xx_timer-test.c          | 1 -
 tests/qtest/npcm7xx_watchdog_timer-test.c | 1 -
 4 files changed, 4 deletions(-)

diff --git a/tests/qtest/npcm7xx_gpio-test.c b/tests/qtest/npcm7xx_gpio-test.c
index 1004cef812..3af49173a7 100644
--- a/tests/qtest/npcm7xx_gpio-test.c
+++ b/tests/qtest/npcm7xx_gpio-test.c
@@ -357,7 +357,6 @@ int main(int argc, char **argv)
     int i;
 
     g_test_init(&argc, &argv, NULL);
-    g_test_set_nonfatal_assertions();
 
     qtest_add_func("/npcm7xx_gpio/dout_to_din", test_dout_to_din);
     qtest_add_func("/npcm7xx_gpio/pullup_pulldown", test_pullup_pulldown);
diff --git a/tests/qtest/npcm7xx_rng-test.c b/tests/qtest/npcm7xx_rng-test.c
index 57787c5ffc..d7e42cf062 100644
--- a/tests/qtest/npcm7xx_rng-test.c
+++ b/tests/qtest/npcm7xx_rng-test.c
@@ -261,7 +261,6 @@ int main(int argc, char **argv)
     int ret;
 
     g_test_init(&argc, &argv, NULL);
-    g_test_set_nonfatal_assertions();
 
     qtest_add_func("npcm7xx_rng/enable_disable", test_enable_disable);
     qtest_add_func("npcm7xx_rng/rosel", test_rosel);
diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c
index f08b0cd62a..77e6e0d472 100644
--- a/tests/qtest/npcm7xx_timer-test.c
+++ b/tests/qtest/npcm7xx_timer-test.c
@@ -530,7 +530,6 @@ int main(int argc, char **argv)
     int i, j;
 
     g_test_init(&argc, &argv, NULL);
-    g_test_set_nonfatal_assertions();
 
     for (i = 0; i < ARRAY_SIZE(timer_block); i++) {
         for (j = 0; j < ARRAY_SIZE(timer); j++) {
diff --git a/tests/qtest/npcm7xx_watchdog_timer-test.c b/tests/qtest/npcm7xx_watchdog_timer-test.c
index 54d5d6d8f2..426303ecfa 100644
--- a/tests/qtest/npcm7xx_watchdog_timer-test.c
+++ b/tests/qtest/npcm7xx_watchdog_timer-test.c
@@ -303,7 +303,6 @@ static void watchdog_add_test(const char *name, const Watchdog* wd,
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
-    g_test_set_nonfatal_assertions();
 
     for (int i = 0; i < ARRAY_SIZE(watchdog_list); ++i) {
         const Watchdog *wd = &watchdog_list[i];
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 3/3] tests/qtest/npcm7xx_rng-test: dump random data on failure

[Havard Skinnemoen via]

Dump the collected random data after a randomness test failure.

Note that you won't actually see this unless you add
g_test_set_nonfatal_assertions() back in.

Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
---
 tests/qtest/npcm7xx_rng-test.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/qtest/npcm7xx_rng-test.c b/tests/qtest/npcm7xx_rng-test.c
index d7e42cf062..09111d640c 100644
--- a/tests/qtest/npcm7xx_rng-test.c
+++ b/tests/qtest/npcm7xx_rng-test.c
@@ -20,6 +20,7 @@
 
 #include "libqtest-single.h"
 #include "qemu/bitops.h"
+#include "qemu-common.h"
 
 #define RNG_BASE_ADDR   0xf000b000
 
@@ -36,6 +37,13 @@
 /* Number of bits to collect for randomness tests. */
 #define TEST_INPUT_BITS  (128)
 
+static void dump_buf_if_failed(const uint8_t *buf, size_t size)
+{
+    if (g_test_failed()) {
+        qemu_hexdump(stderr, "", buf, size);
+    }
+}
+
 static void rng_writeb(unsigned int offset, uint8_t value)
 {
     writeb(RNG_BASE_ADDR + offset, value);
@@ -188,6 +196,7 @@ static void test_continuous_monobit(void)
     }
 
     g_assert_cmpfloat(calc_monobit_p(buf, sizeof(buf)), >, 0.01);
+    dump_buf_if_failed(buf, sizeof(buf));
 }
 
 /*
@@ -209,6 +218,7 @@ static void test_continuous_runs(void)
     }
 
     g_assert_cmpfloat(calc_runs_p(buf.l, sizeof(buf) * BITS_PER_BYTE), >, 0.01);
+    dump_buf_if_failed(buf.c, sizeof(buf));
 }
 
 /*
@@ -230,6 +240,7 @@ static void test_first_byte_monobit(void)
     }
 
     g_assert_cmpfloat(calc_monobit_p(buf, sizeof(buf)), >, 0.01);
+    dump_buf_if_failed(buf, sizeof(buf));
 }
 
 /*
@@ -254,6 +265,7 @@ static void test_first_byte_runs(void)
     }
 
     g_assert_cmpfloat(calc_runs_p(buf.l, sizeof(buf) * BITS_PER_BYTE), >, 0.01);
+    dump_buf_if_failed(buf.c, sizeof(buf));
 }
 
 int main(int argc, char **argv)
-- 
2.29.1.341.ge80a0c044ae-goog

Re: [PATCH 0/3] tests/qtest: npcm7xx test fixes

[Philippe Mathieu-Daudé]

Cc'ing Daniel (patches 1-3) & Marc-André (2).

On 11/3/20 2:14 AM, Havard Skinnemoen via wrote:
> This series contains a fix for the randomness calculation in npcm7xx_rng-test.
> It also makes test failures fatal. The last patch would have dumped the random
> data to stderr if the randomness test fails, except now that failures are
> fatal, it never actually gets a chance to do that.
> 
> It may not make sense to apply all three, but I'd definitely take (1), and I'll
> leave it up to you whether to apply (2), (3) or both.
> 
> Havard Skinnemoen (3):
>   tests/qtest/npcm7xx_rng-test: count runs properly
>   tests/qtest/npcm7xx: Don't call g_test_set_nonfatal_assertions
>   tests/qtest/npcm7xx_rng-test: dump random data on failure
> 
>  tests/qtest/npcm7xx_gpio-test.c           |  1 -
>  tests/qtest/npcm7xx_rng-test.c            | 15 +++++++++++++--
>  tests/qtest/npcm7xx_timer-test.c          |  1 -
>  tests/qtest/npcm7xx_watchdog_timer-test.c |  1 -
>  4 files changed, 13 insertions(+), 5 deletions(-)
> 

Re: [PATCH 0/3] tests/qtest: npcm7xx test fixes

[Peter Maydell]

On Tue, 3 Nov 2020 at 01:52, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Cc'ing Daniel (patches 1-3) & Marc-André (2).
>
> On 11/3/20 2:14 AM, Havard Skinnemoen via wrote:
> > This series contains a fix for the randomness calculation in npcm7xx_rng-test.
> > It also makes test failures fatal. The last patch would have dumped the random
> > data to stderr if the randomness test fails, except now that failures are
> > fatal, it never actually gets a chance to do that.
> >
> > It may not make sense to apply all three, but I'd definitely take (1), and I'll
> > leave it up to you whether to apply (2), (3) or both.
> >
> > Havard Skinnemoen (3):
> >   tests/qtest/npcm7xx_rng-test: count runs properly
> >   tests/qtest/npcm7xx: Don't call g_test_set_nonfatal_assertions
> >   tests/qtest/npcm7xx_rng-test: dump random data on failure

I've applied patch 1 to target-arm.next but will wait to see
if anybody has an opinion about patches 2 and 3.

thanks
-- PMM

Re: [PATCH 0/3] tests/qtest: npcm7xx test fixes

[Peter Maydell]

On Tue, 10 Nov 2020 at 10:48, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 3 Nov 2020 at 01:52, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > Cc'ing Daniel (patches 1-3) & Marc-André (2).
> >
> > On 11/3/20 2:14 AM, Havard Skinnemoen via wrote:
> > > This series contains a fix for the randomness calculation in npcm7xx_rng-test.
> > > It also makes test failures fatal. The last patch would have dumped the random
> > > data to stderr if the randomness test fails, except now that failures are
> > > fatal, it never actually gets a chance to do that.
> > >
> > > It may not make sense to apply all three, but I'd definitely take (1), and I'll
> > > leave it up to you whether to apply (2), (3) or both.
> > >
> > > Havard Skinnemoen (3):
> > >   tests/qtest/npcm7xx_rng-test: count runs properly
> > >   tests/qtest/npcm7xx: Don't call g_test_set_nonfatal_assertions
> > >   tests/qtest/npcm7xx_rng-test: dump random data on failure
>
> I've applied patch 1 to target-arm.next but will wait to see
> if anybody has an opinion about patches 2 and 3.

Had a brief conversation on IRC with Dan about this. The nicest
thing to do seems to be to call g_test_set_nonfatal_assertions()
somewhere more generic, but just at the moment there isn't a
place for that. So for the moment I'm going to take patch 3,
and we'll leave the g_test_set_nonfatal_assertions in the npcm7xx
tests; as and when we make that setting more generally we can
remove these specific calls.

thanks
-- PMM