Hi Gavin,
On 2019-12-13 00:50, Gavin Shan wrote:
On 12/12/19 8:23 PM, Marc Zyngier wrote:
On 2019-12-12 02:45, Gavin Shan wrote:
This standardizes kvm exit reason field name by replacing "esr_ec"
with "exit_reason".
Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
---
virt/kvm/arm/trace.h | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/virt/kvm/arm/trace.h b/virt/kvm/arm/trace.h
index 204d210d01c2..0ac774fd324d 100644
--- a/virt/kvm/arm/trace.h
+++ b/virt/kvm/arm/trace.h
@@ -27,25 +27,27 @@ TRACE_EVENT(kvm_entry,
);
TRACE_EVENT(kvm_exit,
- TP_PROTO(int ret, unsigned int esr_ec, unsigned long vcpu_pc),
- TP_ARGS(ret, esr_ec, vcpu_pc),
+ TP_PROTO(int ret, unsigned int exit_reason, unsigned long
vcpu_pc),
+ TP_ARGS(ret, exit_reason, vcpu_pc),
TP_STRUCT__entry(
__field( int, ret )
- __field( unsigned int, esr_ec )
+ __field( unsigned int, exit_reason )
I don't think the two are the same thing. The exit reason should be
exactly that: why has the guest exited (exception, host interrupt,
trap).
What we're reporting here is the exception class, which doesn't
apply to
interrupts, for example (hence the 0 down below, which we treat as a
catch-all).
Marc, thanks a lot for your reply. Yeah, the combination (ret and
esr_ec) is
complete to indicate the exit reasons if I'm understanding correctly.
The exit reasons seen by kvm_stat is exactly the ESR_EL1[EC]. It's
declared
by marcro AARCH64_EXIT_REASONS in tools/kvm/kvm_stat/kvm_stat. So
it's precise
and complete from perspective of kvm_stat.
For the patch itself, it standardizes the filter name by renaming
"esr_ec"
to "exit_reason", no functional changes introduced and I think it
would be
fine.
It may not be a functional change, but it is a semantic change. To me,
'exit_reason' means something, and esr_ec something entirely different.
But the real blocker is below.
__field( unsigned long, vcpu_pc )
),
TP_fast_assign(
__entry->ret = ARM_EXCEPTION_CODE(ret);
- __entry->esr_ec = ARM_EXCEPTION_IS_TRAP(ret) ? esr_ec : 0;
+ __entry->exit_reason =
+ ARM_EXCEPTION_IS_TRAP(ret) ? exit_reason: 0;
__entry->vcpu_pc = vcpu_pc;
),
TP_printk("%s: HSR_EC: 0x%04x (%s), PC: 0x%08lx",
__print_symbolic(__entry->ret, kvm_arm_exception_type),
- __entry->esr_ec,
- __print_symbolic(__entry->esr_ec,
kvm_arm_exception_class),
+ __entry->exit_reason,
+ __print_symbolic(__entry->exit_reason,
+ kvm_arm_exception_class),
__entry->vcpu_pc)
);
The last thing is whether such a change is an ABI change or not.
I've been very
reluctant to change any of this for that reason.
Yeah, I think it is ABI change unfortunately, but I'm not sure how
many applications are using this filter.
Nobody can tell. The problem is that someone will write a script that
parses this trace point based on an older kernel release (such as
what the distros are shipping today), and two years from now will
shout at you (and me) for having broken their toy.
However, the fixed filter name ("exit_reason") is beneficial in long
run.
The application needn't distinguish architects to provide different
tracepoint filters at least.
Well, you certainly need to understand the actual semantic behind the
fields if you want to draw any meaningful conclusion. Otherwise, all
you need is to measure the frequency of such event.
M.
--
Jazz is not dead. It just smells funny...
