All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
@ 2019-12-12  2:45 Gavin Shan
  2019-12-12  9:23 ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Gavin Shan @ 2019-12-12  2:45 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, paulus, maz, jhogan, drjones, vkuznets, gshan

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	)
 		__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)
 );
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
  2019-12-12  2:45 [PATCH 3/3] kvm/arm: Standardize kvm exit reason field Gavin Shan
@ 2019-12-12  9:23 ` Marc Zyngier
  2019-12-13  0:50   ` Gavin Shan
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2019-12-12  9:23 UTC (permalink / raw)
  To: Gavin Shan; +Cc: kvm, pbonzini, rkrcmar, paulus, jhogan, drjones, vkuznets

Hi Gavin,

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

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

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
  2019-12-12  9:23 ` Marc Zyngier
@ 2019-12-13  0:50   ` Gavin Shan
  2019-12-13  9:47     ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Gavin Shan @ 2019-12-13  0:50 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvm, pbonzini, rkrcmar, paulus, jhogan, drjones, vkuznets

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
  2019-12-13  0:50   ` Gavin Shan
@ 2019-12-13  9:47     ` Marc Zyngier
  2019-12-13 23:14       ` Gavin Shan
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2019-12-13  9:47 UTC (permalink / raw)
  To: Gavin Shan; +Cc: kvm, pbonzini, rkrcmar, paulus, jhogan, drjones, vkuznets

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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
  2019-12-13  9:47     ` Marc Zyngier
@ 2019-12-13 23:14       ` Gavin Shan
  2019-12-16  9:14         ` Vitaly Kuznetsov
  0 siblings, 1 reply; 9+ messages in thread
From: Gavin Shan @ 2019-12-13 23:14 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvm, pbonzini, rkrcmar, paulus, jhogan, drjones, vkuznets

On 12/13/19 8:47 PM, Marc Zyngier wrote:
> 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@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.
> 
> 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.
> 

Agree.

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

Ah, it's not good :)

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

Well, I would like to receive Vitaly's comments here. Vitaly, it seems it's
more realistic to fix the issue from kvm_stat side according to the comments
given by Marc?

Thanks,
Gavin


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
  2019-12-13 23:14       ` Gavin Shan
@ 2019-12-16  9:14         ` Vitaly Kuznetsov
  2019-12-16  9:35           ` Marc Zyngier
  2019-12-17 10:56           ` Paolo Bonzini
  0 siblings, 2 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2019-12-16  9:14 UTC (permalink / raw)
  To: Gavin Shan; +Cc: kvm, pbonzini, rkrcmar, paulus, jhogan, drjones, Marc Zyngier

Gavin Shan <gshan@redhat.com> writes:

> On 12/13/19 8:47 PM, Marc Zyngier wrote:
>> On 2019-12-13 00:50, Gavin Shan wrote:
>>>
>>> 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.
>> 
>
> Well, I would like to receive Vitaly's comments here. Vitaly, it seems it's
> more realistic to fix the issue from kvm_stat side according to the comments
> given by Marc?
>

Sure, if we decide to treat tracepoints as ABI then fixing users is
likely the way to go. Personally, I think that we should have certain
freedom with them and consider only tools which live in linux.git when
making changes (and changing the tool to match in the same patch series
is OK from this PoV, no need to support all possible versions of the
tool). 

Also, we can be a bit more conservative and in this particular case
instead of renaming fields just add 'exit_reason' to all architectures
where it's missing. For ARM, 'esr_ec' will then stay with what it is and
'exit_reason' may contain something different (like the information why
the guest exited actually). But I don't know much about ARM specifics
and I'm not sure how feasible the suggestion would be.

-- 
Vitaly


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
  2019-12-16  9:14         ` Vitaly Kuznetsov
@ 2019-12-16  9:35           ` Marc Zyngier
  2019-12-17  2:03             ` Gavin Shan
  2019-12-17 10:56           ` Paolo Bonzini
  1 sibling, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2019-12-16  9:35 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Gavin Shan, kvm, pbonzini, rkrcmar, paulus, jhogan, drjones

On 2019-12-16 09:14, Vitaly Kuznetsov wrote:
> Gavin Shan <gshan@redhat.com> writes:
>
>> On 12/13/19 8:47 PM, Marc Zyngier wrote:
>>> On 2019-12-13 00:50, Gavin Shan wrote:
>>>>
>>>> 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.
>>>
>>
>> Well, I would like to receive Vitaly's comments here. Vitaly, it 
>> seems it's
>> more realistic to fix the issue from kvm_stat side according to the 
>> comments
>> given by Marc?
>>
>
> Sure, if we decide to treat tracepoints as ABI then fixing users is
> likely the way to go. Personally, I think that we should have certain
> freedom with them and consider only tools which live in linux.git 
> when
> making changes (and changing the tool to match in the same patch 
> series
> is OK from this PoV, no need to support all possible versions of the
> tool).

So far, the approach has been a pretty conservative one, and there was
countless discussions (including a pretty heated one at KS two years 
ago,
see [1] which did set the tone for the whole of the discussion).

So as far as I have something to say about this, we're not renaming 
fields
in existing tracepoints.

> Also, we can be a bit more conservative and in this particular case
> instead of renaming fields just add 'exit_reason' to all 
> architectures
> where it's missing. For ARM, 'esr_ec' will then stay with what it is 
> and
> 'exit_reason' may contain something different (like the information 
> why
> the guest exited actually). But I don't know much about ARM specifics
> and I'm not sure how feasible the suggestion would be.

It should be possible to /extend/ tracepoints without breaking 
compatibility,
and I don't have any issue with that. This could either report the 
unmodified
'ret' value, or something more synthetic. It really depends on what you 
want
this information for.

         M.

[1] https://lwn.net/Articles/737532/
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
  2019-12-16  9:35           ` Marc Zyngier
@ 2019-12-17  2:03             ` Gavin Shan
  0 siblings, 0 replies; 9+ messages in thread
From: Gavin Shan @ 2019-12-17  2:03 UTC (permalink / raw)
  To: Marc Zyngier, Vitaly Kuznetsov
  Cc: kvm, pbonzini, rkrcmar, paulus, jhogan, drjones

On 12/16/19 8:35 PM, Marc Zyngier wrote:
> On 2019-12-16 09:14, Vitaly Kuznetsov wrote:
>> Gavin Shan <gshan@redhat.com> writes:
>>
>>> On 12/13/19 8:47 PM, Marc Zyngier wrote:
>>>> On 2019-12-13 00:50, Gavin Shan wrote:
>>>>>
>>>>> 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.
>>>>
>>>
>>> Well, I would like to receive Vitaly's comments here. Vitaly, it seems it's
>>> more realistic to fix the issue from kvm_stat side according to the comments
>>> given by Marc?
>>>
>>
>> Sure, if we decide to treat tracepoints as ABI then fixing users is
>> likely the way to go. Personally, I think that we should have certain
>> freedom with them and consider only tools which live in linux.git when
>> making changes (and changing the tool to match in the same patch series
>> is OK from this PoV, no need to support all possible versions of the
>> tool).
> 
> So far, the approach has been a pretty conservative one, and there was
> countless discussions (including a pretty heated one at KS two years ago,
> see [1] which did set the tone for the whole of the discussion).
> 
> So as far as I have something to say about this, we're not renaming fields
> in existing tracepoints.
> 
>> Also, we can be a bit more conservative and in this particular case
>> instead of renaming fields just add 'exit_reason' to all architectures
>> where it's missing. For ARM, 'esr_ec' will then stay with what it is and
>> 'exit_reason' may contain something different (like the information why
>> the guest exited actually). But I don't know much about ARM specifics
>> and I'm not sure how feasible the suggestion would be.
> 
> It should be possible to /extend/ tracepoints without breaking compatibility,
> and I don't have any issue with that. This could either report the unmodified
> 'ret' value, or something more synthetic. It really depends on what you want
> this information for.
> 
>          M.
> 
> [1] https://lwn.net/Articles/737532/
>

The extension means to add a new field "exit_reason" for aarch64. We can't
delete or rename the existing fields in order to keep the compatibility.
Also, the newly added field ("exit_reason") will carry duplicated information
that have been carried by "ret" and "esr_ec". I guess it's going to make the
interface ugly. So I personally prefer to fix the issue from "kvm_stat" and
I'm going to post a v2 patch for this shortly.

Thanks again to Vitaly and Marc for your comments :)

Regards,
Gavin


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] kvm/arm: Standardize kvm exit reason field
  2019-12-16  9:14         ` Vitaly Kuznetsov
  2019-12-16  9:35           ` Marc Zyngier
@ 2019-12-17 10:56           ` Paolo Bonzini
  1 sibling, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2019-12-17 10:56 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Gavin Shan
  Cc: kvm, rkrcmar, paulus, jhogan, drjones, Marc Zyngier

On 16/12/19 10:14, Vitaly Kuznetsov wrote:
>> Well, I would like to receive Vitaly's comments here. Vitaly, it seems it's
>> more realistic to fix the issue from kvm_stat side according to the comments
>> given by Marc?
>>
> Sure, if we decide to treat tracepoints as ABI then fixing users is
> likely the way to go. Personally, I think that we should have certain
> freedom with them and consider only tools which live in linux.git when
> making changes (and changing the tool to match in the same patch series
> is OK from this PoV, no need to support all possible versions of the
> tool). 

I'd agree with that, plus trace-cmd since it has a KVM plugin,
especially since KVM has for example debugfs stats unlike other
subsystems (and we're tracing debugfs stats as stable userspace API, and
planning to move them somewhere out of debugfs).  But I wouldn't
complain either if architecture maintainers feel differently.

Paolo

> Also, we can be a bit more conservative and in this particular case
> instead of renaming fields just add 'exit_reason' to all architectures
> where it's missing. For ARM, 'esr_ec' will then stay with what it is and
> 'exit_reason' may contain something different (like the information why
> the guest exited actually). But I don't know much about ARM specifics
> and I'm not sure how feasible the suggestion would be.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-12-17 10:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-12  2:45 [PATCH 3/3] kvm/arm: Standardize kvm exit reason field Gavin Shan
2019-12-12  9:23 ` Marc Zyngier
2019-12-13  0:50   ` Gavin Shan
2019-12-13  9:47     ` Marc Zyngier
2019-12-13 23:14       ` Gavin Shan
2019-12-16  9:14         ` Vitaly Kuznetsov
2019-12-16  9:35           ` Marc Zyngier
2019-12-17  2:03             ` Gavin Shan
2019-12-17 10:56           ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.