[PATCH] hvf: arm: Remove $pc from trace_hvf_data_abort()

[PATCH] hvf: arm: Remove $pc from trace_hvf_data_abort()

[Zenghui Yu]

We don't synchronize vcpu registers from the hardware accelerator (e.g., by
cpu_synchronize_state()) in the Dabort handler, so env->pc points to the
instruction which has nothing to do with the Dabort at all.

And it doesn't seem to make much sense to log PC in every Dabort handler,
let's just remove it from this trace event.

Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
---
 target/arm/hvf/hvf.c        | 2 +-
 target/arm/hvf/trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index c9cfcdc08b..8f93e42b34 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -2005,7 +2005,7 @@ int hvf_vcpu_exec(CPUState *cpu)
         uint32_t cm = (syndrome >> 8) & 0x1;
         uint64_t val = 0;
 
-        trace_hvf_data_abort(env->pc, hvf_exit->exception.virtual_address,
+        trace_hvf_data_abort(hvf_exit->exception.virtual_address,
                              hvf_exit->exception.physical_address, isv,
                              iswrite, s1ptw, len, srt);
 
diff --git a/target/arm/hvf/trace-events b/target/arm/hvf/trace-events
index b49746f28d..b29a995f3d 100644
--- a/target/arm/hvf/trace-events
+++ b/target/arm/hvf/trace-events
@@ -2,7 +2,7 @@ hvf_unhandled_sysreg_read(uint64_t pc, uint32_t reg, uint32_t op0, uint32_t op1,
 hvf_unhandled_sysreg_write(uint64_t pc, uint32_t reg, uint32_t op0, uint32_t op1, uint32_t crn, uint32_t crm, uint32_t op2) "unhandled sysreg write at pc=0x%"PRIx64": 0x%08x (op0=%d op1=%d crn=%d crm=%d op2=%d)"
 hvf_inject_fiq(void) "injecting FIQ"
 hvf_inject_irq(void) "injecting IRQ"
-hvf_data_abort(uint64_t pc, uint64_t va, uint64_t pa, bool isv, bool iswrite, bool s1ptw, uint32_t len, uint32_t srt) "data abort: [pc=0x%"PRIx64" va=0x%016"PRIx64" pa=0x%016"PRIx64" isv=%d iswrite=%d s1ptw=%d len=%d srt=%d]"
+hvf_data_abort(uint64_t va, uint64_t pa, bool isv, bool iswrite, bool s1ptw, uint32_t len, uint32_t srt) "data abort: [va=0x%016"PRIx64" pa=0x%016"PRIx64" isv=%d iswrite=%d s1ptw=%d len=%d srt=%d]"
 hvf_sysreg_read(uint32_t reg, uint32_t op0, uint32_t op1, uint32_t crn, uint32_t crm, uint32_t op2, uint64_t val) "sysreg read 0x%08x (op0=%d op1=%d crn=%d crm=%d op2=%d) = 0x%016"PRIx64
 hvf_sysreg_write(uint32_t reg, uint32_t op0, uint32_t op1, uint32_t crn, uint32_t crm, uint32_t op2, uint64_t val) "sysreg write 0x%08x (op0=%d op1=%d crn=%d crm=%d op2=%d, val=0x%016"PRIx64")"
 hvf_unknown_hvc(uint64_t pc, uint64_t x0) "pc=0x%"PRIx64" unknown HVC! 0x%016"PRIx64
-- 
2.34.1

Re: [PATCH] hvf: arm: Remove $pc from trace_hvf_data_abort()

[Mads Ynddal]

> On 13 Jul 2025, at 17.47, Zenghui Yu <zenghui.yu@linux.dev> wrote:
> 
> We don't synchronize vcpu registers from the hardware accelerator (e.g., by
> cpu_synchronize_state()) in the Dabort handler, so env->pc points to the
> instruction which has nothing to do with the Dabort at all.
> 
> And it doesn't seem to make much sense to log PC in every Dabort handler,
> let's just remove it from this trace event.
> 
> Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>

I agree. Unless cpu_synchronize_state(cpu) is called, the value in env->pc is
stale.

Reviewed-by: Mads Ynddal <mads@ynddal.dk>

Re: [PATCH] hvf: arm: Remove $pc from trace_hvf_data_abort()

[Peter Maydell]

On Sun, 13 Jul 2025 at 16:47, Zenghui Yu <zenghui.yu@linux.dev> wrote:
>
> We don't synchronize vcpu registers from the hardware accelerator (e.g., by
> cpu_synchronize_state()) in the Dabort handler, so env->pc points to the
> instruction which has nothing to do with the Dabort at all.
>
> And it doesn't seem to make much sense to log PC in every Dabort handler,
> let's just remove it from this trace event.



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH] hvf: arm: Remove $pc from trace_hvf_data_abort()

[Philippe Mathieu-Daudé]

On 13/7/25 17:47, Zenghui Yu wrote:
> We don't synchronize vcpu registers from the hardware accelerator (e.g., by
> cpu_synchronize_state()) in the Dabort handler, so env->pc points to the
> instruction which has nothing to do with the Dabort at all.
> 
> And it doesn't seem to make much sense to log PC in every Dabort handler,
> let's just remove it from this trace event.
> 
> Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> ---
>   target/arm/hvf/hvf.c        | 2 +-
>   target/arm/hvf/trace-events | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

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