| Message ID | 20241211233727.98923-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | qtest: Remove uses of 'first_cpu' | expand |
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~
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.
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.
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.
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
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.
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/