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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.01.2021 00:40, Tamas K Lengyel wrote:
> On Fri, Jan 29, 2021 at 6:22 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> 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