| Message ID | 20210121212718.2441-11-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement support for external IPT monitoring | expand |
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
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
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
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
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 --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;