[PATCH 0/6] target/arm: Few accel cleanups

[PATCH 0/6] target/arm: Few accel cleanups

[Philippe Mathieu-Daudé]

Based-on: <20250623121845.7214-1-philmd@linaro.org>
     "arm: Fixes and preparatory cleanups for split-accel"

Philippe Mathieu-Daudé (6):
  target/arm: Only set CPU_INTERRUPT_EXITTB for TCG
  target/arm: Only allow disabling NEON when using TCG
  target/arm: Better describe PMU depends on TCG or HVF
  target/arm: Re-use arm_is_psci_call() in HVF
  target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF
  target/arm: Allow arm_cpu_tlb_fill_align optionally set
    CPUTLBEntryFull

 target/arm/cpu.c            |  4 ++--
 target/arm/helper.c         |  2 +-
 target/arm/hvf/hvf.c        |  7 ++++---
 target/arm/machine.c        | 10 +++++-----
 target/arm/tcg/psci.c       |  3 +++
 target/arm/tcg/tlb_helper.c |  4 +++-
 target/arm/trace-events     |  3 +++
 7 files changed, 21 insertions(+), 12 deletions(-)

-- 
2.49.0

[PATCH 1/6] target/arm: Only set CPU_INTERRUPT_EXITTB for TCG

[Philippe Mathieu-Daudé]

Commit 34c45d53026 ("target-arm: kvm - re-inject guest debug
exceptions") removed CPU_INTERRUPT_EXITTB from KVM, but it
also appears on HVF. Better to restrict it to TCG.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 889d3088079..764b1f82dc2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10929,7 +10929,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
 
     arm_call_el_change_hook(cpu);
 
-    if (!kvm_enabled()) {
+    if (tcg_enabled()) {
         cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
     }
 }
-- 
2.49.0

[PATCH 2/6] target/arm: Only allow disabling NEON when using TCG

[Philippe Mathieu-Daudé]

Only allow disabling NEON when using TCG.

This avoids confusing user experience:

  $ qemu-system-aarch64 -M virt -accel hvf \
                        -cpu host,neon=off,vfp=off,vfp-d32=off
  qemu-system-aarch64: AArch64 CPUs must have both VFP and Neon or neither

  $ qemu-system-aarch64 -M virt -accel hvf \
                        -cpu host,neon=off,vfp=off,vfp-d32=off
  qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither

  $ qemu-system-aarch64 -M virt -accel hvf \
                        -cpu host,neon=off,vfp=off,vfp-d32=off
  qemu-system-aarch64: can't apply global host-arm-cpu.vfp-d32=off: Property 'host-arm-cpu.vfp-d32' not found

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index ab5fbd9b40b..b6a8ba83a46 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1810,7 +1810,7 @@ static void arm_cpu_post_init(Object *obj)
 
     if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
         cpu->has_neon = true;
-        if (!kvm_enabled()) {
+        if (tcg_enabled() || qtest_enabled()) {
             qdev_property_add_static(DEVICE(obj), &arm_cpu_has_neon_property);
         }
     }
-- 
2.49.0

[PATCH 3/6] target/arm: Better describe PMU depends on TCG or HVF

[Philippe Mathieu-Daudé]

Simplify PMU logic by rewriting '!KVM' as 'TCG || HVF'
(ignoring QTest, because vCPUs are not available there).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.c     |  2 +-
 target/arm/machine.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b6a8ba83a46..0311ff315fe 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2352,7 +2352,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     if (arm_feature(env, ARM_FEATURE_PMU)) {
         pmu_init(cpu);
 
-        if (!kvm_enabled()) {
+        if (tcg_enabled() || hvf_enabled()) {
             arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
             arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
         }
diff --git a/target/arm/machine.c b/target/arm/machine.c
index e442d485241..baa7ad25ca9 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -1,7 +1,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "qemu/error-report.h"
-#include "system/kvm.h"
+#include "system/hvf.h"
 #include "system/tcg.h"
 #include "kvm_arm.h"
 #include "internals.h"
@@ -853,7 +853,7 @@ static int cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
-    if (!kvm_enabled()) {
+    if (tcg_enabled() || hvf_enabled()) {
         pmu_op_start(&cpu->env);
     }
 
@@ -888,7 +888,7 @@ static int cpu_post_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
-    if (!kvm_enabled()) {
+    if (tcg_enabled() || hvf_enabled()) {
         pmu_op_finish(&cpu->env);
     }
 
@@ -921,7 +921,7 @@ static int cpu_pre_load(void *opaque)
      */
     env->irq_line_state = UINT32_MAX;
 
-    if (!kvm_enabled()) {
+    if (tcg_enabled() || hvf_enabled()) {
         pmu_op_start(env);
     }
 
@@ -1013,7 +1013,7 @@ static int cpu_post_load(void *opaque, int version_id)
         }
     }
 
-    if (!kvm_enabled()) {
+    if (tcg_enabled() || hvf_enabled()) {
         pmu_op_finish(env);
     }
 
-- 
2.49.0

[PATCH 4/6] target/arm: Re-use arm_is_psci_call() in HVF

[Philippe Mathieu-Daudé]

Re-use arm_is_psci_call() instead of open-coding it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/hvf/hvf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 52199c4ff9d..7a99118c8c2 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -2084,7 +2084,7 @@ int hvf_vcpu_exec(CPUState *cpu)
         break;
     case EC_AA64_HVC:
         cpu_synchronize_state(cpu);
-        if (arm_cpu->psci_conduit == QEMU_PSCI_CONDUIT_HVC) {
+        if (arm_is_psci_call(arm_cpu, EXCP_HVC)) {
             if (!hvf_handle_psci_call(cpu)) {
                 trace_hvf_unknown_hvc(env->pc, env->xregs[0]);
                 /* SMCCC 1.3 section 5.2 says every unknown SMCCC call returns -1 */
@@ -2097,7 +2097,7 @@ int hvf_vcpu_exec(CPUState *cpu)
         break;
     case EC_AA64_SMC:
         cpu_synchronize_state(cpu);
-        if (arm_cpu->psci_conduit == QEMU_PSCI_CONDUIT_SMC) {
+        if (arm_is_psci_call(arm_cpu, EXCP_SMC)) {
             advance_pc = true;
 
             if (!hvf_handle_psci_call(cpu)) {
-- 
2.49.0

[PATCH 5/6] target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF

[Philippe Mathieu-Daudé]

It is useful to compare PSCI calls of the same guest running
under TCG or HVF.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/hvf/hvf.c    | 3 ++-
 target/arm/tcg/psci.c   | 3 +++
 target/arm/trace-events | 3 +++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 7a99118c8c2..6309c5b872e 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -34,6 +34,7 @@
 #include "target/arm/multiprocessing.h"
 #include "target/arm/gtimer.h"
 #include "trace.h"
+#include "../trace.h"
 #include "migration/vmstate.h"
 
 #include "gdbstub/enums.h"
@@ -1149,7 +1150,7 @@ static bool hvf_handle_psci_call(CPUState *cpu)
     int target_el = 1;
     int32_t ret = 0;
 
-    trace_hvf_psci_call(param[0], param[1], param[2], param[3],
+    trace_arm_psci_call(param[0], param[1], param[2], param[3],
                         arm_cpu_mp_affinity(arm_cpu));
 
     switch (param[0]) {
diff --git a/target/arm/tcg/psci.c b/target/arm/tcg/psci.c
index cabed43e8a8..8df27ca123e 100644
--- a/target/arm/tcg/psci.c
+++ b/target/arm/tcg/psci.c
@@ -25,6 +25,7 @@
 #include "internals.h"
 #include "arm-powerctl.h"
 #include "target/arm/multiprocessing.h"
+#include "../trace.h"
 
 bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
 {
@@ -79,6 +80,8 @@ void arm_handle_psci_call(ARMCPU *cpu)
          */
         param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];
     }
+    trace_arm_psci_call(param[0], param[1], param[2], param[3],
+                        arm_cpu_mp_affinity(cpu));
 
     if ((param[0] & QEMU_PSCI_0_2_64BIT) && !is_a64(env)) {
         ret = QEMU_PSCI_RET_NOT_SUPPORTED;
diff --git a/target/arm/trace-events b/target/arm/trace-events
index 4438dce7bec..a9cb5e0f5c6 100644
--- a/target/arm/trace-events
+++ b/target/arm/trace-events
@@ -13,3 +13,6 @@ arm_gt_update_irq(int timer, int irqstate) "gt_update_irq: timer %d irqstate %d"
 
 # kvm.c
 kvm_arm_fixup_msi_route(uint64_t iova, uint64_t gpa) "MSI iova = 0x%"PRIx64" is translated into 0x%"PRIx64
+
+# tcg/psci.c and hvf/hvf.c
+arm_psci_call(uint64_t x0, uint64_t x1, uint64_t x2, uint64_t x3, uint32_t cpuid) "PSCI Call x0=0x%016"PRIx64" x1=0x%016"PRIx64" x2=0x%016"PRIx64" x3=0x%016"PRIx64" cpuid=0x%x"
-- 
2.49.0

[PATCH 6/6] target/arm: Allow arm_cpu_tlb_fill_align optionally set CPUTLBEntryFull

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/tcg/tlb_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/arm/tcg/tlb_helper.c b/target/arm/tcg/tlb_helper.c
index 23c72a99f5c..df04ef351d1 100644
--- a/target/arm/tcg/tlb_helper.c
+++ b/target/arm/tcg/tlb_helper.c
@@ -349,7 +349,9 @@ bool arm_cpu_tlb_fill_align(CPUState *cs, CPUTLBEntryFull *out, vaddr address,
                               &res, fi)) {
         res.f.extra.arm.pte_attrs = res.cacheattrs.attrs;
         res.f.extra.arm.shareability = res.cacheattrs.shareability;
-        *out = res.f;
+        if (out) {
+            *out = res.f;
+        }
         return true;
     }
     if (probe) {
-- 
2.49.0

Re: [PATCH 1/6] target/arm: Only set CPU_INTERRUPT_EXITTB for TCG

[Richard Henderson]

On 6/30/25 07:09, Philippe Mathieu-Daudé wrote:
> Commit 34c45d53026 ("target-arm: kvm - re-inject guest debug
> exceptions") removed CPU_INTERRUPT_EXITTB from KVM, but it
> also appears on HVF. Better to restrict it to TCG.
> 
> Suggested-by: Richard Henderson<richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/6] target/arm: Only allow disabling NEON when using TCG

[Richard Henderson]

On 6/30/25 07:09, Philippe Mathieu-Daudé wrote:
> Only allow disabling NEON when using TCG.
> 
> This avoids confusing user experience:
> 
>    $ qemu-system-aarch64 -M virt -accel hvf \
>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>    qemu-system-aarch64: AArch64 CPUs must have both VFP and Neon or neither
> 
>    $ qemu-system-aarch64 -M virt -accel hvf \
>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>    qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
> 
>    $ qemu-system-aarch64 -M virt -accel hvf \
>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>    qemu-system-aarch64: can't apply global host-arm-cpu.vfp-d32=off: Property 'host-arm-cpu.vfp-d32' not found
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index ab5fbd9b40b..b6a8ba83a46 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1810,7 +1810,7 @@ static void arm_cpu_post_init(Object *obj)
>   
>       if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>           cpu->has_neon = true;
> -        if (!kvm_enabled()) {
> +        if (tcg_enabled() || qtest_enabled()) {
>               qdev_property_add_static(DEVICE(obj), &arm_cpu_has_neon_property);
>           }
>       }

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/6] target/arm: Re-use arm_is_psci_call() in HVF

[Richard Henderson]

On 6/30/25 07:09, Philippe Mathieu-Daudé wrote:
> Re-use arm_is_psci_call() instead of open-coding it.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/hvf/hvf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 5/6] target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF

[Richard Henderson]

On 6/30/25 07:09, Philippe Mathieu-Daudé wrote:
> It is useful to compare PSCI calls of the same guest running
> under TCG or HVF.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/hvf/hvf.c    | 3 ++-
>   target/arm/tcg/psci.c   | 3 +++
>   target/arm/trace-events | 3 +++
>   3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 7a99118c8c2..6309c5b872e 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -34,6 +34,7 @@
>   #include "target/arm/multiprocessing.h"
>   #include "target/arm/gtimer.h"
>   #include "trace.h"
> +#include "../trace.h"
>   #include "migration/vmstate.h"
>   
>   #include "gdbstub/enums.h"
> @@ -1149,7 +1150,7 @@ static bool hvf_handle_psci_call(CPUState *cpu)
>       int target_el = 1;
>       int32_t ret = 0;
>   
> -    trace_hvf_psci_call(param[0], param[1], param[2], param[3],
> +    trace_arm_psci_call(param[0], param[1], param[2], param[3],
>                           arm_cpu_mp_affinity(arm_cpu));

Lacks removal of the hvf trace?

r~

>   
>       switch (param[0]) {
> diff --git a/target/arm/tcg/psci.c b/target/arm/tcg/psci.c
> index cabed43e8a8..8df27ca123e 100644
> --- a/target/arm/tcg/psci.c
> +++ b/target/arm/tcg/psci.c
> @@ -25,6 +25,7 @@
>   #include "internals.h"
>   #include "arm-powerctl.h"
>   #include "target/arm/multiprocessing.h"
> +#include "../trace.h"
>   
>   bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>   {
> @@ -79,6 +80,8 @@ void arm_handle_psci_call(ARMCPU *cpu)
>            */
>           param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];
>       }
> +    trace_arm_psci_call(param[0], param[1], param[2], param[3],
> +                        arm_cpu_mp_affinity(cpu));
>   
>       if ((param[0] & QEMU_PSCI_0_2_64BIT) && !is_a64(env)) {
>           ret = QEMU_PSCI_RET_NOT_SUPPORTED;
> diff --git a/target/arm/trace-events b/target/arm/trace-events
> index 4438dce7bec..a9cb5e0f5c6 100644
> --- a/target/arm/trace-events
> +++ b/target/arm/trace-events
> @@ -13,3 +13,6 @@ arm_gt_update_irq(int timer, int irqstate) "gt_update_irq: timer %d irqstate %d"
>   
>   # kvm.c
>   kvm_arm_fixup_msi_route(uint64_t iova, uint64_t gpa) "MSI iova = 0x%"PRIx64" is translated into 0x%"PRIx64
> +
> +# tcg/psci.c and hvf/hvf.c
> +arm_psci_call(uint64_t x0, uint64_t x1, uint64_t x2, uint64_t x3, uint32_t cpuid) "PSCI Call x0=0x%016"PRIx64" x1=0x%016"PRIx64" x2=0x%016"PRIx64" x3=0x%016"PRIx64" cpuid=0x%x"

Re: [PATCH 6/6] target/arm: Allow arm_cpu_tlb_fill_align optionally set CPUTLBEntryFull

[Richard Henderson]

On 6/30/25 07:09, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/tcg/tlb_helper.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/tcg/tlb_helper.c b/target/arm/tcg/tlb_helper.c
> index 23c72a99f5c..df04ef351d1 100644
> --- a/target/arm/tcg/tlb_helper.c
> +++ b/target/arm/tcg/tlb_helper.c
> @@ -349,7 +349,9 @@ bool arm_cpu_tlb_fill_align(CPUState *cs, CPUTLBEntryFull *out, vaddr address,
>                                 &res, fi)) {
>           res.f.extra.arm.pte_attrs = res.cacheattrs.attrs;
>           res.f.extra.arm.shareability = res.cacheattrs.shareability;
> -        *out = res.f;
> +        if (out) {
> +            *out = res.f;
> +        }
>           return true;
>       }
>       if (probe) {

Why?  There's no other way to get the phys addr result.
Are you only calling this for the raise-exception side effect?


r~

Re: [PATCH 6/6] target/arm: Allow arm_cpu_tlb_fill_align optionally set CPUTLBEntryFull

[Philippe Mathieu-Daudé]

On 30/6/25 15:56, Richard Henderson wrote:
> On 6/30/25 07:09, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/arm/tcg/tlb_helper.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/tcg/tlb_helper.c b/target/arm/tcg/tlb_helper.c
>> index 23c72a99f5c..df04ef351d1 100644
>> --- a/target/arm/tcg/tlb_helper.c
>> +++ b/target/arm/tcg/tlb_helper.c
>> @@ -349,7 +349,9 @@ bool arm_cpu_tlb_fill_align(CPUState *cs, 
>> CPUTLBEntryFull *out, vaddr address,
>>                                 &res, fi)) {
>>           res.f.extra.arm.pte_attrs = res.cacheattrs.attrs;
>>           res.f.extra.arm.shareability = res.cacheattrs.shareability;
>> -        *out = res.f;
>> +        if (out) {
>> +            *out = res.f;
>> +        }
>>           return true;
>>       }
>>       if (probe) {
> 
> Why?  There's no other way to get the phys addr result.
> Are you only calling this for the raise-exception side effect?

Yes:

-- >8 --
@@ -2121,6 +2121,25 @@ int hvf_vcpu_exec(CPUState *cpu)
              hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized(), 1);
          }
          break;
+    case EC_INSNABORT: {
+        uint32_t set = (syndrome >> 12) & 3;
+        bool fnv = (syndrome >> 10) & 1;
+        bool ea = (syndrome >> 9) & 1;
+        bool s1ptw = (syndrome >> 7) & 1;
+        uint32_t ifsc = (syndrome >> 0) & 0x3f;
+
+        trace_hvf_insn_abort(env->pc, set, fnv, ea, s1ptw, ifsc);
+
+        cpu_synchronize_state(cpu);
+        if (tcg_enabled()) {
+            ret = EXCP_EMULATE;
+            arm_cpu_tlb_fill_align(cpu, NULL, env->pc, MMU_INST_FETCH,
+                                   arm_env_mmu_index(env), MO_32, 4, 
false, -1);
+        }
+        break;
+    }
      default:
          cpu_synchronize_state(cpu);
          trace_hvf_exit(syndrome, ec, env->pc);
---

I see probe_access_full_mmu() uses discard_tlb, I can use a similar
stack variable if you rather.

Re: [PATCH 5/6] target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF

[Philippe Mathieu-Daudé]

On 30/6/25 15:53, Richard Henderson wrote:
> On 6/30/25 07:09, Philippe Mathieu-Daudé wrote:
>> It is useful to compare PSCI calls of the same guest running
>> under TCG or HVF.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/arm/hvf/hvf.c    | 3 ++-
>>   target/arm/tcg/psci.c   | 3 +++
>>   target/arm/trace-events | 3 +++
>>   3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>> index 7a99118c8c2..6309c5b872e 100644
>> --- a/target/arm/hvf/hvf.c
>> +++ b/target/arm/hvf/hvf.c
>> @@ -34,6 +34,7 @@
>>   #include "target/arm/multiprocessing.h"
>>   #include "target/arm/gtimer.h"
>>   #include "trace.h"
>> +#include "../trace.h"
>>   #include "migration/vmstate.h"
>>   #include "gdbstub/enums.h"
>> @@ -1149,7 +1150,7 @@ static bool hvf_handle_psci_call(CPUState *cpu)
>>       int target_el = 1;
>>       int32_t ret = 0;
>> -    trace_hvf_psci_call(param[0], param[1], param[2], param[3],
>> +    trace_arm_psci_call(param[0], param[1], param[2], param[3],
>>                           arm_cpu_mp_affinity(arm_cpu));
> 
> Lacks removal of the hvf trace?

Oops indeed...

Re: [PATCH 1/6] target/arm: Only set CPU_INTERRUPT_EXITTB for TCG

[Pierrick Bouvier]

On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> Commit 34c45d53026 ("target-arm: kvm - re-inject guest debug
> exceptions") removed CPU_INTERRUPT_EXITTB from KVM, but it
> also appears on HVF. Better to restrict it to TCG.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 2/6] target/arm: Only allow disabling NEON when using TCG

[Pierrick Bouvier]

On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> Only allow disabling NEON when using TCG.
> 
> This avoids confusing user experience:
> 
>    $ qemu-system-aarch64 -M virt -accel hvf \
>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>    qemu-system-aarch64: AArch64 CPUs must have both VFP and Neon or neither
> 
>    $ qemu-system-aarch64 -M virt -accel hvf \
>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>    qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
> 
>    $ qemu-system-aarch64 -M virt -accel hvf \
>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>    qemu-system-aarch64: can't apply global host-arm-cpu.vfp-d32=off: Property 'host-arm-cpu.vfp-d32' not found
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 4/6] target/arm: Re-use arm_is_psci_call() in HVF

[Pierrick Bouvier]

On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> Re-use arm_is_psci_call() instead of open-coding it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/hvf/hvf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 5/6] target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF

[Pierrick Bouvier]

On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> It is useful to compare PSCI calls of the same guest running
> under TCG or HVF.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/hvf/hvf.c    | 3 ++-
>   target/arm/tcg/psci.c   | 3 +++
>   target/arm/trace-events | 3 +++
>   3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 7a99118c8c2..6309c5b872e 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -34,6 +34,7 @@
>   #include "target/arm/multiprocessing.h"
>   #include "target/arm/gtimer.h"
>   #include "trace.h"
> +#include "../trace.h"
>   #include "migration/vmstate.h"
>   
>   #include "gdbstub/enums.h"
> @@ -1149,7 +1150,7 @@ static bool hvf_handle_psci_call(CPUState *cpu)
>       int target_el = 1;
>       int32_t ret = 0;
>   
> -    trace_hvf_psci_call(param[0], param[1], param[2], param[3],
> +    trace_arm_psci_call(param[0], param[1], param[2], param[3],
>                           arm_cpu_mp_affinity(arm_cpu));
>   
>       switch (param[0]) {
> diff --git a/target/arm/tcg/psci.c b/target/arm/tcg/psci.c
> index cabed43e8a8..8df27ca123e 100644
> --- a/target/arm/tcg/psci.c
> +++ b/target/arm/tcg/psci.c
> @@ -25,6 +25,7 @@
>   #include "internals.h"
>   #include "arm-powerctl.h"
>   #include "target/arm/multiprocessing.h"
> +#include "../trace.h"
>   
>   bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>   {
> @@ -79,6 +80,8 @@ void arm_handle_psci_call(ARMCPU *cpu)
>            */
>           param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];
>       }
> +    trace_arm_psci_call(param[0], param[1], param[2], param[3],
> +                        arm_cpu_mp_affinity(cpu));
>   
>       if ((param[0] & QEMU_PSCI_0_2_64BIT) && !is_a64(env)) {
>           ret = QEMU_PSCI_RET_NOT_SUPPORTED;
> diff --git a/target/arm/trace-events b/target/arm/trace-events
> index 4438dce7bec..a9cb5e0f5c6 100644
> --- a/target/arm/trace-events
> +++ b/target/arm/trace-events
> @@ -13,3 +13,6 @@ arm_gt_update_irq(int timer, int irqstate) "gt_update_irq: timer %d irqstate %d"
>   
>   # kvm.c
>   kvm_arm_fixup_msi_route(uint64_t iova, uint64_t gpa) "MSI iova = 0x%"PRIx64" is translated into 0x%"PRIx64
> +
> +# tcg/psci.c and hvf/hvf.c
> +arm_psci_call(uint64_t x0, uint64_t x1, uint64_t x2, uint64_t x3, uint32_t cpuid) "PSCI Call x0=0x%016"PRIx64" x1=0x%016"PRIx64" x2=0x%016"PRIx64" x3=0x%016"PRIx64" cpuid=0x%x"

Just a nit, using 'target/arm/trace.h' might be more readable than 
'../trace.h'.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 6/6] target/arm: Allow arm_cpu_tlb_fill_align optionally set CPUTLBEntryFull

[Pierrick Bouvier]

On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/tcg/tlb_helper.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/tcg/tlb_helper.c b/target/arm/tcg/tlb_helper.c
> index 23c72a99f5c..df04ef351d1 100644
> --- a/target/arm/tcg/tlb_helper.c
> +++ b/target/arm/tcg/tlb_helper.c
> @@ -349,7 +349,9 @@ bool arm_cpu_tlb_fill_align(CPUState *cs, CPUTLBEntryFull *out, vaddr address,
>                                 &res, fi)) {
>           res.f.extra.arm.pte_attrs = res.cacheattrs.attrs;
>           res.f.extra.arm.shareability = res.cacheattrs.shareability;
> -        *out = res.f;
> +        if (out) {
> +            *out = res.f;
> +        }
>           return true;
>       }
>       if (probe) {

Would that be possible to provide more context about why it's needed?

The goal of tlb_fill_align is precisely to return a new CPUTLBEntryFull, 
while checking all the protection to access this page and generating a 
page fault if needed.

In case you just want to check if an address is valid, 
arm_cpu_get_phys_page_attrs_debug might be a better choice (checking for 
return value -1 in case of error).

Thanks,
Pierrick

Re: [PATCH 3/6] target/arm: Better describe PMU depends on TCG or HVF

[Pierrick Bouvier]

On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> Simplify PMU logic by rewriting '!KVM' as 'TCG || HVF'
> (ignoring QTest, because vCPUs are not available there).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu.c     |  2 +-
>   target/arm/machine.c | 10 +++++-----
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b6a8ba83a46..0311ff315fe 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2352,7 +2352,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>       if (arm_feature(env, ARM_FEATURE_PMU)) {
>           pmu_init(cpu);
>   
> -        if (!kvm_enabled()) {
> +        if (tcg_enabled() || hvf_enabled()) {
>               arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
>               arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
>           }
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index e442d485241..baa7ad25ca9 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -1,7 +1,7 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "qemu/error-report.h"
> -#include "system/kvm.h"
> +#include "system/hvf.h"
>   #include "system/tcg.h"
>   #include "kvm_arm.h"
>   #include "internals.h"
> @@ -853,7 +853,7 @@ static int cpu_pre_save(void *opaque)
>   {
>       ARMCPU *cpu = opaque;
>   
> -    if (!kvm_enabled()) {
> +    if (tcg_enabled() || hvf_enabled()) {
>           pmu_op_start(&cpu->env);
>       }
>   
> @@ -888,7 +888,7 @@ static int cpu_post_save(void *opaque)
>   {
>       ARMCPU *cpu = opaque;
>   
> -    if (!kvm_enabled()) {
> +    if (tcg_enabled() || hvf_enabled()) {
>           pmu_op_finish(&cpu->env);
>       }
>   
> @@ -921,7 +921,7 @@ static int cpu_pre_load(void *opaque)
>        */
>       env->irq_line_state = UINT32_MAX;
>   
> -    if (!kvm_enabled()) {
> +    if (tcg_enabled() || hvf_enabled()) {
>           pmu_op_start(env);
>       }
>   
> @@ -1013,7 +1013,7 @@ static int cpu_post_load(void *opaque, int version_id)
>           }
>       }
>   
> -    if (!kvm_enabled()) {
> +    if (tcg_enabled() || hvf_enabled()) {
>           pmu_op_finish(env);
>       }
>   

If I understand correctly, this is supported for tcg and hvf, but not 
kvm, right?
I'm just a bit confused by commit description mentioning "simplify 
logic", which is more "make it explicit".

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Thanks,
Pierrick

Re: [PATCH 3/6] target/arm: Better describe PMU depends on TCG or HVF

[Pierrick Bouvier]

On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> Simplify PMU logic by rewriting '!KVM' as 'TCG || HVF'
> (ignoring QTest, because vCPUs are not available there).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu.c     |  2 +-
>   target/arm/machine.c | 10 +++++-----
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b6a8ba83a46..0311ff315fe 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2352,7 +2352,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>       if (arm_feature(env, ARM_FEATURE_PMU)) {
>           pmu_init(cpu);
>   
> -        if (!kvm_enabled()) {
> +        if (tcg_enabled() || hvf_enabled()) {
>               arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
>               arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
>           }
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index e442d485241..baa7ad25ca9 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -1,7 +1,7 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "qemu/error-report.h"
> -#include "system/kvm.h"
> +#include "system/hvf.h"
>   #include "system/tcg.h"
>   #include "kvm_arm.h"
>   #include "internals.h"
> @@ -853,7 +853,7 @@ static int cpu_pre_save(void *opaque)
>   {
>       ARMCPU *cpu = opaque;
>   
> -    if (!kvm_enabled()) {
> +    if (tcg_enabled() || hvf_enabled()) {
>           pmu_op_start(&cpu->env);
>       }
>   
> @@ -888,7 +888,7 @@ static int cpu_post_save(void *opaque)
>   {
>       ARMCPU *cpu = opaque;
>   
> -    if (!kvm_enabled()) {
> +    if (tcg_enabled() || hvf_enabled()) {
>           pmu_op_finish(&cpu->env);
>       }
>   
> @@ -921,7 +921,7 @@ static int cpu_pre_load(void *opaque)
>        */
>       env->irq_line_state = UINT32_MAX;
>   
> -    if (!kvm_enabled()) {
> +    if (tcg_enabled() || hvf_enabled()) {
>           pmu_op_start(env);
>       }
>   
> @@ -1013,7 +1013,7 @@ static int cpu_post_load(void *opaque, int version_id)
>           }
>       }
>   
> -    if (!kvm_enabled()) {
> +    if (tcg_enabled() || hvf_enabled()) {
>           pmu_op_finish(env);
>       }
>   

If I understand correctly, this is supported for tcg and hvf, but not 
kvm, right?
I'm just a bit confused by commit description mentioning "simplify 
logic", which is more "make it explicit".

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Thanks,
Pierrick

Re: [PATCH 2/6] target/arm: Only allow disabling NEON when using TCG

[Peter Maydell]

On Mon, 30 Jun 2025 at 14:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Only allow disabling NEON when using TCG.
>
> This avoids confusing user experience:
>
>   $ qemu-system-aarch64 -M virt -accel hvf \
>                         -cpu host,neon=off,vfp=off,vfp-d32=off
>   qemu-system-aarch64: AArch64 CPUs must have both VFP and Neon or neither
>
>   $ qemu-system-aarch64 -M virt -accel hvf \
>                         -cpu host,neon=off,vfp=off,vfp-d32=off
>   qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
>
>   $ qemu-system-aarch64 -M virt -accel hvf \
>                         -cpu host,neon=off,vfp=off,vfp-d32=off
>   qemu-system-aarch64: can't apply global host-arm-cpu.vfp-d32=off: Property 'host-arm-cpu.vfp-d32' not found

I don't know about users, but I'm definitely confused.
Aren't these three all the same command line ? What's
the problem it's trying to show ?

-- PMM

Re: [PATCH 5/6] target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF

[Peter Maydell]

On Mon, 30 Jun 2025 at 17:53, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> > It is useful to compare PSCI calls of the same guest running
> > under TCG or HVF.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   target/arm/hvf/hvf.c    | 3 ++-
> >   target/arm/tcg/psci.c   | 3 +++
> >   target/arm/trace-events | 3 +++
> >   3 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> > index 7a99118c8c2..6309c5b872e 100644
> > --- a/target/arm/hvf/hvf.c
> > +++ b/target/arm/hvf/hvf.c
> > @@ -34,6 +34,7 @@
> >   #include "target/arm/multiprocessing.h"
> >   #include "target/arm/gtimer.h"
> >   #include "trace.h"
> > +#include "../trace.h"


> Just a nit, using 'target/arm/trace.h' might be more readable than
> '../trace.h'.

Mmm. docs/devel/tracing.rst rather discourages this:

# While it is possible to include a trace.h file from outside a source
file's own
# sub-directory, this is discouraged in general. It is strongly preferred that
# all events be declared directly in the sub-directory that uses them. The only
# exception is where there are some shared trace events defined in the top level
# directory trace-events file.

I don't know if we want to loosen that to permit events
that are shared between multiple subdirs (cc'ing the
trace subsystem maintainers for their view).

git grep 'include.*trace.h' | grep -v '"trace.h"'| grep -v 'trace.h:'|less

suggests that the only current place where we're including
a trace.h not in the same directory is linux-user, where
we opt to use the full linux-user/trace.h path. So probably
for consistency we should use target/arm/trace.h here.

(That grep also shows up that hw/uefi is missing its
trace.h header and the .c files are including
trace-hw_uefi.h directly...)

-- PMM

Re: [PATCH 2/6] target/arm: Only allow disabling NEON when using TCG

[Philippe Mathieu-Daudé]

On 4/7/25 15:03, Peter Maydell wrote:
> On Mon, 30 Jun 2025 at 14:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Only allow disabling NEON when using TCG.
>>
>> This avoids confusing user experience:
>>
>>    $ qemu-system-aarch64 -M virt -accel hvf \
>>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>>    qemu-system-aarch64: AArch64 CPUs must have both VFP and Neon or neither
>>
>>    $ qemu-system-aarch64 -M virt -accel hvf \
>>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>>    qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
>>
>>    $ qemu-system-aarch64 -M virt -accel hvf \
>>                          -cpu host,neon=off,vfp=off,vfp-d32=off
>>    qemu-system-aarch64: can't apply global host-arm-cpu.vfp-d32=off: Property 'host-arm-cpu.vfp-d32' not found
> 
> I don't know about users, but I'm definitely confused.
> Aren't these three all the same command line ? What's
> the problem it's trying to show ?

I should have only shared this confusing example:

$ qemu-system-aarch64 -M virt -accel hvf -cpu host,vfp-d32=off
qemu-system-aarch64: can't apply global host-arm-cpu.vfp-d32=off: 
Property 'host-arm-cpu.vfp-d32' not found

Re: [PATCH 2/6] target/arm: Only allow disabling NEON when using TCG

[Peter Maydell]

On Fri, 4 Jul 2025 at 14:17, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 4/7/25 15:03, Peter Maydell wrote:
> > On Mon, 30 Jun 2025 at 14:09, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> Only allow disabling NEON when using TCG.
> >>
> >> This avoids confusing user experience:
> >>
> >>    $ qemu-system-aarch64 -M virt -accel hvf \
> >>                          -cpu host,neon=off,vfp=off,vfp-d32=off
> >>    qemu-system-aarch64: AArch64 CPUs must have both VFP and Neon or neither
> >>
> >>    $ qemu-system-aarch64 -M virt -accel hvf \
> >>                          -cpu host,neon=off,vfp=off,vfp-d32=off
> >>    qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
> >>
> >>    $ qemu-system-aarch64 -M virt -accel hvf \
> >>                          -cpu host,neon=off,vfp=off,vfp-d32=off
> >>    qemu-system-aarch64: can't apply global host-arm-cpu.vfp-d32=off: Property 'host-arm-cpu.vfp-d32' not found
> >
> > I don't know about users, but I'm definitely confused.
> > Aren't these three all the same command line ? What's
> > the problem it's trying to show ?
>
> I should have only shared this confusing example:
>
> $ qemu-system-aarch64 -M virt -accel hvf -cpu host,vfp-d32=off
> qemu-system-aarch64: can't apply global host-arm-cpu.vfp-d32=off:
> Property 'host-arm-cpu.vfp-d32' not found

That seems correct, though ? We say
     * Allow user to turn off VFP and Neon support, but only for TCG --
     * KVM does not currently allow us to lie to the guest about its
     * ID/feature registers, so the guest always sees what the host has.
and hvf is currently the same. The bug the patch is fixing
looks like it is that -accel hvf lets us say '-cpu host,neon=off'
but it won't actually do what the user might be intending. So
we should avoid creating the property, as we already do for KVM.

(The other odd thing here is that hvf CPUs don't support
AArch32, but ARM_FEATURE_NEON is about AArch32. But that's
a separate thing and we'd need to cross-check that nothing
aarch64 is incorrectly looking at that feature flag before
we suppressed it for CPUs without any AArch32 support.)

-- PMM

Re: [PATCH 5/6] target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2549 bytes --]

On Fri, Jul 04, 2025 at 02:14:35PM +0100, Peter Maydell wrote:
> On Mon, 30 Jun 2025 at 17:53, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> >
> > On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> > > It is useful to compare PSCI calls of the same guest running
> > > under TCG or HVF.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   target/arm/hvf/hvf.c    | 3 ++-
> > >   target/arm/tcg/psci.c   | 3 +++
> > >   target/arm/trace-events | 3 +++
> > >   3 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> > > index 7a99118c8c2..6309c5b872e 100644
> > > --- a/target/arm/hvf/hvf.c
> > > +++ b/target/arm/hvf/hvf.c
> > > @@ -34,6 +34,7 @@
> > >   #include "target/arm/multiprocessing.h"
> > >   #include "target/arm/gtimer.h"
> > >   #include "trace.h"
> > > +#include "../trace.h"
> 
> 
> > Just a nit, using 'target/arm/trace.h' might be more readable than
> > '../trace.h'.
> 
> Mmm. docs/devel/tracing.rst rather discourages this:
> 
> # While it is possible to include a trace.h file from outside a source
> file's own
> # sub-directory, this is discouraged in general. It is strongly preferred that
> # all events be declared directly in the sub-directory that uses them. The only
> # exception is where there are some shared trace events defined in the top level
> # directory trace-events file.
> 
> I don't know if we want to loosen that to permit events
> that are shared between multiple subdirs (cc'ing the
> trace subsystem maintainers for their view).

Code is generated from the trace-events files and my main concern is
that the build dependencies on the generated code may not be obvious if
a trace event from somewhere far away in the source tree hierarchy is
used. You might hit linker errors because the .o files needed for the
trace events are not being linked in.

I would try to stick with what's described in tracing.rst to avoid
difficulties now or in the future.

> 
> git grep 'include.*trace.h' | grep -v '"trace.h"'| grep -v 'trace.h:'|less
> 
> suggests that the only current place where we're including
> a trace.h not in the same directory is linux-user, where
> we opt to use the full linux-user/trace.h path. So probably
> for consistency we should use target/arm/trace.h here.
> 
> (That grep also shows up that hw/uefi is missing its
> trace.h header and the .c files are including
> trace-hw_uefi.h directly...)
> 
> -- PMM
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/6] target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF

[Daniel P. Berrangé]

On Fri, Jul 04, 2025 at 02:14:35PM +0100, Peter Maydell wrote:
> On Mon, 30 Jun 2025 at 17:53, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> >
> > On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> > > It is useful to compare PSCI calls of the same guest running
> > > under TCG or HVF.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   target/arm/hvf/hvf.c    | 3 ++-
> > >   target/arm/tcg/psci.c   | 3 +++
> > >   target/arm/trace-events | 3 +++
> > >   3 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> > > index 7a99118c8c2..6309c5b872e 100644
> > > --- a/target/arm/hvf/hvf.c
> > > +++ b/target/arm/hvf/hvf.c
> > > @@ -34,6 +34,7 @@
> > >   #include "target/arm/multiprocessing.h"
> > >   #include "target/arm/gtimer.h"
> > >   #include "trace.h"
> > > +#include "../trace.h"
> 
> 
> > Just a nit, using 'target/arm/trace.h' might be more readable than
> > '../trace.h'.
> 
> Mmm. docs/devel/tracing.rst rather discourages this:
> 
> # While it is possible to include a trace.h file from outside a source
> file's own
> # sub-directory, this is discouraged in general. It is strongly preferred that
> # all events be declared directly in the sub-directory that uses them. The only
> # exception is where there are some shared trace events defined in the top level
> # directory trace-events file.
> 
> I don't know if we want to loosen that to permit events
> that are shared between multiple subdirs (cc'ing the
> trace subsystem maintainers for their view).
> 
> git grep 'include.*trace.h' | grep -v '"trace.h"'| grep -v 'trace.h:'|less
> 
> suggests that the only current place where we're including
> a trace.h not in the same directory is linux-user, where
> we opt to use the full linux-user/trace.h path. So probably
> for consistency we should use target/arm/trace.h here.

IMHO using the up-level relative paths is desirable, as it reinforces the
intent that we shouldn't be pulling in trace events from arbitrary different
sub-trees of the codebase, only the current dir & its (near) parents.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/6] target/arm: Share ARM_PSCI_CALL trace event between TCG and HVF

[Peter Maydell]

On Mon, 7 Jul 2025 at 15:03, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Fri, Jul 04, 2025 at 02:14:35PM +0100, Peter Maydell wrote:
> > On Mon, 30 Jun 2025 at 17:53, Pierrick Bouvier
> > <pierrick.bouvier@linaro.org> wrote:
> > >
> > > On 6/30/25 6:09 AM, Philippe Mathieu-Daudé wrote:
> > > > It is useful to compare PSCI calls of the same guest running
> > > > under TCG or HVF.
> > > >
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > ---
> > > >   target/arm/hvf/hvf.c    | 3 ++-
> > > >   target/arm/tcg/psci.c   | 3 +++
> > > >   target/arm/trace-events | 3 +++
> > > >   3 files changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> > > > index 7a99118c8c2..6309c5b872e 100644
> > > > --- a/target/arm/hvf/hvf.c
> > > > +++ b/target/arm/hvf/hvf.c
> > > > @@ -34,6 +34,7 @@
> > > >   #include "target/arm/multiprocessing.h"
> > > >   #include "target/arm/gtimer.h"
> > > >   #include "trace.h"
> > > > +#include "../trace.h"
> >
> >
> > > Just a nit, using 'target/arm/trace.h' might be more readable than
> > > '../trace.h'.
> >
> > Mmm. docs/devel/tracing.rst rather discourages this:
> >
> > # While it is possible to include a trace.h file from outside a source
> > file's own
> > # sub-directory, this is discouraged in general. It is strongly preferred that
> > # all events be declared directly in the sub-directory that uses them. The only
> > # exception is where there are some shared trace events defined in the top level
> > # directory trace-events file.
> >
> > I don't know if we want to loosen that to permit events
> > that are shared between multiple subdirs (cc'ing the
> > trace subsystem maintainers for their view).
>
> Code is generated from the trace-events files and my main concern is
> that the build dependencies on the generated code may not be obvious if
> a trace event from somewhere far away in the source tree hierarchy is
> used. You might hit linker errors because the .o files needed for the
> trace events are not being linked in.
>
> I would try to stick with what's described in tracing.rst to avoid
> difficulties now or in the future.

So how should we deal with "two different source files in
different subdirectories both want to emit trace event X" ?
Creating a utility function that can live in the parent dir
just to wrap the trace event seems a bit inefficient.

I agree that using a trace event from a long way away across
the source tree is probably a bad idea, but "immediate parent
directory" doesn't seem too likely to cause issues ?

-- PMM