[PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[Philippe Mathieu-Daudé]

Replace first_cpu->as by address_space_memory.

Philippe Mathieu-Daudé (2):
  system/qtest: Remove uses of 'first_cpu'
  qtest/fuzz: Remove uses of 'first_cpu'

 system/qtest.c                    | 53 ++++++++++++++++---------------
 tests/qtest/fuzz/generic_fuzz.c   |  3 +-
 tests/qtest/fuzz/qtest_wrappers.c | 53 ++++++++++++++++---------------
 3 files changed, 56 insertions(+), 53 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] system/qtest: Remove uses of 'first_cpu'

[Philippe Mathieu-Daudé]

There is no vCPU within the QTest accelerator (well, they
are stubs doing nothing, see dummy_cpu_thread_fn).

Directly access the global &address_space_memory to reduce
access to the 'first_cpu' global (which is meaningless in
a heterogeneous emulation setup).

Cast the returned value to (void) to explicit we don't
care about invalid accesses.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
We will likely remove &address_space_memory too, but one
global at a time. first_cpu is more annoying so I'm starting
with it.
---
 system/qtest.c | 53 +++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/system/qtest.c b/system/qtest.c
index 12703a20455..cc26dd75bef 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -16,6 +16,7 @@
 #include "sysemu/qtest.h"
 #include "sysemu/runstate.h"
 #include "chardev/char-fe.h"
+#include "exec/address-spaces.h"
 #include "exec/ioport.h"
 #include "exec/memory.h"
 #include "exec/tswap.h"
@@ -514,23 +515,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
 
         if (words[0][5] == 'b') {
             uint8_t data = value;
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                &data, 1);
+            (void)address_space_write(&address_space_memory, addr,
+                                      MEMTXATTRS_UNSPECIFIED, &data, 1);
         } else if (words[0][5] == 'w') {
             uint16_t data = value;
             tswap16s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                &data, 2);
+            (void)address_space_write(&address_space_memory, addr,
+                                      MEMTXATTRS_UNSPECIFIED, &data, 2);
         } else if (words[0][5] == 'l') {
             uint32_t data = value;
             tswap32s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                &data, 4);
+            (void)address_space_write(&address_space_memory, addr,
+                                      MEMTXATTRS_UNSPECIFIED, &data, 4);
         } else if (words[0][5] == 'q') {
             uint64_t data = value;
             tswap64s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                &data, 8);
+            (void)address_space_write(&address_space_memory, addr,
+                                      MEMTXATTRS_UNSPECIFIED, &data, 8);
         }
         qtest_send_prefix(chr);
         qtest_send(chr, "OK\n");
@@ -548,22 +549,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
 
         if (words[0][4] == 'b') {
             uint8_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                               &data, 1);
+            (void)address_space_read(&address_space_memory, addr,
+                                     MEMTXATTRS_UNSPECIFIED, &data, 1);
             value = data;
         } else if (words[0][4] == 'w') {
             uint16_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                               &data, 2);
+            (void)address_space_read(&address_space_memory, addr,
+                                     MEMTXATTRS_UNSPECIFIED, &data, 2);
             value = tswap16(data);
         } else if (words[0][4] == 'l') {
             uint32_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                               &data, 4);
+            (void)address_space_read(&address_space_memory, addr,
+                                     MEMTXATTRS_UNSPECIFIED, &data, 4);
             value = tswap32(data);
         } else if (words[0][4] == 'q') {
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                               &value, 8);
+            (void)address_space_read(&address_space_memory, addr,
+                                     MEMTXATTRS_UNSPECIFIED, &value, 8);
             tswap64s(&value);
         }
         qtest_send_prefix(chr);
@@ -583,8 +584,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         g_assert(len);
 
         data = g_malloc(len);
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                           len);
+        (void)address_space_read(&address_space_memory, addr,
+                                 MEMTXATTRS_UNSPECIFIED, data, len);
 
         enc = qemu_hexdump_line(NULL, data, len, 0, 0);
 
@@ -605,8 +606,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         g_assert(ret == 0);
 
         data = g_malloc(len);
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                           len);
+        (void)address_space_read(&address_space_memory, addr,
+                                 MEMTXATTRS_UNSPECIFIED, data, len);
         b64_data = g_base64_encode(data, len);
         qtest_send_prefix(chr);
         qtest_sendf(chr, "OK %s\n", b64_data);
@@ -640,8 +641,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                 data[i] = 0;
             }
         }
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                            len);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, data, len);
         g_free(data);
 
         qtest_send_prefix(chr);
@@ -663,8 +664,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         if (len) {
             data = g_malloc(len);
             memset(data, pattern, len);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                data, len);
+            (void)address_space_write(&address_space_memory, addr,
+                                      MEMTXATTRS_UNSPECIFIED, data, len);
             g_free(data);
         }
 
@@ -697,8 +698,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
             out_len = MIN(out_len, len);
         }
 
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                            len);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, data, len);
 
         qtest_send_prefix(chr);
         qtest_send(chr, "OK\n");
-- 
2.45.2

[PATCH v2 2/2] qtest/fuzz: Remove uses of 'first_cpu'

[Philippe Mathieu-Daudé]

There is no vCPU within the QTest accelerator (well, they
are stubs doing nothing, see dummy_cpu_thread_fn).

Directly access the global &address_space_memory to reduce
access to the 'first_cpu' global (which is meaningless in
a heterogeneous emulation setup).

Cast the returned value to (void) to explicit we don't
care about invalid accesses.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
We will likely remove &address_space_memory too, but one
global at a time. first_cpu is more annoying so I'm starting
with it.
---
 tests/qtest/fuzz/generic_fuzz.c   |  3 +-
 tests/qtest/fuzz/qtest_wrappers.c | 53 ++++++++++++++++---------------
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index d107a496da6..a72a3e99f7d 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -20,6 +20,7 @@
 #include "tests/qtest/libqos/pci-pc.h"
 #include "fuzz.h"
 #include "string.h"
+#include "exec/address-spaces.h"
 #include "exec/memory.h"
 #include "exec/ramblock.h"
 #include "hw/qdev-core.h"
@@ -239,7 +240,7 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr)
     MemoryRegion *mr1;
     while (len > 0) {
         l = len;
-        mr1 = address_space_translate(first_cpu->as,
+        mr1 = address_space_translate(&address_space_memory,
                                       addr, &addr1, &l, true,
                                       MEMTXATTRS_UNSPECIFIED);
 
diff --git a/tests/qtest/fuzz/qtest_wrappers.c b/tests/qtest/fuzz/qtest_wrappers.c
index 0580f8df860..a2d562dc7ed 100644
--- a/tests/qtest/fuzz/qtest_wrappers.c
+++ b/tests/qtest/fuzz/qtest_wrappers.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/core/cpu.h"
+#include "exec/address-spaces.h"
 #include "exec/ioport.h"
 
 #include "fuzz.h"
@@ -107,8 +108,8 @@ uint8_t __wrap_qtest_readb(QTestState *s, uint64_t addr)
 {
     uint8_t value;
     if (!serialize) {
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            &value, 1);
+        (void)address_space_read(&address_space_memory, addr,
+                                 MEMTXATTRS_UNSPECIFIED, &value, 1);
         return value;
     } else {
         return __real_qtest_readb(s, addr);
@@ -119,8 +120,8 @@ uint16_t __wrap_qtest_readw(QTestState *s, uint64_t addr)
 {
     uint16_t value;
     if (!serialize) {
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            &value, 2);
+        (void)address_space_read(&address_space_memory, addr,
+                                 MEMTXATTRS_UNSPECIFIED, &value, 2);
         return value;
     } else {
         return __real_qtest_readw(s, addr);
@@ -131,8 +132,8 @@ uint32_t __wrap_qtest_readl(QTestState *s, uint64_t addr)
 {
     uint32_t value;
     if (!serialize) {
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            &value, 4);
+        (void)address_space_read(&address_space_memory, addr,
+                                 MEMTXATTRS_UNSPECIFIED, &value, 4);
         return value;
     } else {
         return __real_qtest_readl(s, addr);
@@ -143,8 +144,8 @@ uint64_t __wrap_qtest_readq(QTestState *s, uint64_t addr)
 {
     uint64_t value;
     if (!serialize) {
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            &value, 8);
+        (void)address_space_read(&address_space_memory, addr,
+                                 MEMTXATTRS_UNSPECIFIED, &value, 8);
         return value;
     } else {
         return __real_qtest_readq(s, addr);
@@ -154,8 +155,8 @@ uint64_t __wrap_qtest_readq(QTestState *s, uint64_t addr)
 void __wrap_qtest_writeb(QTestState *s, uint64_t addr, uint8_t value)
 {
     if (!serialize) {
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            &value, 1);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, &value, 1);
     } else {
         __real_qtest_writeb(s, addr, value);
     }
@@ -164,8 +165,8 @@ void __wrap_qtest_writeb(QTestState *s, uint64_t addr, uint8_t value)
 void __wrap_qtest_writew(QTestState *s, uint64_t addr, uint16_t value)
 {
     if (!serialize) {
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            &value, 2);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, &value, 2);
     } else {
         __real_qtest_writew(s, addr, value);
     }
@@ -174,8 +175,8 @@ void __wrap_qtest_writew(QTestState *s, uint64_t addr, uint16_t value)
 void __wrap_qtest_writel(QTestState *s, uint64_t addr, uint32_t value)
 {
     if (!serialize) {
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            &value, 4);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, &value, 4);
     } else {
         __real_qtest_writel(s, addr, value);
     }
@@ -184,8 +185,8 @@ void __wrap_qtest_writel(QTestState *s, uint64_t addr, uint32_t value)
 void __wrap_qtest_writeq(QTestState *s, uint64_t addr, uint64_t value)
 {
     if (!serialize) {
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            &value, 8);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, &value, 8);
     } else {
         __real_qtest_writeq(s, addr, value);
     }
@@ -194,8 +195,8 @@ void __wrap_qtest_writeq(QTestState *s, uint64_t addr, uint64_t value)
 void __wrap_qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
 {
     if (!serialize) {
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                           size);
+        (void)address_space_read(&address_space_memory, addr,
+                                 MEMTXATTRS_UNSPECIFIED, data, size);
     } else {
         __real_qtest_memread(s, addr, data, size);
     }
@@ -204,8 +205,8 @@ void __wrap_qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
 void __wrap_qtest_bufread(QTestState *s, uint64_t addr, void *data, size_t size)
 {
     if (!serialize) {
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                           size);
+        (void)address_space_read(&address_space_memory, addr,
+                                 MEMTXATTRS_UNSPECIFIED, data, size);
     } else {
         __real_qtest_bufread(s, addr, data, size);
     }
@@ -215,8 +216,8 @@ void __wrap_qtest_memwrite(QTestState *s, uint64_t addr, const void *data,
                            size_t size)
 {
     if (!serialize) {
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            data, size);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, data, size);
     } else {
         __real_qtest_memwrite(s, addr, data, size);
     }
@@ -226,8 +227,8 @@ void __wrap_qtest_bufwrite(QTestState *s, uint64_t addr,
                     const void *data, size_t size)
 {
     if (!serialize) {
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            data, size);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, data, size);
     } else {
         __real_qtest_bufwrite(s, addr, data, size);
     }
@@ -239,8 +240,8 @@ void __wrap_qtest_memset(QTestState *s, uint64_t addr,
     if (!serialize) {
         data = malloc(size);
         memset(data, patt, size);
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                            data, size);
+        (void)address_space_write(&address_space_memory, addr,
+                                  MEMTXATTRS_UNSPECIFIED, data, size);
     } else {
         __real_qtest_memset(s, addr, patt, size);
     }
-- 
2.45.2

Re: [PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[Richard Henderson]

On 12/11/24 17:37, Philippe Mathieu-Daudé wrote:
> Replace first_cpu->as by address_space_memory.
> 
> Philippe Mathieu-Daudé (2):
>    system/qtest: Remove uses of 'first_cpu'
>    qtest/fuzz: Remove uses of 'first_cpu'
> 
>   system/qtest.c                    | 53 ++++++++++++++++---------------
>   tests/qtest/fuzz/generic_fuzz.c   |  3 +-
>   tests/qtest/fuzz/qtest_wrappers.c | 53 ++++++++++++++++---------------
>   3 files changed, 56 insertions(+), 53 deletions(-)
> 

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[Philippe Mathieu-Daudé]

Hi Fabiano,

On 12/12/24 00:37, Philippe Mathieu-Daudé wrote:
> Replace first_cpu->as by address_space_memory.
> 
> Philippe Mathieu-Daudé (2):
>    system/qtest: Remove uses of 'first_cpu'
>    qtest/fuzz: Remove uses of 'first_cpu'
> 
>   system/qtest.c                    | 53 ++++++++++++++++---------------
>   tests/qtest/fuzz/generic_fuzz.c   |  3 +-
>   tests/qtest/fuzz/qtest_wrappers.c | 53 ++++++++++++++++---------------
>   3 files changed, 56 insertions(+), 53 deletions(-)

Ping :)

Tell me if you prefer I merge these patch myself.

Regards,

Phil.

Re: [PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Fabiano,
>
> On 12/12/24 00:37, Philippe Mathieu-Daudé wrote:
>> Replace first_cpu->as by address_space_memory.
>> 
>> Philippe Mathieu-Daudé (2):
>>    system/qtest: Remove uses of 'first_cpu'
>>    qtest/fuzz: Remove uses of 'first_cpu'
>> 
>>   system/qtest.c                    | 53 ++++++++++++++++---------------
>>   tests/qtest/fuzz/generic_fuzz.c   |  3 +-
>>   tests/qtest/fuzz/qtest_wrappers.c | 53 ++++++++++++++++---------------
>>   3 files changed, 56 insertions(+), 53 deletions(-)
>
> Ping :)

Hi!

Good that you pinged, I thought you were looking into the test failures
from v1. I copied you in this other thread that mentioned them as well:

https://lore.kernel.org/r/87y10jctbd.fsf@suse.de

Applying this series on top of master just now:

Summary of Failures:

 10/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_usart-test               ERROR            1.40s   exit status 1
165/519 qemu:qtest+qtest-arm / qtest-arm/sse-timer-test                     ERROR            0.40s   killed by signal 6 SIGABRT
185/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_exti-test                ERROR            0.19s   exit status 1
187/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_rcc-test                 ERROR            0.19s   exit status 1
515/519 qemu:qtest+qtest-arm / qtest-arm/microbit-test                      TIMEOUT         60.01s   killed by signal 15 SIGTERM


>
> Tell me if you prefer I merge these patch myself.
>
> Regards,
>
> Phil.

Re: [PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[Philippe Mathieu-Daudé]

On 3/1/25 13:26, Fabiano Rosas wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Hi Fabiano,
>>
>> On 12/12/24 00:37, Philippe Mathieu-Daudé wrote:
>>> Replace first_cpu->as by address_space_memory.
>>>
>>> Philippe Mathieu-Daudé (2):
>>>     system/qtest: Remove uses of 'first_cpu'
>>>     qtest/fuzz: Remove uses of 'first_cpu'

>> Ping :)
> 
> Hi!
> 
> Good that you pinged, I thought you were looking into the test failures
> from v1. I copied you in this other thread that mentioned them as well:
> 
> https://lore.kernel.org/r/87y10jctbd.fsf@suse.de
> 
> Applying this series on top of master just now:
> 
> Summary of Failures:
> 
>   10/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_usart-test               ERROR            1.40s   exit status 1
> 165/519 qemu:qtest+qtest-arm / qtest-arm/sse-timer-test                     ERROR            0.40s   killed by signal 6 SIGABRT
> 185/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_exti-test                ERROR            0.19s   exit status 1
> 187/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_rcc-test                 ERROR            0.19s   exit status 1
> 515/519 qemu:qtest+qtest-arm / qtest-arm/microbit-test                      TIMEOUT         60.01s   killed by signal 15 SIGTERM

Hmm interesting, I have a "quick-before-posting-series" set
of tests, and a "full-before-posting-pullreq" one. The quick
is quite extensive, but only include Aarch64. I thought, since
it contains the ARM targets, they would also be tested, but
no...

All these tests use ARM Cortex-M cores, which have a particularity,
the ARM cores are tied to architectural specific HW (NVIC). We
model cores in target/arm/cpu, and HW part in hw/intc/armv7m_nvic.c.

NVIC is only 'visible' from vCPU address space, not the "main sysbus".

See this flatview diff of tcg/qtest:

@@ -1,11 +1,12 @@
  FlatView #1
- AS "cpu-memory-0", root: armv7m-container
+ AS "memory", root: system
- Root memory region: armv7m-container
+ Root memory region: system
    0000000000000000-00000000000fffff (prio 0, rom): flash
    0000000008000000-00000000080fffff (prio 0, rom): flash
    0000000010000000-0000000010007fff (prio 0, ram): SRAM2
    0000000020000000-0000000020017fff (prio 0, ram): SRAM1
-  0000000022000000-0000000023ffffff (prio 0, i/o): bitband
    0000000040000000-00000000400003ff (prio -1000, i/o): TIM2
    0000000040000400-00000000400007ff (prio -1000, i/o): TIM3
    0000000040000800-0000000040000bff (prio -1000, i/o): TIM4
@@ -52,7 +53,6 @@
    0000000040022000-00000000400223ff (prio -1000, i/o): FLASH
    0000000040023000-00000000400233ff (prio -1000, i/o): CRC
    0000000040024000-00000000400243ff (prio -1000, i/o): TSC
-  0000000042000000-0000000043ffffff (prio 0, i/o): bitband
    0000000048000000-00000000480003ff (prio 0, i/o): stm32l4x5-gpio
    0000000048000400-00000000480007ff (prio 0, i/o): stm32l4x5-gpio
    0000000048000800-0000000048000bff (prio 0, i/o): stm32l4x5-gpio
@@ -66,9 +66,4 @@
    0000000050060800-0000000050060bff (prio -1000, i/o): RNG
    00000000a0000000-00000000a0000fff (prio -1000, i/o): FMC
    00000000a0001000-00000000a00013ff (prio -1000, i/o): QUADSPI
-  00000000e0000000-00000000e000dfff (prio -1, i/o): nvic-default
-  00000000e000e000-00000000e000e00f (prio 0, i/o): nvic_sysregs
-  00000000e000e010-00000000e000e0ef (prio 1, i/o): v7m_systick
-  00000000e000e0f0-00000000e000efff (prio 0, i/o): nvic_sysregs 
@00000000000000f0
-  00000000e000f000-00000000e00fffff (prio -1, i/o): nvic-default 
@000000000000f000

So under qtest with no vcpu, the nvic is not accessible without
specifying a non-global address space.

And qtests access NVIC, see this function:

     #define NVIC_ISPR1 0XE000E204
     #define NVIC_ICPR1 0xE000E284
     #define USART1_IRQ 37

     static bool check_nvic_pending(QTestState *qts, unsigned int n)
     {
         /* No USART interrupts are less than 32 */
         assert(n > 32);
         n -= 32;
         return qtest_readl(qts, NVIC_ISPR1) & (1 << n);
     }

I tend to think the current situation works by luck, and this series
is yet another example of sysbus abuses.

I'll give it some thoughts. Maybe we can discuss it at the next
community call.

Regards,

Phil.

Re: [PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[Thomas Huth]

On 04/01/2025 00.48, Philippe Mathieu-Daudé wrote:
> On 3/1/25 13:26, Fabiano Rosas wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> Hi Fabiano,
>>>
>>> On 12/12/24 00:37, Philippe Mathieu-Daudé wrote:
>>>> Replace first_cpu->as by address_space_memory.
>>>>
>>>> Philippe Mathieu-Daudé (2):
>>>>     system/qtest: Remove uses of 'first_cpu'
>>>>     qtest/fuzz: Remove uses of 'first_cpu'
> 
>>> Ping :)
>>
>> Hi!
>>
>> Good that you pinged, I thought you were looking into the test failures
>> from v1. I copied you in this other thread that mentioned them as well:
>>
>> https://lore.kernel.org/r/87y10jctbd.fsf@suse.de
>>
>> Applying this series on top of master just now:
>>
>> Summary of Failures:
>>
>>   10/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_usart- 
>> test               ERROR            1.40s   exit status 1
>> 165/519 qemu:qtest+qtest-arm / qtest-arm/sse-timer- 
>> test                     ERROR            0.40s   killed by signal 6 SIGABRT
>> 185/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_exti- 
>> test                ERROR            0.19s   exit status 1
>> 187/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_rcc- 
>> test                 ERROR            0.19s   exit status 1
>> 515/519 qemu:qtest+qtest-arm / qtest-arm/microbit- 
>> test                      TIMEOUT         60.01s   killed by signal 15 
>> SIGTERM
> 
> Hmm interesting, I have a "quick-before-posting-series" set
> of tests, and a "full-before-posting-pullreq" one. The quick
> is quite extensive, but only include Aarch64. I thought, since
> it contains the ARM targets, they would also be tested, but
> no...

IIRC this was a deliberate decision once in the past to avoid double 
testing: qtests_aarch64 in tests/qtest/meson.build does not include 
qtest_arm, it's a separate set of tests indeed.

IMHO it's a little bit unfortunate, since in a couple of spots in the CI, we 
are taking the shortcut of only adding aarch64-softmmu to the target list, 
but not arm-softmmu.

Maybe we should add some logic to tests/qtest/meson.build so that if 
arm-softmmu is not in target_dirs, we add the tests to qtests_aarch64 ?

  Thomas

Re: [PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[Philippe Mathieu-Daudé]

On 7/1/25 09:03, Thomas Huth wrote:
> On 04/01/2025 00.48, Philippe Mathieu-Daudé wrote:
>> On 3/1/25 13:26, Fabiano Rosas wrote:
>>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>>
>>>> Hi Fabiano,
>>>>
>>>> On 12/12/24 00:37, Philippe Mathieu-Daudé wrote:
>>>>> Replace first_cpu->as by address_space_memory.
>>>>>
>>>>> Philippe Mathieu-Daudé (2):
>>>>>     system/qtest: Remove uses of 'first_cpu'
>>>>>     qtest/fuzz: Remove uses of 'first_cpu'
>>
>>>> Ping :)
>>>
>>> Hi!
>>>
>>> Good that you pinged, I thought you were looking into the test failures
>>> from v1. I copied you in this other thread that mentioned them as well:
>>>
>>> https://lore.kernel.org/r/87y10jctbd.fsf@suse.de
>>>
>>> Applying this series on top of master just now:
>>>
>>> Summary of Failures:
>>>
>>>   10/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_usart- 
>>> test               ERROR            1.40s   exit status 1
>>> 165/519 qemu:qtest+qtest-arm / qtest-arm/sse-timer- 
>>> test                     ERROR            0.40s   killed by signal 6 
>>> SIGABRT
>>> 185/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_exti- 
>>> test                ERROR            0.19s   exit status 1
>>> 187/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_rcc- 
>>> test                 ERROR            0.19s   exit status 1
>>> 515/519 qemu:qtest+qtest-arm / qtest-arm/microbit- 
>>> test                      TIMEOUT         60.01s   killed by signal 
>>> 15 SIGTERM
>>
>> Hmm interesting, I have a "quick-before-posting-series" set
>> of tests, and a "full-before-posting-pullreq" one. The quick
>> is quite extensive, but only include Aarch64. I thought, since
>> it contains the ARM targets, they would also be tested, but
>> no...
> 
> IIRC this was a deliberate decision once in the past to avoid double 
> testing: qtests_aarch64 in tests/qtest/meson.build does not include 
> qtest_arm, it's a separate set of tests indeed.
> 
> IMHO it's a little bit unfortunate, since in a couple of spots in the 
> CI, we are taking the shortcut of only adding aarch64-softmmu to the 
> target list, but not arm-softmmu.
> 
> Maybe we should add some logic to tests/qtest/meson.build so that if 
> arm-softmmu is not in target_dirs, we add the tests to qtests_aarch64 ?

I'm working in unifying both targets. With that in mind, optimizing
CI coverage doesn't seem a good use of our time IMHO, because once
I get there the CI will also be unified.

Re: [PATCH v2 0/2] qtest: Remove uses of 'first_cpu'

[Philippe Mathieu-Daudé]

On 4/1/25 00:48, Philippe Mathieu-Daudé wrote:
> On 3/1/25 13:26, Fabiano Rosas wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> Hi Fabiano,
>>>
>>> On 12/12/24 00:37, Philippe Mathieu-Daudé wrote:
>>>> Replace first_cpu->as by address_space_memory.
>>>>
>>>> Philippe Mathieu-Daudé (2):
>>>>     system/qtest: Remove uses of 'first_cpu'
>>>>     qtest/fuzz: Remove uses of 'first_cpu'
> 
>>> Ping :)
>>
>> Hi!
>>
>> Good that you pinged, I thought you were looking into the test failures
>> from v1. I copied you in this other thread that mentioned them as well:
>>
>> https://lore.kernel.org/r/87y10jctbd.fsf@suse.de
>>
>> Applying this series on top of master just now:
>>
>> Summary of Failures:
>>
>>   10/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_usart- 
>> test               ERROR            1.40s   exit status 1
>> 165/519 qemu:qtest+qtest-arm / qtest-arm/sse-timer- 
>> test                     ERROR            0.40s   killed by signal 6 
>> SIGABRT
>> 185/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_exti- 
>> test                ERROR            0.19s   exit status 1
>> 187/519 qemu:qtest+qtest-arm / qtest-arm/stm32l4x5_rcc- 
>> test                 ERROR            0.19s   exit status 1
>> 515/519 qemu:qtest+qtest-arm / qtest-arm/microbit- 
>> test                      TIMEOUT         60.01s   killed by signal 15 
>> SIGTERM
> 
> Hmm interesting, I have a "quick-before-posting-series" set
> of tests, and a "full-before-posting-pullreq" one. The quick
> is quite extensive, but only include Aarch64. I thought, since
> it contains the ARM targets, they would also be tested, but
> no...
> 
> All these tests use ARM Cortex-M cores, which have a particularity,
> the ARM cores are tied to architectural specific HW (NVIC). We
> model cores in target/arm/cpu, and HW part in hw/intc/armv7m_nvic.c.
> 
> NVIC is only 'visible' from vCPU address space, not the "main sysbus".
> 
> See this flatview diff of tcg/qtest:
> 
> @@ -1,11 +1,12 @@
>   FlatView #1
> - AS "cpu-memory-0", root: armv7m-container
> + AS "memory", root: system
> - Root memory region: armv7m-container
> + Root memory region: system
>     0000000000000000-00000000000fffff (prio 0, rom): flash
>     0000000008000000-00000000080fffff (prio 0, rom): flash
>     0000000010000000-0000000010007fff (prio 0, ram): SRAM2
>     0000000020000000-0000000020017fff (prio 0, ram): SRAM1
> -  0000000022000000-0000000023ffffff (prio 0, i/o): bitband
>     0000000040000000-00000000400003ff (prio -1000, i/o): TIM2
>     0000000040000400-00000000400007ff (prio -1000, i/o): TIM3
>     0000000040000800-0000000040000bff (prio -1000, i/o): TIM4
> @@ -52,7 +53,6 @@
>     0000000040022000-00000000400223ff (prio -1000, i/o): FLASH
>     0000000040023000-00000000400233ff (prio -1000, i/o): CRC
>     0000000040024000-00000000400243ff (prio -1000, i/o): TSC
> -  0000000042000000-0000000043ffffff (prio 0, i/o): bitband
>     0000000048000000-00000000480003ff (prio 0, i/o): stm32l4x5-gpio
>     0000000048000400-00000000480007ff (prio 0, i/o): stm32l4x5-gpio
>     0000000048000800-0000000048000bff (prio 0, i/o): stm32l4x5-gpio
> @@ -66,9 +66,4 @@
>     0000000050060800-0000000050060bff (prio -1000, i/o): RNG
>     00000000a0000000-00000000a0000fff (prio -1000, i/o): FMC
>     00000000a0001000-00000000a00013ff (prio -1000, i/o): QUADSPI
> -  00000000e0000000-00000000e000dfff (prio -1, i/o): nvic-default
> -  00000000e000e000-00000000e000e00f (prio 0, i/o): nvic_sysregs
> -  00000000e000e010-00000000e000e0ef (prio 1, i/o): v7m_systick
> -  00000000e000e0f0-00000000e000efff (prio 0, i/o): nvic_sysregs 
> @00000000000000f0
> -  00000000e000f000-00000000e00fffff (prio -1, i/o): nvic-default 
> @000000000000f000
> 
> So under qtest with no vcpu, the nvic is not accessible without
> specifying a non-global address space.
> 
> And qtests access NVIC, see this function:
> 
>      #define NVIC_ISPR1 0XE000E204
>      #define NVIC_ICPR1 0xE000E284
>      #define USART1_IRQ 37
> 
>      static bool check_nvic_pending(QTestState *qts, unsigned int n)
>      {
>          /* No USART interrupts are less than 32 */
>          assert(n > 32);
>          n -= 32;
>          return qtest_readl(qts, NVIC_ISPR1) & (1 << n);
>      }
> 
> I tend to think the current situation works by luck, and this series
> is yet another example of sysbus abuses.
> 
> I'll give it some thoughts. Maybe we can discuss it at the next
> community call.

Tentative fix after today's community call:
https://lore.kernel.org/qemu-devel/20250107192637.67683-1-philmd@linaro.org/