Re: [PATCH v8 12/16] xen/domctl: Add XEN_DOMCTL_vmtrace_op

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 01, 2021 at 01:00:47PM +0000, Andrew Cooper wrote:
> On 01/02/2021 12:01, Roger Pau Monné wrote:
> > On Sat, Jan 30, 2021 at 02:58:48AM +0000, Andrew Cooper wrote:
> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> >> index 12b961113e..a64c4e4177 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> @@ -2261,6 +2261,157 @@ static bool vmx_get_pending_event(struct vcpu *v, 
> >> struct x86_event *info)
> >>      return true;
> >>  }
> >>  
> >> +/*
> >> + * We only let vmtrace agents see and modify a subset of bits in 
> >> MSR_RTIT_CTL.
> >> + * These all pertain to data-emitted into the trace buffer(s).  Must not
> >> + * include controls pertaining to the structure/position of the trace
> >> + * buffer(s).
> >> + */
> >> +#define RTIT_CTL_MASK                                                   \
> >> +    (RTIT_CTL_TRACE_EN | RTIT_CTL_OS | RTIT_CTL_USR | RTIT_CTL_TSC_EN | \
> >> +     RTIT_CTL_DIS_RETC | RTIT_CTL_BRANCH_EN)
> >> +
> >> +/*
> >> + * Status bits restricted to the first-gen subset (i.e. no further CPUID
> >> + * requirements.)
> >> + */
> >> +#define RTIT_STATUS_MASK                                                \
> >> +    (RTIT_STATUS_FILTER_EN | RTIT_STATUS_CONTEXT_EN | 
> >> RTIT_STATUS_TRIGGER_EN | \
> >> +     RTIT_STATUS_ERROR | RTIT_STATUS_STOPPED)
> >> +
> >> +static int vmtrace_get_option(struct vcpu *v, uint64_t key, uint64_t 
> >> *output)
> >> +{
> >> +    const struct vcpu_msrs *msrs = v->arch.msrs;
> >> +
> >> +    switch ( key )
> >> +    {
> >> +    case MSR_RTIT_OUTPUT_MASK:
> > Is there any value in returning the raw value of this MSR instead of
> > just using XEN_DOMCTL_vmtrace_output_position?
> 
> Yes, but for interface reasons.
> 
> There are deliberately some common interfaces (for the subset of options
> expected to be useful), and some platform-specific ones (because there's
> no possible way we encode all of the options in some "common" interface).
> 
> Yes - there is some overlap between the two sets - that is unavoidable
> IMO.  A user of this interface already needs platform specific knowledge
> because it has to interpret the contents of the trace buffer.
> 
> Future extensions to this interface would be setting up the CR3 filter
> and range filters, which definitely shouldn't be common, and can be
> added without new subops in the current model.
> 
> > The size of the buffer should be known to user-space, and then setting
> > the offset could be done by adding a XEN_DOMCTL_vmtrace_set_output_position?
> >
> > Also the contents of this MSR depend on whether ToPA mode is used, and
> > that's not under the control of the guest. So if Xen is switched to
> > use ToPA mode at some point the value of this MSR might not be what a
> > user of the interface expects.
> >
> > From an interface PoV it might be better to offer:
> >
> > XEN_DOMCTL_vmtrace_get_limit
> > XEN_DOMCTL_vmtrace_get_output_position
> > XEN_DOMCTL_vmtrace_set_output_position
> >
> > IMO, as that would be compatible with ToPA if we ever switch to it.
> 
> ToPA is definitely more complicated.  We'd need to stitch the disparate
> buffers back together into one logical view, at which point
> get_output_position becomes more complicated.
> 
> As for set_output_position, that's not useful.  You either want to keep
> the position as-is, or reset back to 0, hence having a platform-neutral
> reset option.
> 
> However, based on this reasoning, I think I should drop access to
> MSR_RTIT_OUTPUT_MASK entirely.  Neither half is useful for userspace to
> access in a platforms-specific way, and disallowing access entirely will
> simplify adding ToPA support in the future.

Exactly. Dropping access to MSR_RTIT_OUTPUT_MASK would indeed solve my
concerns. I somehow assumed that setting the offset was needed for the
users of the interface. With that dropped you can add:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.