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
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>
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
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 >
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
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 :|
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"
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...
© 2016 - 2025 Red Hat, Inc.