From: "Steven Rostedt (Red Hat)" <[email protected]>
To allow for the restructiong of the trace_seq code, we need users
of it to use the helper functions instead of accessing the internals
of the trace_seq structure itself.
Cc: Mark Rustad <[email protected]>
Cc: Jeff Kirsher <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
arch/x86/kvm/mmutrace.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
index 5aaf35641768..ce463a9cc8fb 100644
--- a/arch/x86/kvm/mmutrace.h
+++ b/arch/x86/kvm/mmutrace.h
@@ -22,7 +22,7 @@
__entry->unsync = sp->unsync;
#define KVM_MMU_PAGE_PRINTK() ({ \
- const u32 saved_len = p->len; \
+ const char *saved_ptr = trace_seq_buffer_ptr(p); \
static const char *access_str[] = { \
"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" \
}; \
@@ -41,7 +41,7 @@
role.nxe ? "" : "!", \
__entry->root_count, \
__entry->unsync ? "unsync" : "sync", 0); \
- p->buffer + saved_len; \
+ saved_ptr; \
})
#define kvm_mmu_trace_pferr_flags \
--
2.1.1
On 04/11/2014 16:52, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> To allow for the restructiong of the trace_seq code, we need users
> of it to use the helper functions instead of accessing the internals
> of the trace_seq structure itself.
>
> Cc: Mark Rustad <[email protected]>
> Cc: Jeff Kirsher <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> arch/x86/kvm/mmutrace.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
> index 5aaf35641768..ce463a9cc8fb 100644
> --- a/arch/x86/kvm/mmutrace.h
> +++ b/arch/x86/kvm/mmutrace.h
> @@ -22,7 +22,7 @@
> __entry->unsync = sp->unsync;
>
> #define KVM_MMU_PAGE_PRINTK() ({ \
> - const u32 saved_len = p->len; \
> + const char *saved_ptr = trace_seq_buffer_ptr(p); \
> static const char *access_str[] = { \
> "---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" \
> }; \
> @@ -41,7 +41,7 @@
> role.nxe ? "" : "!", \
> __entry->root_count, \
> __entry->unsync ? "unsync" : "sync", 0); \
> - p->buffer + saved_len; \
> + saved_ptr; \
> })
>
> #define kvm_mmu_trace_pferr_flags \
> -- 2.1.1
>
Acked-by: Paolo Bonzini <[email protected]>
On Nov 4, 2014, at 7:52 AM, Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> To allow for the restructiong of the trace_seq code, we need users
> of it to use the helper functions instead of accessing the internals
> of the trace_seq structure itself.
>
> Cc: Mark Rustad <[email protected]>
> Cc: Jeff Kirsher <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> arch/x86/kvm/mmutrace.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
> index 5aaf35641768..ce463a9cc8fb 100644
> --- a/arch/x86/kvm/mmutrace.h
> +++ b/arch/x86/kvm/mmutrace.h
> @@ -22,7 +22,7 @@
> __entry->unsync = sp->unsync;
>
> #define KVM_MMU_PAGE_PRINTK() ({ \
> - const u32 saved_len = p->len; \
> + const char *saved_ptr = trace_seq_buffer_ptr(p); \
I think the above should not be a const char *, because the location pointed to is surely being changed. It should either be a char * or a char * const.
> static const char *access_str[] = { \
> "---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" \
> }; \
> @@ -41,7 +41,7 @@
> role.nxe ? "" : "!", \
> __entry->root_count, \
> __entry->unsync ? "unsync" : "sync", 0); \
> - p->buffer + saved_len; \
> + saved_ptr; \
> })
>
> #define kvm_mmu_trace_pferr_flags \
> --
> 2.1.1
--
Mark Rustad, Networking Division, Intel Corporation
On Tue, 4 Nov 2014 17:17:08 +0000
"Rustad, Mark D" <[email protected]> wrote:
> On Nov 4, 2014, at 7:52 AM, Steven Rostedt <[email protected]> wrote:
>
> > From: "Steven Rostedt (Red Hat)" <[email protected]>
> >
> > To allow for the restructiong of the trace_seq code, we need users
> > of it to use the helper functions instead of accessing the internals
> > of the trace_seq structure itself.
> >
> > Cc: Mark Rustad <[email protected]>
> > Cc: Jeff Kirsher <[email protected]>
> > Cc: Paolo Bonzini <[email protected]>
> > Signed-off-by: Steven Rostedt <[email protected]>
> > ---
> > arch/x86/kvm/mmutrace.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
> > index 5aaf35641768..ce463a9cc8fb 100644
> > --- a/arch/x86/kvm/mmutrace.h
> > +++ b/arch/x86/kvm/mmutrace.h
> > @@ -22,7 +22,7 @@
> > __entry->unsync = sp->unsync;
> >
> > #define KVM_MMU_PAGE_PRINTK() ({ \
> > - const u32 saved_len = p->len; \
> > + const char *saved_ptr = trace_seq_buffer_ptr(p); \
>
> I think the above should not be a const char *, because the location pointed to is surely being changed. It should either be a char * or a char * const.
Ah right. It should be 'char * const'.
Thanks, I'll update it.
-- Steve
>
> > static const char *access_str[] = { \
> > "---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" \
> > }; \
> > @@ -41,7 +41,7 @@
> > role.nxe ? "" : "!", \
> > __entry->root_count, \
> > __entry->unsync ? "unsync" : "sync", 0); \
> > - p->buffer + saved_len; \
> > + saved_ptr; \
> > })
> >
> > #define kvm_mmu_trace_pferr_flags \
> > --
> > 2.1.1
>
On Tue, 4 Nov 2014 14:09:54 -0500
Steven Rostedt <[email protected]> wrote:
> On Tue, 4 Nov 2014 17:17:08 +0000
> "Rustad, Mark D" <[email protected]> wrote:
>
> > On Nov 4, 2014, at 7:52 AM, Steven Rostedt <[email protected]> wrote:
> >
> > > From: "Steven Rostedt (Red Hat)" <[email protected]>
> > >
> > > To allow for the restructiong of the trace_seq code, we need users
> > > of it to use the helper functions instead of accessing the internals
> > > of the trace_seq structure itself.
> > >
> > > Cc: Mark Rustad <[email protected]>
> > > Cc: Jeff Kirsher <[email protected]>
> > > Cc: Paolo Bonzini <[email protected]>
> > > Signed-off-by: Steven Rostedt <[email protected]>
> > > ---
> > > arch/x86/kvm/mmutrace.h | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
> > > index 5aaf35641768..ce463a9cc8fb 100644
> > > --- a/arch/x86/kvm/mmutrace.h
> > > +++ b/arch/x86/kvm/mmutrace.h
> > > @@ -22,7 +22,7 @@
> > > __entry->unsync = sp->unsync;
> > >
> > > #define KVM_MMU_PAGE_PRINTK() ({ \
> > > - const u32 saved_len = p->len; \
> > > + const char *saved_ptr = trace_seq_buffer_ptr(p); \
> >
> > I think the above should not be a const char *, because the location pointed to is surely being changed. It should either be a char * or a char * const.
>
> Ah right. It should be 'char * const'.
>
Actually, I take that back. The contents of saved_ptr should not be
modified.
It may seem strange, but the update is done via the trace_seq_printf().
Then that content is return back to the user. The user should
definitely *not* modify the contents of saved_ptr.
This patch is good as is. It should not be a char *, or char * const.
-- Steve
>
> >
> > > static const char *access_str[] = { \
> > > "---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" \
> > > }; \
> > > @@ -41,7 +41,7 @@
> > > role.nxe ? "" : "!", \
> > > __entry->root_count, \
> > > __entry->unsync ? "unsync" : "sync", 0); \
> > > - p->buffer + saved_len; \
> > > + saved_ptr; \
> > > })
> > >
> > > #define kvm_mmu_trace_pferr_flags \
> > > --
> > > 2.1.1
> >
>
On Nov 4, 2014, at 11:35 AM, Steven Rostedt <[email protected]> wrote:
> On Tue, 4 Nov 2014 14:09:54 -0500
> Steven Rostedt <[email protected]> wrote:
>
>> On Tue, 4 Nov 2014 17:17:08 +0000
>> "Rustad, Mark D" <[email protected]> wrote:
>>
>>> On Nov 4, 2014, at 7:52 AM, Steven Rostedt <[email protected]> wrote:
>>>
>>>> From: "Steven Rostedt (Red Hat)" <[email protected]>
>>>>
>>>> To allow for the restructiong of the trace_seq code, we need users
>>>> of it to use the helper functions instead of accessing the internals
>>>> of the trace_seq structure itself.
>>>>
>>>> Cc: Mark Rustad <[email protected]>
>>>> Cc: Jeff Kirsher <[email protected]>
>>>> Cc: Paolo Bonzini <[email protected]>
>>>> Signed-off-by: Steven Rostedt <[email protected]>
>>>> ---
>>>> arch/x86/kvm/mmutrace.h | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
>>>> index 5aaf35641768..ce463a9cc8fb 100644
>>>> --- a/arch/x86/kvm/mmutrace.h
>>>> +++ b/arch/x86/kvm/mmutrace.h
>>>> @@ -22,7 +22,7 @@
>>>> __entry->unsync = sp->unsync;
>>>>
>>>> #define KVM_MMU_PAGE_PRINTK() ({ \
>>>> - const u32 saved_len = p->len; \
>>>> + const char *saved_ptr = trace_seq_buffer_ptr(p); \
>>>
>>> I think the above should not be a const char *, because the location pointed to is surely being changed. It should either be a char * or a char * const.
>>
>> Ah right. It should be 'char * const'.
>>
>
> Actually, I take that back. The contents of saved_ptr should not be
> modified.
At least not by the caller of the macro. The subsequent call to trace_seq_printf will be changing the contents at that address, but not through use of that pointer.
> It may seem strange, but the update is done via the trace_seq_printf().
> Then that content is return back to the user. The user should
> definitely *not* modify the contents of saved_ptr.
Agreed.
> This patch is good as is. It should not be a char *, or char * const.
Yes. I did further checking and agree. Although that memory will be written, it isn't written through that pointer and it is the best type as a return value.
> -- Steve
>
>
>>
>>>
>>>> static const char *access_str[] = { \
>>>> "---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" \
>>>> }; \
>>>> @@ -41,7 +41,7 @@
>>>> role.nxe ? "" : "!", \
>>>> __entry->root_count, \
>>>> __entry->unsync ? "unsync" : "sync", 0); \
>>>> - p->buffer + saved_len; \
>>>> + saved_ptr; \
>>>> })
>>>>
>>>> #define kvm_mmu_trace_pferr_flags \
>>>> --
>>>> 2.1.1
>>>
>>
>
Acked-by: Mark Rustad <[email protected]>
--
Mark Rustad, Networking Division, Intel Corporation
On Tue 2014-11-04 10:52:38, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> To allow for the restructiong of the trace_seq code, we need users
> of it to use the helper functions instead of accessing the internals
> of the trace_seq structure itself.
>
> Cc: Mark Rustad <[email protected]>
> Cc: Jeff Kirsher <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
> ---
> arch/x86/kvm/mmutrace.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
> index 5aaf35641768..ce463a9cc8fb 100644
> --- a/arch/x86/kvm/mmutrace.h
> +++ b/arch/x86/kvm/mmutrace.h
> @@ -22,7 +22,7 @@
> __entry->unsync = sp->unsync;
>
> #define KVM_MMU_PAGE_PRINTK() ({ \
> - const u32 saved_len = p->len; \
> + const char *saved_ptr = trace_seq_buffer_ptr(p); \
> static const char *access_str[] = { \
> "---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" \
> }; \
> @@ -41,7 +41,7 @@
> role.nxe ? "" : "!", \
> __entry->root_count, \
> __entry->unsync ? "unsync" : "sync", 0); \
> - p->buffer + saved_len; \
> + saved_ptr; \
> })
>
> #define kvm_mmu_trace_pferr_flags \
> --
> 2.1.1
>
>