Re: [PATCH v7 06/10] xen/domctl: Add XEN_DOMCTL_vmtrace_op

[Jan Beulich <jbeulich@suse.com>]

On 21.01.2021 22:27, Andrew Cooper wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -155,6 +155,55 @@ void arch_get_domain_info(const struct domain *d,
>      info->arch_config.emulation_flags = d->arch.emulation_flags;
>  }
>  
> +static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
> +                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    struct vcpu *v;
> +    int rc;
> +
> +    if ( !d->vmtrace_frames || d == current->domain /* No vcpu_pause() */ )
> +        return -EINVAL;
> +
> +    ASSERT(is_hvm_domain(d)); /* Restricted by domain creation logic. */
> +
> +    v = domain_vcpu(d, op->vcpu);
> +    if ( !v )
> +        return -ENOENT;
> +
> +    vcpu_pause(v);
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_vmtrace_enable:
> +    case XEN_DOMCTL_vmtrace_disable:
> +    case XEN_DOMCTL_vmtrace_reset_and_enable:
> +        rc = hvm_vmtrace_control(
> +            v, op->cmd != XEN_DOMCTL_vmtrace_disable,
> +            op->cmd == XEN_DOMCTL_vmtrace_reset_and_enable);
> +        break;
> +
> +    case XEN_DOMCTL_vmtrace_output_position:
> +        rc = hvm_vmtrace_output_position(v, &op->value);
> +        if ( rc >= 0 )
> +            rc = 0;

So vmtrace_output_position() effectively returns a boolean, and
there is no other caller of it afaics. I understand the hook and
function return int to allow for error indicators. But what's
the purpose of returning ipt_active when the only caller doesn't
care?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2261,6 +2261,153 @@ static bool vmx_get_pending_event(struct vcpu *v, struct x86_event *info)
>      return true;
>  }
>  
> +static int vmtrace_get_option(struct vcpu *v, uint64_t key, uint64_t *output)
> +{
> +    const struct vcpu_msrs *msrs = v->arch.msrs;
> +
> +    /*
> +     * We only let vmtrace agents see and modify a subset of bits in
> +     * MSR_RTIT_CTL.  These all pertain to date emitted into the trace

s/date/data/ ?

> +     * 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)

The placement of these two #define-s kind of suggests they're
intended for this function only, but the next one (at least)
also uses them. May I suggest to move these ahead of this
function?

> +static int vmtrace_set_option(struct vcpu *v, uint64_t key, uint64_t value)
> +{
> +    struct vcpu_msrs *msrs = v->arch.msrs;
> +    bool new_en, old_en = msrs->rtit.ctl & RTIT_CTL_TRACE_EN;
> +
> +    switch ( key )
> +    {
> +    case MSR_RTIT_OUTPUT_MASK:
> +        /*
> +         * MSR_RTIT_OUTPUT_MASK, when using Single Output mode, has a limit
> +         * field in the lower 32 bits, and an offset in the upper 32 bits.
> +         *
> +         * Limit is fixed by the vmtrace buffer size and must not be
> +         * controlled by userspace, while offset must be within the limit.
> +         *
> +         * Drop writes to the limit field to simply userspace wanting to reset
> +         * the offset by writing 0.
> +         */
> +        if ( (value >> 32) > msrs->rtit.output_limit )
> +            return -EINVAL;
> +        msrs->rtit.output_offset = value >> 32;
> +        break;
> +
> +    case MSR_RTIT_CTL:
> +        if ( value & ~RTIT_CTL_MASK )
> +            return -EINVAL;
> +
> +        msrs->rtit.ctl &= ~RTIT_CTL_MASK;
> +        msrs->rtit.ctl |= (value & RTIT_CTL_MASK);
> +        break;
> +
> +    case MSR_RTIT_STATUS:
> +        if ( value & ~RTIT_STATUS_MASK )
> +            return -EINVAL;
> +
> +        msrs->rtit.status &= ~RTIT_STATUS_MASK;
> +        msrs->rtit.status |= (value & RTIT_STATUS_MASK);
> +        break;
> +
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    new_en = msrs->rtit.ctl & RTIT_CTL_TRACE_EN;
> +
> +    /* ctl.trace_en changed => update MSR load/save lists appropriately. */
> +    if ( !old_en && new_en )
> +    {
> +        if ( vmx_add_guest_msr(v, MSR_RTIT_CTL, msrs->rtit.ctl) ||
> +             vmx_add_host_load_msr(v, MSR_RTIT_CTL, 0) )
> +        {
> +            /*
> +             * The only failure cases here are failing the
> +             * singleton-per-domain memory allocation, or exceeding the space
> +             * in the allocation.  We could unwind in principle, but there is
> +             * nothing userspace can usefully do to continue using this VM.
> +             */
> +            domain_crash(v->domain);
> +            return -ENXIO;

I don't think I fully agree with the 2nd half of the last
sentence, but well, so be it then for the time being at least.
Why could userspace not decide to continue running this VM
with ipt disabled?

> +static int vmtrace_control(struct vcpu *v, bool enable, bool reset)
> +{
> +    struct vcpu_msrs *msrs = v->arch.msrs;
> +    uint64_t new_ctl;
> +    int rc;
> +
> +    if ( v->arch.hvm.vmx.ipt_active == enable )
> +        return -EINVAL;

Why is XEN_DOMCTL_vmtrace_reset_and_enable not permitted
when ipt_active is true? And, considering ...

> +    if ( reset )
> +    {
> +        msrs->rtit.status = 0;
> +        msrs->rtit.output_offset = 0;
> +    }
> +
> +    new_ctl = msrs->rtit.ctl & ~RTIT_CTL_TRACE_EN;
> +    if ( enable )
> +        new_ctl |= RTIT_CTL_TRACE_EN;
> +
> +    rc = vmtrace_set_option(v, MSR_RTIT_CTL, new_ctl);

... this is just a wrapper around a function directly
reachable via XEN_DOMCTL_vmtrace_set_option, why any
restriction at all?

> +    if ( rc )
> +        return rc;
> +
> +    v->arch.hvm.vmx.ipt_active = enable;

Shouldn't this be done in vmtrace_set_option(), to also
cover the other path leading there?

Jan