[PATCH] tools/kvm_stat: Fix kvm_exit filter name

[PATCH] tools/kvm_stat: Fix kvm_exit filter name

[Gavin Shan]

The filter name is fixed to "exit_reason" for some kvm_exit events, no
matter what architect we have. Actually, the filter name ("exit_reason")
is only applicable to x86, meaning it's broken on other architects
including aarch64.

This fixes the issue by providing various kvm_exit filter names, depending
on architect we're on. Afterwards, the variable filter name is picked and
applied through ioctl(fd, SET_FILTER).

Reported-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 tools/kvm/kvm_stat/kvm_stat | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index ad1b9e646c49..f9273614b7e3 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -270,6 +270,7 @@ class ArchX86(Arch):
     def __init__(self, exit_reasons):
         self.sc_perf_evt_open = 298
         self.ioctl_numbers = IOCTL_NUMBERS
+        self.exit_field = 'exit_reason'
         self.exit_reasons = exit_reasons
 
     def debugfs_is_child(self, field):
@@ -289,6 +290,7 @@ class ArchPPC(Arch):
         # numbers depend on the wordsize.
         char_ptr_size = ctypes.sizeof(ctypes.c_char_p)
         self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16
+        self.exit_field = 'exit_nr'
         self.exit_reasons = {}
 
     def debugfs_is_child(self, field):
@@ -300,6 +302,7 @@ class ArchA64(Arch):
     def __init__(self):
         self.sc_perf_evt_open = 241
         self.ioctl_numbers = IOCTL_NUMBERS
+        self.exit_field = 'ret'
         self.exit_reasons = AARCH64_EXIT_REASONS
 
     def debugfs_is_child(self, field):
@@ -311,6 +314,7 @@ class ArchS390(Arch):
     def __init__(self):
         self.sc_perf_evt_open = 331
         self.ioctl_numbers = IOCTL_NUMBERS
+        self.exit_field = None
         self.exit_reasons = None
 
     def debugfs_is_child(self, field):
@@ -541,8 +545,8 @@ class TracepointProvider(Provider):
         """
         filters = {}
         filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS)
-        if ARCH.exit_reasons:
-            filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
+        if ARCH.exit_field and ARCH.exit_reasons:
+            filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons)
         return filters
 
     def _get_available_fields(self):
-- 
2.23.0

Re: [PATCH] tools/kvm_stat: Fix kvm_exit filter name

[Vitaly Kuznetsov]

Gavin Shan <gshan@redhat.com> writes:

> The filter name is fixed to "exit_reason" for some kvm_exit events, no
> matter what architect we have. Actually, the filter name ("exit_reason")
> is only applicable to x86, meaning it's broken on other architects
> including aarch64.
>
> This fixes the issue by providing various kvm_exit filter names, depending
> on architect we're on. Afterwards, the variable filter name is picked and
> applied through ioctl(fd, SET_FILTER).

Would it actually make sense to standardize (to certain extent) kvm_exit
tracepoints instead?

>
> Reported-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  tools/kvm/kvm_stat/kvm_stat | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
> index ad1b9e646c49..f9273614b7e3 100755
> --- a/tools/kvm/kvm_stat/kvm_stat
> +++ b/tools/kvm/kvm_stat/kvm_stat
> @@ -270,6 +270,7 @@ class ArchX86(Arch):
>      def __init__(self, exit_reasons):
>          self.sc_perf_evt_open = 298
>          self.ioctl_numbers = IOCTL_NUMBERS
> +        self.exit_field = 'exit_reason'

Also, 'exit_field' name is confusing (probably because I'm thinking of
VMCS fields)

>          self.exit_reasons = exit_reasons
>  
>      def debugfs_is_child(self, field):
> @@ -289,6 +290,7 @@ class ArchPPC(Arch):
>          # numbers depend on the wordsize.
>          char_ptr_size = ctypes.sizeof(ctypes.c_char_p)
>          self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16
> +        self.exit_field = 'exit_nr'
>          self.exit_reasons = {}
>  
>      def debugfs_is_child(self, field):
> @@ -300,6 +302,7 @@ class ArchA64(Arch):
>      def __init__(self):
>          self.sc_perf_evt_open = 241
>          self.ioctl_numbers = IOCTL_NUMBERS
> +        self.exit_field = 'ret'

And this is the most confusing part. Why do we have 'ret' as an exit
reason in the first place?

>          self.exit_reasons = AARCH64_EXIT_REASONS
>  
>      def debugfs_is_child(self, field):
> @@ -311,6 +314,7 @@ class ArchS390(Arch):
>      def __init__(self):
>          self.sc_perf_evt_open = 331
>          self.ioctl_numbers = IOCTL_NUMBERS
> +        self.exit_field = None
>          self.exit_reasons = None
>  
>      def debugfs_is_child(self, field):
> @@ -541,8 +545,8 @@ class TracepointProvider(Provider):
>          """
>          filters = {}
>          filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS)
> -        if ARCH.exit_reasons:
> -            filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
> +        if ARCH.exit_field and ARCH.exit_reasons:
> +            filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons)
>          return filters
>  
>      def _get_available_fields(self):

-- 
Vitaly

Re: [PATCH] tools/kvm_stat: Fix kvm_exit filter name

[Gavin Shan]

On 12/10/19 7:45 PM, Vitaly Kuznetsov wrote:
> Gavin Shan <gshan@redhat.com> writes:
> 
>> The filter name is fixed to "exit_reason" for some kvm_exit events, no
>> matter what architect we have. Actually, the filter name ("exit_reason")
>> is only applicable to x86, meaning it's broken on other architects
>> including aarch64.
>>
>> This fixes the issue by providing various kvm_exit filter names, depending
>> on architect we're on. Afterwards, the variable filter name is picked and
>> applied through ioctl(fd, SET_FILTER).
> 
> Would it actually make sense to standardize (to certain extent) kvm_exit
> tracepoints instead?
> 

Yes, It makes sense, but it's something for future if you agree. Besides,
It seems that other kvm tracepoints need standardization either.

>>
>> Reported-by: Andrew Jones <drjones@redhat.com>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   tools/kvm/kvm_stat/kvm_stat | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
>> index ad1b9e646c49..f9273614b7e3 100755
>> --- a/tools/kvm/kvm_stat/kvm_stat
>> +++ b/tools/kvm/kvm_stat/kvm_stat
>> @@ -270,6 +270,7 @@ class ArchX86(Arch):
>>       def __init__(self, exit_reasons):
>>           self.sc_perf_evt_open = 298
>>           self.ioctl_numbers = IOCTL_NUMBERS
>> +        self.exit_field = 'exit_reason'
> 
> Also, 'exit_field' name is confusing (probably because I'm thinking of
> VMCS fields)
> 

Ok. How about to have 'exit_reason_field' or 'exit_filter_name'?

>>           self.exit_reasons = exit_reasons
>>   
>>       def debugfs_is_child(self, field):
>> @@ -289,6 +290,7 @@ class ArchPPC(Arch):
>>           # numbers depend on the wordsize.
>>           char_ptr_size = ctypes.sizeof(ctypes.c_char_p)
>>           self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16
>> +        self.exit_field = 'exit_nr'
>>           self.exit_reasons = {}
>>   
>>       def debugfs_is_child(self, field):
>> @@ -300,6 +302,7 @@ class ArchA64(Arch):
>>       def __init__(self):
>>           self.sc_perf_evt_open = 241
>>           self.ioctl_numbers = IOCTL_NUMBERS
>> +        self.exit_field = 'ret'
> 
> And this is the most confusing part. Why do we have 'ret' as an exit
> reason in the first place?
> 

ah, it turns to be 'esr_ec', which is the class code from ESR_EL2. Will fix in v2.
The field name isn't meaningful and it's to be sorted out in future standardization
by the way :)


>>           self.exit_reasons = AARCH64_EXIT_REASONS
>>   
>>       def debugfs_is_child(self, field):
>> @@ -311,6 +314,7 @@ class ArchS390(Arch):
>>       def __init__(self):
>>           self.sc_perf_evt_open = 331
>>           self.ioctl_numbers = IOCTL_NUMBERS
>> +        self.exit_field = None
>>           self.exit_reasons = None
>>   
>>       def debugfs_is_child(self, field):
>> @@ -541,8 +545,8 @@ class TracepointProvider(Provider):
>>           """
>>           filters = {}
>>           filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS)
>> -        if ARCH.exit_reasons:
>> -            filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
>> +        if ARCH.exit_field and ARCH.exit_reasons:
>> +            filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons)
>>           return filters
>>   
>>       def _get_available_fields(self):
> 

Regards,
Gavin

Re: [PATCH] tools/kvm_stat: Fix kvm_exit filter name

[Vitaly Kuznetsov]

Gavin Shan <gshan@redhat.com> writes:

> On 12/10/19 7:45 PM, Vitaly Kuznetsov wrote:
>> Gavin Shan <gshan@redhat.com> writes:
>> 
>>> The filter name is fixed to "exit_reason" for some kvm_exit events, no
>>> matter what architect we have. Actually, the filter name ("exit_reason")
>>> is only applicable to x86, meaning it's broken on other architects
>>> including aarch64.
>>>
>>> This fixes the issue by providing various kvm_exit filter names, depending
>>> on architect we're on. Afterwards, the variable filter name is picked and
>>> applied through ioctl(fd, SET_FILTER).
>> 
>> Would it actually make sense to standardize (to certain extent) kvm_exit
>> tracepoints instead?
>> 
>
> Yes, It makes sense, but it's something for future if you agree. Besides,
> It seems that other kvm tracepoints need standardization either.

If we change kvm_stat the way you suggest we'll have reverse issues
after changing tracepoints fields: updated kvm_stat won't work with the
fixed kernel. I understand that kvm_stat doesn't have to work with
anything but the corresponding kernel tree and so we can change it back
in the same series, but wouldn't it be an unnecessary churn?

I'd suggest we standardize 'exit_reason' field name now just to fix the
immediate issue, it shouldn't be a big change, probably the same size as
this patch. Changing other tracepoints to match can be left for future
generations :-) What do you say?

-- 
Vitaly

Re: [PATCH] tools/kvm_stat: Fix kvm_exit filter name

[Gavin Shan]

On 12/11/19 11:15 PM, Vitaly Kuznetsov wrote:
> Gavin Shan <gshan@redhat.com> writes:
> 
>> On 12/10/19 7:45 PM, Vitaly Kuznetsov wrote:
>>> Gavin Shan <gshan@redhat.com> writes:
>>>
>>>> The filter name is fixed to "exit_reason" for some kvm_exit events, no
>>>> matter what architect we have. Actually, the filter name ("exit_reason")
>>>> is only applicable to x86, meaning it's broken on other architects
>>>> including aarch64.
>>>>
>>>> This fixes the issue by providing various kvm_exit filter names, depending
>>>> on architect we're on. Afterwards, the variable filter name is picked and
>>>> applied through ioctl(fd, SET_FILTER).
>>>
>>> Would it actually make sense to standardize (to certain extent) kvm_exit
>>> tracepoints instead?
>>>
>>
>> Yes, It makes sense, but it's something for future if you agree. Besides,
>> It seems that other kvm tracepoints need standardization either.
> 
> If we change kvm_stat the way you suggest we'll have reverse issues
> after changing tracepoints fields: updated kvm_stat won't work with the
> fixed kernel. I understand that kvm_stat doesn't have to work with
> anything but the corresponding kernel tree and so we can change it back
> in the same series, but wouldn't it be an unnecessary churn?
> 
> I'd suggest we standardize 'exit_reason' field name now just to fix the
> immediate issue, it shouldn't be a big change, probably the same size as
> this patch. Changing other tracepoints to match can be left for future
> generations :-) What do you say?
> 

Fair enough. A series of patches ("Standardize kvm exit reason field")
has been posted for review.

Regards,
Gavin

Re: [PATCH] tools/kvm_stat: Fix kvm_exit filter name

[Paolo Bonzini]

On 10/12/19 05:48, Gavin Shan wrote:
> The filter name is fixed to "exit_reason" for some kvm_exit events, no
> matter what architect we have. Actually, the filter name ("exit_reason")
> is only applicable to x86, meaning it's broken on other architects
> including aarch64.
> 
> This fixes the issue by providing various kvm_exit filter names, depending
> on architect we're on. Afterwards, the variable filter name is picked and
> applied through ioctl(fd, SET_FILTER).
> 
> Reported-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  tools/kvm/kvm_stat/kvm_stat | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
> index ad1b9e646c49..f9273614b7e3 100755
> --- a/tools/kvm/kvm_stat/kvm_stat
> +++ b/tools/kvm/kvm_stat/kvm_stat
> @@ -270,6 +270,7 @@ class ArchX86(Arch):
>      def __init__(self, exit_reasons):
>          self.sc_perf_evt_open = 298
>          self.ioctl_numbers = IOCTL_NUMBERS
> +        self.exit_field = 'exit_reason'
>          self.exit_reasons = exit_reasons
>  
>      def debugfs_is_child(self, field):
> @@ -289,6 +290,7 @@ class ArchPPC(Arch):
>          # numbers depend on the wordsize.
>          char_ptr_size = ctypes.sizeof(ctypes.c_char_p)
>          self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16
> +        self.exit_field = 'exit_nr'
>          self.exit_reasons = {}
>  
>      def debugfs_is_child(self, field):
> @@ -300,6 +302,7 @@ class ArchA64(Arch):
>      def __init__(self):
>          self.sc_perf_evt_open = 241
>          self.ioctl_numbers = IOCTL_NUMBERS
> +        self.exit_field = 'ret'
>          self.exit_reasons = AARCH64_EXIT_REASONS
>  
>      def debugfs_is_child(self, field):
> @@ -311,6 +314,7 @@ class ArchS390(Arch):
>      def __init__(self):
>          self.sc_perf_evt_open = 331
>          self.ioctl_numbers = IOCTL_NUMBERS
> +        self.exit_field = None
>          self.exit_reasons = None
>  
>      def debugfs_is_child(self, field):
> @@ -541,8 +545,8 @@ class TracepointProvider(Provider):
>          """
>          filters = {}
>          filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS)
> -        if ARCH.exit_reasons:
> -            filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
> +        if ARCH.exit_field and ARCH.exit_reasons:
> +            filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons)
>          return filters
>  
>      def _get_available_fields(self):
> 

Queued, thanks.

Paolo

Re: [PATCH] tools/kvm_stat: Fix kvm_exit filter name

[Gavin Shan]

On 1/19/20 6:18 AM, Paolo Bonzini wrote:
> On 10/12/19 05:48, Gavin Shan wrote:
>> The filter name is fixed to "exit_reason" for some kvm_exit events, no
>> matter what architect we have. Actually, the filter name ("exit_reason")
>> is only applicable to x86, meaning it's broken on other architects
>> including aarch64.
>>
>> This fixes the issue by providing various kvm_exit filter names, depending
>> on architect we're on. Afterwards, the variable filter name is picked and
>> applied through ioctl(fd, SET_FILTER).
>>
>> Reported-by: Andrew Jones <drjones@redhat.com>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   tools/kvm/kvm_stat/kvm_stat | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
>> index ad1b9e646c49..f9273614b7e3 100755
>> --- a/tools/kvm/kvm_stat/kvm_stat
>> +++ b/tools/kvm/kvm_stat/kvm_stat
>> @@ -270,6 +270,7 @@ class ArchX86(Arch):
>>       def __init__(self, exit_reasons):
>>           self.sc_perf_evt_open = 298
>>           self.ioctl_numbers = IOCTL_NUMBERS
>> +        self.exit_field = 'exit_reason'
>>           self.exit_reasons = exit_reasons
>>   
>>       def debugfs_is_child(self, field):
>> @@ -289,6 +290,7 @@ class ArchPPC(Arch):
>>           # numbers depend on the wordsize.
>>           char_ptr_size = ctypes.sizeof(ctypes.c_char_p)
>>           self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16
>> +        self.exit_field = 'exit_nr'
>>           self.exit_reasons = {}
>>   
>>       def debugfs_is_child(self, field):
>> @@ -300,6 +302,7 @@ class ArchA64(Arch):
>>       def __init__(self):
>>           self.sc_perf_evt_open = 241
>>           self.ioctl_numbers = IOCTL_NUMBERS
>> +        self.exit_field = 'ret'
>>           self.exit_reasons = AARCH64_EXIT_REASONS
>>   
>>       def debugfs_is_child(self, field):
>> @@ -311,6 +314,7 @@ class ArchS390(Arch):
>>       def __init__(self):
>>           self.sc_perf_evt_open = 331
>>           self.ioctl_numbers = IOCTL_NUMBERS
>> +        self.exit_field = None
>>           self.exit_reasons = None
>>   
>>       def debugfs_is_child(self, field):
>> @@ -541,8 +545,8 @@ class TracepointProvider(Provider):
>>           """
>>           filters = {}
>>           filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS)
>> -        if ARCH.exit_reasons:
>> -            filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons)
>> +        if ARCH.exit_field and ARCH.exit_reasons:
>> +            filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons)
>>           return filters
>>   
>>       def _get_available_fields(self):
>>
> 
> Queued, thanks.
> 
> Paolo
> 

Paolo, actually the v2 instead of this v1 patch needs to be merged:

https://www.spinics.net/lists/kvm/msg202841.html

Thanks,
Gavin