| Message ID | 20240618144009.3137806-1-zheyuma97@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu | expand |
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);
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
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.
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
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
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);
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(+)