Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field

[Gavin Shan <gshan@redhat.com>]

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@redhat.com>
>> ---
>>  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.

>>          __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. 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.

Regards,
Gavin