diff mbox series

[v7,10/10] x86/vm_event: Carry Processor Trace buffer offset in vm_event

Message ID 20210121212718.2441-11-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series Implement support for external IPT monitoring | expand

Commit Message

Andrew Cooper Jan. 21, 2021, 9:27 p.m. UTC
From: Tamas K Lengyel <tamas.lengyel@intel.com>

Add pt_offset field to x86 regs in vm_event. Initialized to ~0 if PT
is not in use.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>

v7:
 * New
---
 xen/arch/x86/vm_event.c       | 3 +++
 xen/include/public/vm_event.h | 6 ++++++
 2 files changed, 9 insertions(+)

Comments

Jan Beulich Jan. 26, 2021, 2:27 p.m. UTC | #1
On 21.01.2021 22:27, Andrew Cooper wrote:
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -251,6 +251,9 @@ void vm_event_fill_regs(vm_event_request_t *req)
>  
>      req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
>      req->data.regs.x86.dr6 = ctxt.dr6;
> +
> +    if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.pt_offset) != 1 )
> +        req->data.regs.x86.pt_offset = ~0;

Ah. (Regarding my earlier question about this returning -errno or
boolean).

> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -223,6 +223,12 @@ struct vm_event_regs_x86 {
>       */
>      uint64_t npt_base;
>  
> +    /*
> +     * Current offset in the Processor Trace buffer. For Intel Processor Trace
> +     * this is MSR_RTIT_OUTPUT_MASK. Set to ~0 if no Processor Trace is active.
> +     */
> +    uint64_t pt_offset;

According to vmtrace_output_position() the value is only one half
of what the named MSR contains. Perhaps "... this is from MSR_..."?
Not sure whether, despite this, there still is a reason to have
this 64-bit wide.

Jan
Andrew Cooper Jan. 29, 2021, 11:22 p.m. UTC | #2
On 26/01/2021 14:27, Jan Beulich wrote:
> On 21.01.2021 22:27, Andrew Cooper wrote:
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -251,6 +251,9 @@ void vm_event_fill_regs(vm_event_request_t *req)
>>  
>>      req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
>>      req->data.regs.x86.dr6 = ctxt.dr6;
>> +
>> +    if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.pt_offset) != 1 )
>> +        req->data.regs.x86.pt_offset = ~0;
> Ah. (Regarding my earlier question about this returning -errno or
> boolean).
>
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -223,6 +223,12 @@ struct vm_event_regs_x86 {
>>       */
>>      uint64_t npt_base;
>>  
>> +    /*
>> +     * Current offset in the Processor Trace buffer. For Intel Processor Trace
>> +     * this is MSR_RTIT_OUTPUT_MASK. Set to ~0 if no Processor Trace is active.
>> +     */
>> +    uint64_t pt_offset;
> According to vmtrace_output_position() the value is only one half
> of what the named MSR contains. Perhaps "... this is from MSR_..."?
> Not sure whether, despite this, there still is a reason to have
> this 64-bit wide.

This is a vestigial remnant which escaped the "use vmtrace uniformly" work.

It should match the domctl_vmtrace_output_position() format, so each
interface gives the same content for the attempted-platform-neutral version.

~Andrew
Tamas K Lengyel Jan. 29, 2021, 11:40 p.m. UTC | #3
On Fri, Jan 29, 2021 at 6:22 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 26/01/2021 14:27, Jan Beulich wrote:
> > On 21.01.2021 22:27, Andrew Cooper wrote:
> >> --- a/xen/arch/x86/vm_event.c
> >> +++ b/xen/arch/x86/vm_event.c
> >> @@ -251,6 +251,9 @@ void vm_event_fill_regs(vm_event_request_t *req)
> >>
> >>      req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
> >>      req->data.regs.x86.dr6 = ctxt.dr6;
> >> +
> >> +    if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.pt_offset) != 1 )
> >> +        req->data.regs.x86.pt_offset = ~0;
> > Ah. (Regarding my earlier question about this returning -errno or
> > boolean).
> >
> >> --- a/xen/include/public/vm_event.h
> >> +++ b/xen/include/public/vm_event.h
> >> @@ -223,6 +223,12 @@ struct vm_event_regs_x86 {
> >>       */
> >>      uint64_t npt_base;
> >>
> >> +    /*
> >> +     * Current offset in the Processor Trace buffer. For Intel Processor Trace
> >> +     * this is MSR_RTIT_OUTPUT_MASK. Set to ~0 if no Processor Trace is active.
> >> +     */
> >> +    uint64_t pt_offset;
> > According to vmtrace_output_position() the value is only one half
> > of what the named MSR contains. Perhaps "... this is from MSR_..."?
> > Not sure whether, despite this, there still is a reason to have
> > this 64-bit wide.
>
> This is a vestigial remnant which escaped the "use vmtrace uniformly" work.
>
> It should match the domctl_vmtrace_output_position() format, so each
> interface gives the same content for the attempted-platform-neutral version.

From the vm_event ABI perspective it's simpler to have a 64-bit value
here even if the max value it may possibly carry is never going to use
the whole 64-bit width. I rather not play with shortening it just to
add padding somewhere else.

As for what it's called is not that important from my perspective,
vmtrace_pos or something like that for example is fine by me.

Tamas
Jan Beulich Feb. 1, 2021, 8:55 a.m. UTC | #4
On 30.01.2021 00:40, Tamas K Lengyel wrote:
> On Fri, Jan 29, 2021 at 6:22 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> On 26/01/2021 14:27, Jan Beulich wrote:
>>> On 21.01.2021 22:27, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/vm_event.c
>>>> +++ b/xen/arch/x86/vm_event.c
>>>> @@ -251,6 +251,9 @@ void vm_event_fill_regs(vm_event_request_t *req)
>>>>
>>>>      req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
>>>>      req->data.regs.x86.dr6 = ctxt.dr6;
>>>> +
>>>> +    if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.pt_offset) != 1 )
>>>> +        req->data.regs.x86.pt_offset = ~0;
>>> Ah. (Regarding my earlier question about this returning -errno or
>>> boolean).
>>>
>>>> --- a/xen/include/public/vm_event.h
>>>> +++ b/xen/include/public/vm_event.h
>>>> @@ -223,6 +223,12 @@ struct vm_event_regs_x86 {
>>>>       */
>>>>      uint64_t npt_base;
>>>>
>>>> +    /*
>>>> +     * Current offset in the Processor Trace buffer. For Intel Processor Trace
>>>> +     * this is MSR_RTIT_OUTPUT_MASK. Set to ~0 if no Processor Trace is active.
>>>> +     */
>>>> +    uint64_t pt_offset;
>>> According to vmtrace_output_position() the value is only one half
>>> of what the named MSR contains. Perhaps "... this is from MSR_..."?
>>> Not sure whether, despite this, there still is a reason to have
>>> this 64-bit wide.
>>
>> This is a vestigial remnant which escaped the "use vmtrace uniformly" work.
>>
>> It should match the domctl_vmtrace_output_position() format, so each
>> interface gives the same content for the attempted-platform-neutral version.
> 
> From the vm_event ABI perspective it's simpler to have a 64-bit value
> here even if the max value it may possibly carry is never going to use
> the whole 64-bit width. I rather not play with shortening it just to
> add padding somewhere else.
> 
> As for what it's called is not that important from my perspective,
> vmtrace_pos or something like that for example is fine by me.

The important thing to me is that the comment not be misleading.
Whether that's arranged for by adjusting the comment of the
commented declaration is up to what you deem better, i.e. I
understand the comment it is.

Jan
Andrew Cooper Feb. 1, 2021, 9:06 a.m. UTC | #5
On 01/02/2021 08:55, Jan Beulich wrote:
> On 30.01.2021 00:40, Tamas K Lengyel wrote:
>> On Fri, Jan 29, 2021 at 6:22 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 26/01/2021 14:27, Jan Beulich wrote:
>>>> On 21.01.2021 22:27, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/vm_event.c
>>>>> +++ b/xen/arch/x86/vm_event.c
>>>>> @@ -251,6 +251,9 @@ void vm_event_fill_regs(vm_event_request_t *req)
>>>>>
>>>>>      req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
>>>>>      req->data.regs.x86.dr6 = ctxt.dr6;
>>>>> +
>>>>> +    if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.pt_offset) != 1 )
>>>>> +        req->data.regs.x86.pt_offset = ~0;
>>>> Ah. (Regarding my earlier question about this returning -errno or
>>>> boolean).
>>>>
>>>>> --- a/xen/include/public/vm_event.h
>>>>> +++ b/xen/include/public/vm_event.h
>>>>> @@ -223,6 +223,12 @@ struct vm_event_regs_x86 {
>>>>>       */
>>>>>      uint64_t npt_base;
>>>>>
>>>>> +    /*
>>>>> +     * Current offset in the Processor Trace buffer. For Intel Processor Trace
>>>>> +     * this is MSR_RTIT_OUTPUT_MASK. Set to ~0 if no Processor Trace is active.
>>>>> +     */
>>>>> +    uint64_t pt_offset;
>>>> According to vmtrace_output_position() the value is only one half
>>>> of what the named MSR contains. Perhaps "... this is from MSR_..."?
>>>> Not sure whether, despite this, there still is a reason to have
>>>> this 64-bit wide.
>>> This is a vestigial remnant which escaped the "use vmtrace uniformly" work.
>>>
>>> It should match the domctl_vmtrace_output_position() format, so each
>>> interface gives the same content for the attempted-platform-neutral version.
>> From the vm_event ABI perspective it's simpler to have a 64-bit value
>> here even if the max value it may possibly carry is never going to use
>> the whole 64-bit width. I rather not play with shortening it just to
>> add padding somewhere else.
>>
>> As for what it's called is not that important from my perspective,
>> vmtrace_pos or something like that for example is fine by me.
> The important thing to me is that the comment not be misleading.
> Whether that's arranged for by adjusting the comment of the
> commented declaration is up to what you deem better, i.e. I
> understand the comment it is.

Please see v8.  I rewrote the comment.

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 848d69c1b0..09dfc0924e 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -251,6 +251,9 @@  void vm_event_fill_regs(vm_event_request_t *req)
 
     req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
     req->data.regs.x86.dr6 = ctxt.dr6;
+
+    if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.pt_offset) != 1 )
+        req->data.regs.x86.pt_offset = ~0;
 #endif
 }
 
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 141ea024a3..57f34bf902 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -223,6 +223,12 @@  struct vm_event_regs_x86 {
      */
     uint64_t npt_base;
 
+    /*
+     * Current offset in the Processor Trace buffer. For Intel Processor Trace
+     * this is MSR_RTIT_OUTPUT_MASK. Set to ~0 if no Processor Trace is active.
+     */
+    uint64_t pt_offset;
+
     uint32_t cs_base;
     uint32_t ss_base;
     uint32_t ds_base;