Re: [PATCH] tools lib traceevent: update KVM plugin

[Arnaldo Carvalho de Melo]

Em Fri, Oct 09, 2015 at 10:10:13PM +0200, Paolo Bonzini escreveu:
> On 01/10/2015 12:28, Paolo Bonzini wrote:
> > The format of the role word has changed through the years and the
> > plugin was never updated; some VMX exit reasons were missing too.
> > 
> > Signed-off-by: Paolo Bonzini 
> > ---
> >  tools/lib/traceevent/plugin_kvm.c | 25 +
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/lib/traceevent/plugin_kvm.c 
> > b/tools/lib/traceevent/plugin_kvm.c
> > index 88fe83dff7cd..18536f756577 100644
> > --- a/tools/lib/traceevent/plugin_kvm.c
> > +++ b/tools/lib/traceevent/plugin_kvm.c
> > @@ -124,7 +124,10 @@ static const char *disassemble(unsigned char *insn, 
> > int len, uint64_t rip,
> > _ER(WBINVD,  54)\
> > _ER(XSETBV,  55)\
> > _ER(APIC_WRITE,  56)\
> > -   _ER(INVPCID, 58)
> > +   _ER(INVPCID, 58)\
> > +   _ER(PML_FULL,62)\
> > +   _ER(XSAVES,  63)\
> > +   _ER(XRSTORS, 64)
> >  
> >  #define SVM_EXIT_REASONS \
> > _ER(EXIT_READ_CR0,  0x000)  \
> > @@ -352,15 +355,18 @@ static int kvm_nested_vmexit_handler(struct trace_seq 
> > *s, struct pevent_record *
> >  union kvm_mmu_page_role {
> > unsigned word;
> > struct {
> > -   unsigned glevels:4;
> > unsigned level:4;
> > +   unsigned cr4_pae:1;
> > unsigned quadrant:2;
> > -   unsigned pad_for_nice_hex_output:6;
> > unsigned direct:1;
> > unsigned access:3;
> > unsigned invalid:1;
> > -   unsigned cr4_pge:1;
> > unsigned nxe:1;
> > +   unsigned cr0_wp:1;
> > +   unsigned smep_and_not_wp:1;
> > +   unsigned smap_and_not_wp:1;
> > +   unsigned pad_for_nice_hex_output:8;
> > +   unsigned smm:8;
> > };
> >  };
> >  
> > @@ -385,15 +391,18 @@ static int kvm_mmu_print_role(struct trace_seq *s, 
> > struct pevent_record *record,
> > if (pevent_is_file_bigendian(event->pevent) ==
> > pevent_is_host_bigendian(event->pevent)) {
> >  
> > -   trace_seq_printf(s, "%u/%u q%u%s %s%s %spge %snxe",
> > +   trace_seq_printf(s, "%u q%u%s %s%s %spae %snxe %swp%s%s%s",
> >  role.level,
> > -role.glevels,
> >  role.quadrant,
> >  role.direct ? " direct" : "",
> >  access_str[role.access],
> >  role.invalid ? " invalid" : "",
> > -role.cr4_pge ? "" : "!",
> > -role.nxe ? "" : "!");
> > +role.cr4_pae ? "" : "!",
> > +role.nxe ? "" : "!",
> > +role.cr0_wp ? "" : "!",
> > +role.smep_and_not_wp ? " smep" : "",
> > +role.smap_and_not_wp ? " smap" : "",
> > +role.smm ? " smm" : "");
> > } else
> > trace_seq_printf(s, "WORD: %08x", role.word);
> >  
> > 
> 
> Ping?  Arnaldo, ok to include this patch in my tree?

Applying, I saw it before, but lost track, perhaps was waiting for
Steven Rostedt to chime in, but now I noticed he wasn't CCed, he is now.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Steven Rostedt]

On Tue, 20 Oct 2015 17:19:12 +0200
Paolo Bonzini  wrote:

> 
> 
> On 20/10/2015 16:44, Steven Rostedt wrote:
> > What happens if you run new perf on an older kernel. Is this new plugin
> > going to be screwed up? Plugins should be backward compatible.
> 
> If you run new perf on older kernel, the new plugin will print the 
> "role" field (see kvm_mmu_print_role) slightly incorrectly.  That said, 
> the existing plugin was _also_ printing the role in a wildly wrong 
> format, like 2.6.35 vintage; the glevels field was removed by commit 
> 5b7e0102ae74, "KVM: MMU: Replace role.glevels with role.cr4_pae", in 
> April 2010.

Can we add a check if glevels field exists, and do the old format if it
does? I'm very strict on making sure event-parse works with all
incarnations of kernels.

> 
> Going forward it's really unlikely that the role will change apart from 
> adding new bits.  These can be added to the plugin while keeping it 
> backwards-compatible.  Addition to the role happen when you implement 
> new virtual MMU features such as SMEP, SMAP or SMM.  That's once per year
> or less.
> 
> > Is the plugin even still needed? I'm looking at some of the kvm events
> > and they seem to be mostly self sufficient. What ones need a plugin
> > today?
> 
> Yes, most of them are.  It's only needed for kvm_mmu_get_page, 
> kvm_mmu_prepare_zap_page and kvm_emulate_insn.  The latter is only 
> interesting if you install the disassembler library, and I wouldn't 
> really care if it went away.
> 
> kvm_mmu_get_page and kvm_mmu_prepare_zap_page, however, have output like
> 
> kvm_mmu_get_page: [FAILED TO PARSE] mmu_valid_gen=0x2 gfn=786432 
> role=1923 root_count=0 unsync=0 created=1
> 
> without the plugin vs.
> 
> kvm_mmu_get_page: new sp gfn c 3 q0 direct rwx !pae !nxe !wp root 0 
> sync
> 
> with the plugin.

Thanks for the update.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Arnaldo Carvalho de Melo]

Em Tue, Oct 20, 2015 at 05:49:22PM +0200, Paolo Bonzini escreveu:
> 
> 
> On 20/10/2015 17:48, Steven Rostedt wrote:
> > On Tue, 20 Oct 2015 17:37:43 +0200
> > Paolo Bonzini  wrote:
> > 
> >> However, it frankly seems a bit academic.  The parsing _will_ work,
> >> apart from printing a nonsensical role just like it has always done for
> >> the past four years.
> > 
> > I'm not going to be too picky about it. But things like this may seem
> > academic, but it's also what we forget about, and people still use
> > 2.6.32 kernels, and this may come back and bite us later.
> > 
> > But it's more likely to bite you than me, so if you don't think it's
> > worth while, then I'm not going to push it.
> 
> Thanks.  The role is not going to matter except in really weird cases
> and hopefully those are just not there in 2.6.32 kernels.  Thanks also
> for educating me so I won't get it wrong in the future!

Cool, Steven, can I take that as an Acked-by for this patch?

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Paolo Bonzini]

On 20/10/2015 17:26, Steven Rostedt wrote:
> > > > What happens if you run new perf on an older kernel. Is this new plugin
> > > > going to be screwed up? Plugins should be backward compatible.
> > 
> > If you run new perf on older kernel, the new plugin will print the 
> > "role" field (see kvm_mmu_print_role) slightly incorrectly.  That said, 
> > the existing plugin was _also_ printing the role in a wildly wrong 
> > format, like 2.6.35 vintage; the glevels field was removed by commit 
> > 5b7e0102ae74, "KVM: MMU: Replace role.glevels with role.cr4_pae", in 
> > April 2010.
> 
> Can we add a check if glevels field exists, and do the old format if it
> does? I'm very strict on making sure event-parse works with all
> incarnations of kernels.

Note that this is not a glevels _tracepoint_ field.  The tracepoint
field is "role" and it is a 64-bit integer.  The plugin interprets the
bitfields of that integer, and the position/meaning of the bitfields has
changed.

I guess I could use the mmu_valid_gen field as a proxy (it was
introduced after glevels was removed), and only provide the "very old"
(up to 2.6.34) and "new" (4.2+) formats.  The "old" format (2.6.35-4.1)
was never implemented in the plugin; it may well remain that way.

However, it frankly seems a bit academic.  The parsing _will_ work,
apart from printing a nonsensical role just like it has always done for
the past four years.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Steven Rostedt]

On Tue, 20 Oct 2015 12:50:26 -0300
Arnaldo Carvalho de Melo  wrote:

> Cool, Steven, can I take that as an Acked-by for this patch?
> 

Acked-by: Steven Rostedt 

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Paolo Bonzini]

On 20/10/2015 16:44, Steven Rostedt wrote:
> What happens if you run new perf on an older kernel. Is this new plugin
> going to be screwed up? Plugins should be backward compatible.

If you run new perf on older kernel, the new plugin will print the 
"role" field (see kvm_mmu_print_role) slightly incorrectly.  That said, 
the existing plugin was _also_ printing the role in a wildly wrong 
format, like 2.6.35 vintage; the glevels field was removed by commit 
5b7e0102ae74, "KVM: MMU: Replace role.glevels with role.cr4_pae", in 
April 2010.

Going forward it's really unlikely that the role will change apart from 
adding new bits.  These can be added to the plugin while keeping it 
backwards-compatible.  Addition to the role happen when you implement 
new virtual MMU features such as SMEP, SMAP or SMM.  That's once per year
or less.

> Is the plugin even still needed? I'm looking at some of the kvm events
> and they seem to be mostly self sufficient. What ones need a plugin
> today?

Yes, most of them are.  It's only needed for kvm_mmu_get_page, 
kvm_mmu_prepare_zap_page and kvm_emulate_insn.  The latter is only 
interesting if you install the disassembler library, and I wouldn't 
really care if it went away.

kvm_mmu_get_page and kvm_mmu_prepare_zap_page, however, have output like

kvm_mmu_get_page: [FAILED TO PARSE] mmu_valid_gen=0x2 gfn=786432 role=1923 
root_count=0 unsync=0 created=1

without the plugin vs.

kvm_mmu_get_page: new sp gfn c 3 q0 direct rwx !pae !nxe !wp root 0 sync

with the plugin.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Paolo Bonzini]

On 20/10/2015 17:48, Steven Rostedt wrote:
> On Tue, 20 Oct 2015 17:37:43 +0200
> Paolo Bonzini  wrote:
> 
>> However, it frankly seems a bit academic.  The parsing _will_ work,
>> apart from printing a nonsensical role just like it has always done for
>> the past four years.
> 
> I'm not going to be too picky about it. But things like this may seem
> academic, but it's also what we forget about, and people still use
> 2.6.32 kernels, and this may come back and bite us later.
> 
> But it's more likely to bite you than me, so if you don't think it's
> worth while, then I'm not going to push it.

Thanks.  The role is not going to matter except in really weird cases
and hopefully those are just not there in 2.6.32 kernels.  Thanks also
for educating me so I won't get it wrong in the future!

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Steven Rostedt]

On Tue, 20 Oct 2015 17:37:43 +0200
Paolo Bonzini  wrote:

> However, it frankly seems a bit academic.  The parsing _will_ work,
> apart from printing a nonsensical role just like it has always done for
> the past four years.

I'm not going to be too picky about it. But things like this may seem
academic, but it's also what we forget about, and people still use
2.6.32 kernels, and this may come back and bite us later.

But it's more likely to bite you than me, so if you don't think it's
worth while, then I'm not going to push it.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Steven Rostedt]

On Tue, 20 Oct 2015 12:32:44 -0200
Arnaldo Carvalho de Melo  wrote:


> > Ping?  Arnaldo, ok to include this patch in my tree?
> 
> Applying, I saw it before, but lost track, perhaps was waiting for
> Steven Rostedt to chime in, but now I noticed he wasn't CCed, he is now.

What happens if you run new perf on an older kernel. Is this new plugin
going to be screwed up? Plugins should be backward compatible.

Is the plugin even still needed? I'm looking at some of the kvm events
and they seem to be mostly self sufficient. What ones need a plugin
today?

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools lib traceevent: update KVM plugin

[Paolo Bonzini]

On 01/10/2015 12:28, Paolo Bonzini wrote:
> The format of the role word has changed through the years and the
> plugin was never updated; some VMX exit reasons were missing too.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  tools/lib/traceevent/plugin_kvm.c | 25 +
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/lib/traceevent/plugin_kvm.c 
> b/tools/lib/traceevent/plugin_kvm.c
> index 88fe83dff7cd..18536f756577 100644
> --- a/tools/lib/traceevent/plugin_kvm.c
> +++ b/tools/lib/traceevent/plugin_kvm.c
> @@ -124,7 +124,10 @@ static const char *disassemble(unsigned char *insn, int 
> len, uint64_t rip,
>   _ER(WBINVD,  54)\
>   _ER(XSETBV,  55)\
>   _ER(APIC_WRITE,  56)\
> - _ER(INVPCID, 58)
> + _ER(INVPCID, 58)\
> + _ER(PML_FULL,62)\
> + _ER(XSAVES,  63)\
> + _ER(XRSTORS, 64)
>  
>  #define SVM_EXIT_REASONS \
>   _ER(EXIT_READ_CR0,  0x000)  \
> @@ -352,15 +355,18 @@ static int kvm_nested_vmexit_handler(struct trace_seq 
> *s, struct pevent_record *
>  union kvm_mmu_page_role {
>   unsigned word;
>   struct {
> - unsigned glevels:4;
>   unsigned level:4;
> + unsigned cr4_pae:1;
>   unsigned quadrant:2;
> - unsigned pad_for_nice_hex_output:6;
>   unsigned direct:1;
>   unsigned access:3;
>   unsigned invalid:1;
> - unsigned cr4_pge:1;
>   unsigned nxe:1;
> + unsigned cr0_wp:1;
> + unsigned smep_and_not_wp:1;
> + unsigned smap_and_not_wp:1;
> + unsigned pad_for_nice_hex_output:8;
> + unsigned smm:8;
>   };
>  };
>  
> @@ -385,15 +391,18 @@ static int kvm_mmu_print_role(struct trace_seq *s, 
> struct pevent_record *record,
>   if (pevent_is_file_bigendian(event->pevent) ==
>   pevent_is_host_bigendian(event->pevent)) {
>  
> - trace_seq_printf(s, "%u/%u q%u%s %s%s %spge %snxe",
> + trace_seq_printf(s, "%u q%u%s %s%s %spae %snxe %swp%s%s%s",
>role.level,
> -  role.glevels,
>role.quadrant,
>role.direct ? " direct" : "",
>access_str[role.access],
>role.invalid ? " invalid" : "",
> -  role.cr4_pge ? "" : "!",
> -  role.nxe ? "" : "!");
> +  role.cr4_pae ? "" : "!",
> +  role.nxe ? "" : "!",
> +  role.cr0_wp ? "" : "!",
> +  role.smep_and_not_wp ? " smep" : "",
> +  role.smap_and_not_wp ? " smap" : "",
> +  role.smm ? " smm" : "");
>   } else
>   trace_seq_printf(s, "WORD: %08x", role.word);
>  
> 

Ping?  Arnaldo, ok to include this patch in my tree?

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] tools lib traceevent: update KVM plugin

[Paolo Bonzini]

The format of the role word has changed through the years and the
plugin was never updated; some VMX exit reasons were missing too.

Signed-off-by: Paolo Bonzini 
---
 tools/lib/traceevent/plugin_kvm.c | 25 +
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/tools/lib/traceevent/plugin_kvm.c 
b/tools/lib/traceevent/plugin_kvm.c
index 88fe83dff7cd..18536f756577 100644
--- a/tools/lib/traceevent/plugin_kvm.c
+++ b/tools/lib/traceevent/plugin_kvm.c
@@ -124,7 +124,10 @@ static const char *disassemble(unsigned char *insn, int 
len, uint64_t rip,
_ER(WBINVD,  54)\
_ER(XSETBV,  55)\
_ER(APIC_WRITE,  56)\
-   _ER(INVPCID, 58)
+   _ER(INVPCID, 58)\
+   _ER(PML_FULL,62)\
+   _ER(XSAVES,  63)\
+   _ER(XRSTORS, 64)
 
 #define SVM_EXIT_REASONS \
_ER(EXIT_READ_CR0,  0x000)  \
@@ -352,15 +355,18 @@ static int kvm_nested_vmexit_handler(struct trace_seq *s, 
struct pevent_record *
 union kvm_mmu_page_role {
unsigned word;
struct {
-   unsigned glevels:4;
unsigned level:4;
+   unsigned cr4_pae:1;
unsigned quadrant:2;
-   unsigned pad_for_nice_hex_output:6;
unsigned direct:1;
unsigned access:3;
unsigned invalid:1;
-   unsigned cr4_pge:1;
unsigned nxe:1;
+   unsigned cr0_wp:1;
+   unsigned smep_and_not_wp:1;
+   unsigned smap_and_not_wp:1;
+   unsigned pad_for_nice_hex_output:8;
+   unsigned smm:8;
};
 };
 
@@ -385,15 +391,18 @@ static int kvm_mmu_print_role(struct trace_seq *s, struct 
pevent_record *record,
if (pevent_is_file_bigendian(event->pevent) ==
pevent_is_host_bigendian(event->pevent)) {
 
-   trace_seq_printf(s, "%u/%u q%u%s %s%s %spge %snxe",
+   trace_seq_printf(s, "%u q%u%s %s%s %spae %snxe %swp%s%s%s",
 role.level,
-role.glevels,
 role.quadrant,
 role.direct ? " direct" : "",
 access_str[role.access],
 role.invalid ? " invalid" : "",
-role.cr4_pge ? "" : "!",
-role.nxe ? "" : "!");
+role.cr4_pae ? "" : "!",
+role.nxe ? "" : "!",
+role.cr0_wp ? "" : "!",
+role.smep_and_not_wp ? " smep" : "",
+role.smap_and_not_wp ? " smap" : "",
+role.smm ? " smm" : "");
} else
trace_seq_printf(s, "WORD: %08x", role.word);
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html