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....@citrix.com>
Thanks, Roger.
