[RFC] tests/qtest/stm32l4x5_usart: Avoid accessing NVIC via MMIO

The STM32L4x5 SoC family use a ARM Cortex-M core. Such
core is architecturally tied with a NVIC (interrupt controller),
having the NVIC MMIO mapped in the core address space.

When using the QTest accelerator, we don't emulate vCPU, only
a dummy is created. For now, QTest is only supposed to access
MMIO devices mapped on the main 'SysBus'. Thus it shouldn't
be able to access a NVIC MMIO region, because such region is
specific to a vCPU address space, which isn't available under
QTest.

In order to avoid NVIC MMIO accesses, rather than checking
UART IRQs on the NVIC, intercept the UART IRQ and check for
raised/lowered events.

The sysbus USART1 IRQ output is wired to EXTI #26 input.
Use qtest_irq_intercept_out_named() to intercept it, count
the events with qtest_get_irq_lowered_counter() and
qtest_get_irq_raised_counter().

Remove the then unused check/clear_nvic_pending() methods.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Based-on: <20241216141818.111255-7-gustavo.romero@linaro.org>
"tests/qtest: Add API functions to capture IRQ toggling"

This patch is to fix the problem reported by Fabiano when
removing the &first_cpu global in qtest, see analysis in:
https://lore.kernel.org/qemu-devel/05820c9b-a683-4eb4-a836-e97aa708d5e5@linaro.org/

Note, while writing that patch I noticed a problem with the
b-l475e-iot01a machine. In bl475e_init() somes output GPIOs
are wired twice. The EXTI outputs are passed to the SoC with
qdev_pass_gpios(), and few are re-wired to the various output
IRQ splitters. I'll open a GitLab issue so it can be cleared
later.
---
 tests/qtest/stm32l4x5_usart-test.c | 33 +++++++-----------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

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

> The STM32L4x5 SoC family use a ARM Cortex-M core. Such
> core is architecturally tied with a NVIC (interrupt controller),
> having the NVIC MMIO mapped in the core address space.
>
> When using the QTest accelerator, we don't emulate vCPU, only
> a dummy is created. For now, QTest is only supposed to access
> MMIO devices mapped on the main 'SysBus'. Thus it shouldn't
> be able to access a NVIC MMIO region, because such region is
> specific to a vCPU address space, which isn't available under
> QTest.
>
> In order to avoid NVIC MMIO accesses, rather than checking
> UART IRQs on the NVIC, intercept the UART IRQ and check for
> raised/lowered events.
>
> The sysbus USART1 IRQ output is wired to EXTI #26 input.
> Use qtest_irq_intercept_out_named() to intercept it, count
> the events with qtest_get_irq_lowered_counter() and
> qtest_get_irq_raised_counter().
>
> Remove the then unused check/clear_nvic_pending() methods.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Based-on: <20241216141818.111255-7-gustavo.romero@linaro.org>
> "tests/qtest: Add API functions to capture IRQ toggling"
>
> This patch is to fix the problem reported by Fabiano when
> removing the &first_cpu global in qtest, see analysis in:
> https://lore.kernel.org/qemu-devel/05820c9b-a683-4eb4-a836-e97aa708d5e5@linaro.org/
>
> Note, while writing that patch I noticed a problem with the
> b-l475e-iot01a machine. In bl475e_init() somes output GPIOs
> are wired twice. The EXTI outputs are passed to the SoC with
> qdev_pass_gpios(), and few are re-wired to the various output
> IRQ splitters. I'll open a GitLab issue so it can be cleared
> later.
> ---
>  tests/qtest/stm32l4x5_usart-test.c | 33 +++++++-----------------------

What about these other ones?
stm32l4x5_exti-test.c
stm32l4x5_rcc-test.c

>  1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/tests/qtest/stm32l4x5_usart-test.c b/tests/qtest/stm32l4x5_usart-test.c
> index 927bab63614..35622e9434d 100644
> --- a/tests/qtest/stm32l4x5_usart-test.c
> +++ b/tests/qtest/stm32l4x5_usart-test.c
> @@ -46,26 +46,7 @@ REG32(ICR, 0x20)
>  REG32(RDR, 0x24)
>  REG32(TDR, 0x28)
>  
> -#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);
> -}
> -
> -static bool clear_nvic_pending(QTestState *qts, unsigned int n)
> -{
> -    /* No USART interrupts are less than 32 */
> -    assert(n > 32);
> -    n -= 32;
> -    qtest_writel(qts, NVIC_ICPR1, (1 << n));
> -    return true;
> -}
> +#define USART1_EXTI_IRQ 26
>  
>  /*
>   * Wait indefinitely for the flag to be updated.
> @@ -195,6 +176,8 @@ static void init_uart(QTestState *qts)
>      /* Enable the transmitter, the receiver and the USART. */
>      qtest_writel(qts, (USART1_BASE_ADDR + A_CR1),
>          cr1 | R_CR1_UE_MASK | R_CR1_RE_MASK | R_CR1_TE_MASK);
> +
> +    qtest_irq_intercept_out_named(qts, "machine/soc/exti", "sysbus-irq");
>  }
>  
>  static void test_write_read(void)
> @@ -221,7 +204,7 @@ static void test_receive_char(void)
>      g_assert_true(send(sock_fd, "a", 1, 0) == 1);
>      usart_wait_for_flag(qts, USART1_BASE_ADDR + A_ISR, R_ISR_RXNE_MASK);
>      g_assert_cmphex(qtest_readl(qts, USART1_BASE_ADDR + A_RDR), ==, 'a');
> -    g_assert_false(check_nvic_pending(qts, USART1_IRQ));
> +    g_assert_cmpuint(qtest_get_irq_lowered_counter(qts, USART1_EXTI_IRQ), ==, 0);
>  
>      /* Now with the IRQ */
>      cr1 = qtest_readl(qts, (USART1_BASE_ADDR + A_CR1));
> @@ -230,8 +213,7 @@ static void test_receive_char(void)
>      g_assert_true(send(sock_fd, "b", 1, 0) == 1);
>      usart_wait_for_flag(qts, USART1_BASE_ADDR + A_ISR, R_ISR_RXNE_MASK);
>      g_assert_cmphex(qtest_readl(qts, USART1_BASE_ADDR + A_RDR), ==, 'b');
> -    g_assert_true(check_nvic_pending(qts, USART1_IRQ));
> -    clear_nvic_pending(qts, USART1_IRQ);
> +    g_assert_cmpuint(qtest_get_irq_lowered_counter(qts, USART1_EXTI_IRQ), >, 0);
>  
>      close(sock_fd);
>  
> @@ -251,7 +233,7 @@ static void test_send_char(void)
>      qtest_writel(qts, USART1_BASE_ADDR + A_TDR, 'c');
>      g_assert_true(recv(sock_fd, s, 1, 0) == 1);
>      g_assert_cmphex(s[0], ==, 'c');
> -    g_assert_false(check_nvic_pending(qts, USART1_IRQ));
> +    g_assert_cmpuint(qtest_get_irq_lowered_counter(qts, USART1_EXTI_IRQ), ==, 0);
>  
>      /* Now with the IRQ */
>      cr1 = qtest_readl(qts, (USART1_BASE_ADDR + A_CR1));
> @@ -260,8 +242,7 @@ static void test_send_char(void)
>      qtest_writel(qts, USART1_BASE_ADDR + A_TDR, 'd');
>      g_assert_true(recv(sock_fd, s, 1, 0) == 1);
>      g_assert_cmphex(s[0], ==, 'd');
> -    g_assert_true(check_nvic_pending(qts, USART1_IRQ));
> -    clear_nvic_pending(qts, USART1_IRQ);
> +    g_assert_cmpuint(qtest_get_irq_raised_counter(qts, USART1_EXTI_IRQ), >, 0);
>  
>      close(sock_fd);

On 9/1/25 16:11, Fabiano Rosas wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> The STM32L4x5 SoC family use a ARM Cortex-M core. Such
>> core is architecturally tied with a NVIC (interrupt controller),
>> having the NVIC MMIO mapped in the core address space.
>>
>> When using the QTest accelerator, we don't emulate vCPU, only
>> a dummy is created. For now, QTest is only supposed to access
>> MMIO devices mapped on the main 'SysBus'. Thus it shouldn't
>> be able to access a NVIC MMIO region, because such region is
>> specific to a vCPU address space, which isn't available under
>> QTest.
>>
>> In order to avoid NVIC MMIO accesses, rather than checking
>> UART IRQs on the NVIC, intercept the UART IRQ and check for
>> raised/lowered events.
>>
>> The sysbus USART1 IRQ output is wired to EXTI #26 input.
>> Use qtest_irq_intercept_out_named() to intercept it, count
>> the events with qtest_get_irq_lowered_counter() and
>> qtest_get_irq_raised_counter().
>>
>> Remove the then unused check/clear_nvic_pending() methods.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> Based-on: <20241216141818.111255-7-gustavo.romero@linaro.org>
>> "tests/qtest: Add API functions to capture IRQ toggling"
>>
>> This patch is to fix the problem reported by Fabiano when
>> removing the &first_cpu global in qtest, see analysis in:
>> https://lore.kernel.org/qemu-devel/05820c9b-a683-4eb4-a836-e97aa708d5e5@linaro.org/
>>
>> Note, while writing that patch I noticed a problem with the
>> b-l475e-iot01a machine. In bl475e_init() somes output GPIOs
>> are wired twice. The EXTI outputs are passed to the SoC with
>> qdev_pass_gpios(), and few are re-wired to the various output
>> IRQ splitters. I'll open a GitLab issue so it can be cleared
>> later.
>> ---
>>   tests/qtest/stm32l4x5_usart-test.c | 33 +++++++-----------------------
> 
> What about these other ones?
> stm32l4x5_exti-test.c
> stm32l4x5_rcc-test.c

Same changes, but I'd like to get feedback on this approach first,
because we lose the NVIC coverage. If we want to keep testing it,
then we need to rework the qtest mmio API to somehow specify which
address space is to be used; or a new API to resolve a device
particular MR and access it, without using the current global
sysbus address space.

On Tue, 7 Jan 2025 at 19:26, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The STM32L4x5 SoC family use a ARM Cortex-M core. Such
> core is architecturally tied with a NVIC (interrupt controller),
> having the NVIC MMIO mapped in the core address space.
>
> When using the QTest accelerator, we don't emulate vCPU, only
> a dummy is created. For now, QTest is only supposed to access
> MMIO devices mapped on the main 'SysBus'. Thus it shouldn't
> be able to access a NVIC MMIO region, because such region is
> specific to a vCPU address space, which isn't available under
> QTest.

This doesn't seem quite right. We create vCPUs which should
have all their usual address spaces (e.g. in the arm code
the calls to cpu_address_space_init() are not guarded by
anything that says "don't do this if qtest"). They just
don't *execute* anything.

qtest's model of operation is "the qtest is doing accesses
as-if it was guest code running on the first CPU in the system".
So it should be fine for it to do accesses which access the
NVIC. (Indeed if we wanted a qtest of the NVIC itself that
should also be OK.)

In a more interesting heterogenous system you probably want
the qtest to be able to select the CPU that it is acting
as-if-it-were (though even then the first one in the system
is usually going to be sensible enough, assuming the
heterogenous board model did the usual thing of creating
the main application CPUs first and the BSP or other
minor processor last). Then you could have a qtest which is
"I'm testing a device that's accessible to the main
application processor" and also one which is "I'm testing
this device which is only accessible to the BSP".

> This patch is to fix the problem reported by Fabiano when
> removing the &first_cpu global in qtest, see analysis in:
> https://lore.kernel.org/qemu-devel/05820c9b-a683-4eb4-a836-e97aa708d5e5@linaro.org/

Hmm, OK, so my idea of having qtest use address_space_memory
as a shortcut for getting rid of first_cpu doesn't work.

I'm not super-enthusiastic about this patch, since it's
basically adding an artificial restriction to what you can
do in a qtest. But maybe it's OK as a temporary thing to
avoid going too far down a rabbit hole in refactoring?

If we were going to have the qtest select "this is the
CPU I want to act like", what would it do? Pass a
QOM path to the relevant CPU? I feel like you would
still want to default to "pick the first CPU", because
99% of tests will be fine with that. But maybe we need
to locate the first CPU by some runtime thing rather
than by using the global. IDK...

-- PMM