hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

[PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

Posted by Zheyu Ma 5 months, 1 week ago

This commit updates the a9_gtimer_get_current_cpu() function to handle
cases where QTest is enabled. When QTest is used, it returns 0 instead
of dereferencing the current_cpu, which can be NULL. This prevents the
program from crashing during QTest runs.

Reproducer:
cat << EOF | qemu-system-aarch64 -display \
none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
writel 0xf03fe20c 0x26d7468c
EOF

Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 hw/timer/a9gtimer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
index a2ac5bdfb9..64d80cdf6a 100644
--- a/hw/timer/a9gtimer.c
+++ b/hw/timer/a9gtimer.c
@@ -32,6 +32,7 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "hw/core/cpu.h"
+#include "sysemu/qtest.h"
 
 #ifndef A9_GTIMER_ERR_DEBUG
 #define A9_GTIMER_ERR_DEBUG 0
@@ -48,6 +49,10 @@
 
 static inline int a9_gtimer_get_current_cpu(A9GTimerState *s)
 {
+    if (qtest_enabled()) {
+        return 0;
+    }
+
     if (current_cpu->cpu_index >= s->num_cpu) {
         hw_error("a9gtimer: num-cpu %d but this cpu is %d!\n",
                  s->num_cpu, current_cpu->cpu_index);
-- 
2.34.1

Re: [PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

Posted by Philippe Mathieu-Daudé 5 months, 1 week ago

On 18/6/24 16:40, Zheyu Ma wrote:
> This commit updates the a9_gtimer_get_current_cpu() function to handle
> cases where QTest is enabled. When QTest is used, it returns 0 instead
> of dereferencing the current_cpu, which can be NULL. This prevents the
> program from crashing during QTest runs.
> 
> Reproducer:
> cat << EOF | qemu-system-aarch64 -display \
> none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
> writel 0xf03fe20c 0x26d7468c
> EOF
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>   hw/timer/a9gtimer.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
> index a2ac5bdfb9..64d80cdf6a 100644
> --- a/hw/timer/a9gtimer.c
> +++ b/hw/timer/a9gtimer.c
> @@ -32,6 +32,7 @@
>   #include "qemu/log.h"
>   #include "qemu/module.h"
>   #include "hw/core/cpu.h"
> +#include "sysemu/qtest.h"
>   
>   #ifndef A9_GTIMER_ERR_DEBUG
>   #define A9_GTIMER_ERR_DEBUG 0
> @@ -48,6 +49,10 @@
>   
>   static inline int a9_gtimer_get_current_cpu(A9GTimerState *s)
>   {
> +    if (qtest_enabled()) {
> +        return 0;

Indeed this is how we fixed hw/intc/arm_gic in commit 09bbdb89bc,
so:

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

> +    }
> +
>       if (current_cpu->cpu_index >= s->num_cpu) {

That said, such accesses of @current_cpu from hw/ are dubious.

>           hw_error("a9gtimer: num-cpu %d but this cpu is %d!\n",
>                    s->num_cpu, current_cpu->cpu_index);

Re: [PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

Posted by Peter Maydell 5 months, 1 week ago

On Tue, 18 Jun 2024 at 15:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 18/6/24 16:40, Zheyu Ma wrote:
> > This commit updates the a9_gtimer_get_current_cpu() function to handle
> > cases where QTest is enabled. When QTest is used, it returns 0 instead
> > of dereferencing the current_cpu, which can be NULL. This prevents the
> > program from crashing during QTest runs.
> >
> > Reproducer:
> > cat << EOF | qemu-system-aarch64 -display \
> > none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
> > writel 0xf03fe20c 0x26d7468c
> > EOF
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >   hw/timer/a9gtimer.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
> > index a2ac5bdfb9..64d80cdf6a 100644
> > --- a/hw/timer/a9gtimer.c
> > +++ b/hw/timer/a9gtimer.c
> > @@ -32,6 +32,7 @@
> >   #include "qemu/log.h"
> >   #include "qemu/module.h"
> >   #include "hw/core/cpu.h"
> > +#include "sysemu/qtest.h"
> >
> >   #ifndef A9_GTIMER_ERR_DEBUG
> >   #define A9_GTIMER_ERR_DEBUG 0
> > @@ -48,6 +49,10 @@
> >
> >   static inline int a9_gtimer_get_current_cpu(A9GTimerState *s)
> >   {
> > +    if (qtest_enabled()) {
> > +        return 0;
>
> Indeed this is how we fixed hw/intc/arm_gic in commit 09bbdb89bc,
> so:
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> > +    }
> > +
> >       if (current_cpu->cpu_index >= s->num_cpu) {
>
> That said, such accesses of @current_cpu from hw/ are dubious.

True, but I'm not sure we ever settled on the right way to avoid
them, did we?

Anyway, I've applied this patch to target-arm.next.

-- PMM

Re: [PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

Posted by Philippe Mathieu-Daudé 5 months, 1 week ago

On 20/6/24 12:10, Peter Maydell wrote:
> On Tue, 18 Jun 2024 at 15:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 18/6/24 16:40, Zheyu Ma wrote:
>>> This commit updates the a9_gtimer_get_current_cpu() function to handle
>>> cases where QTest is enabled. When QTest is used, it returns 0 instead
>>> of dereferencing the current_cpu, which can be NULL. This prevents the
>>> program from crashing during QTest runs.
>>>
>>> Reproducer:
>>> cat << EOF | qemu-system-aarch64 -display \
>>> none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
>>> writel 0xf03fe20c 0x26d7468c
>>> EOF
>>>
>>> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
>>> ---
>>>    hw/timer/a9gtimer.c | 5 +++++
>>>    1 file changed, 5 insertions(+)


>>>        if (current_cpu->cpu_index >= s->num_cpu) {
>>
>> That said, such accesses of @current_cpu from hw/ are dubious.
> 
> True, but I'm not sure we ever settled on the right way to avoid
> them, did we?

No we didn't, it is still in my TODO list; we might discuss it
when I post my RFC.

Regards,

Phil.

Re: [PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

Posted by Edgar E. Iglesias via 5 months, 1 week ago

On Thu, Jun 20, 2024 at 12:25:51PM +0200, Philippe Mathieu-Daudé wrote:
> On 20/6/24 12:10, Peter Maydell wrote:
> > On Tue, 18 Jun 2024 at 15:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > > 
> > > On 18/6/24 16:40, Zheyu Ma wrote:
> > > > This commit updates the a9_gtimer_get_current_cpu() function to handle
> > > > cases where QTest is enabled. When QTest is used, it returns 0 instead
> > > > of dereferencing the current_cpu, which can be NULL. This prevents the
> > > > program from crashing during QTest runs.
> > > > 
> > > > Reproducer:
> > > > cat << EOF | qemu-system-aarch64 -display \
> > > > none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
> > > > writel 0xf03fe20c 0x26d7468c
> > > > EOF
> > > > 
> > > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > > > ---
> > > >    hw/timer/a9gtimer.c | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> 
> 
> > > >        if (current_cpu->cpu_index >= s->num_cpu) {
> > > 
> > > That said, such accesses of @current_cpu from hw/ are dubious.
> > 
> > True, but I'm not sure we ever settled on the right way to avoid
> > them, did we?
> 
> No we didn't, it is still in my TODO list; we might discuss it
> when I post my RFC.
>

Yeah, this way of getting the core id is a problem when having multiple
ARM CPU subsystems (and for heterogenous cores).

IIRC, when I looked at what the GIC v2 HW does, the GIC exposes an AMBA
port for each CPU. In my mental model that would translate to exposing
multiple Memory Reginos (sysbus_init_mmio) and mapping the appropriate
device MR to each CPU AddressSpace.

We could also do it with memory attributes but I don't think the
master Ids are standardised enough to extract a core-index from
with out having SoC specific code,, at least not accross Xilinx devices.

I never looked at newer GIC versions nor the mmio mapped timers
though...

Cheers,
Edgar

Re: [PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

Posted by Peter Maydell 5 months, 1 week ago

On Thu, 20 Jun 2024 at 12:16, Edgar E. Iglesias <edgar.iglesias@amd.com> wrote:
>
> On Thu, Jun 20, 2024 at 12:25:51PM +0200, Philippe Mathieu-Daudé wrote:
> > On 20/6/24 12:10, Peter Maydell wrote:
> > > On Tue, 18 Jun 2024 at 15:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > > >
> > > > On 18/6/24 16:40, Zheyu Ma wrote:
> > > > > This commit updates the a9_gtimer_get_current_cpu() function to handle
> > > > > cases where QTest is enabled. When QTest is used, it returns 0 instead
> > > > > of dereferencing the current_cpu, which can be NULL. This prevents the
> > > > > program from crashing during QTest runs.
> > > > >
> > > > > Reproducer:
> > > > > cat << EOF | qemu-system-aarch64 -display \
> > > > > none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
> > > > > writel 0xf03fe20c 0x26d7468c
> > > > > EOF
> > > > >
> > > > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > > > > ---
> > > > >    hw/timer/a9gtimer.c | 5 +++++
> > > > >    1 file changed, 5 insertions(+)
> >
> >
> > > > >        if (current_cpu->cpu_index >= s->num_cpu) {
> > > >
> > > > That said, such accesses of @current_cpu from hw/ are dubious.
> > >
> > > True, but I'm not sure we ever settled on the right way to avoid
> > > them, did we?
> >
> > No we didn't, it is still in my TODO list; we might discuss it
> > when I post my RFC.
> >
>
> Yeah, this way of getting the core id is a problem when having multiple
> ARM CPU subsystems (and for heterogenous cores).
>
> IIRC, when I looked at what the GIC v2 HW does, the GIC exposes an AMBA
> port for each CPU. In my mental model that would translate to exposing
> multiple Memory Reginos (sysbus_init_mmio) and mapping the appropriate
> device MR to each CPU AddressSpace.

Yeah. The trouble is that doing this requires massively rearranging
how all the GICv2 board models connect up address spaces...

> We could also do it with memory attributes but I don't think the
> master Ids are standardised enough to extract a core-index from
> with out having SoC specific code,, at least not accross Xilinx devices.

...and yes, using the requester ID to specify the CPU in the
MemTxAttrs is the other proposal.

thanks
-- PMM