Re: [PATCH v6 09/11] x86/domctl: add XEN_DOMCTL_vmtrace_op

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

On Tue, Jul 07, 2020 at 09:39:48PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> 
> Implement domctl to manage the runtime state of
> processor trace feature.
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> ---
>  xen/arch/x86/domctl.c       | 50 +++++++++++++++++++++++++++++++++++++
>  xen/include/public/domctl.h | 28 +++++++++++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 6f2c69788d..6132499db4 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -322,6 +322,50 @@ 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)
> +{
> +    int rc;
> +    struct vcpu *v;
> +
> +    if ( op->pad1 || op->pad2 )
> +        return -EINVAL;
> +
> +    if ( !vmtrace_supported )
> +        return -EOPNOTSUPP;
> +
> +    if ( !is_hvm_domain(d) )
> +        return -EOPNOTSUPP;

You can join both if into a single one if they both return
-EOPNOTSUPP.

> +
> +    if ( op->vcpu >= d->max_vcpus )
> +        return -EINVAL;

You can remove this check and just use the return value of domain_vcpu
instead, if it's NULL the vCPU ID is wrong.

> +
> +    v = domain_vcpu(d, op->vcpu);
> +    rc = 0;

No need to init rc to 0, as it would be unconditionally initialized in
the switch below due to the default case.

> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_vmtrace_pt_enable:
> +    case XEN_DOMCTL_vmtrace_pt_disable:
> +        vcpu_pause(v);
> +        rc = vmtrace_control_pt(v, op->cmd == XEN_DOMCTL_vmtrace_pt_enable);
> +        vcpu_unpause(v);
> +        break;
> +
> +    case XEN_DOMCTL_vmtrace_pt_get_offset:
> +        rc = vmtrace_get_pt_offset(v, &op->offset, &op->size);

In order to get consistent values here the vCPU should be paused, or
else you just get stale values from the last vmexit?

Maybe that's fine for your use case?

> +
> +        if ( !rc && d->is_dying )
> +            rc = ENODATA;

This check here seems weird, if this is really required shouldn't it
be done before attempting to fetch the data?

> +        break;
> +
> +    default:
> +        rc = -EOPNOTSUPP;
> +    }
> +
> +    return rc;
> +}
> +
>  #define MAX_IOPORTS 0x10000
>  
>  long arch_do_domctl(
> @@ -337,6 +381,12 @@ long arch_do_domctl(
>      switch ( domctl->cmd )
>      {
>  
> +    case XEN_DOMCTL_vmtrace_op:
> +        ret = do_vmtrace_op(d, &domctl->u.vmtrace_op, u_domctl);
> +        if ( !ret )
> +            copyback = true;
> +        break;

Nit: new additions would usually got at the end of the switch.

> +
>      case XEN_DOMCTL_shadow_op:
>          ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
>          if ( ret == -ERESTART )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 7681675a94..73c7ccbd16 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1136,6 +1136,30 @@ struct xen_domctl_vuart_op {
>                                   */
>  };
>  
> +/* XEN_DOMCTL_vmtrace_op: Perform VM tracing related operation */
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
> +struct xen_domctl_vmtrace_op {
> +    /* IN variable */
> +    uint32_t cmd;
> +/* Enable/disable external vmtrace for given domain */
> +#define XEN_DOMCTL_vmtrace_pt_enable      1
> +#define XEN_DOMCTL_vmtrace_pt_disable     2
> +#define XEN_DOMCTL_vmtrace_pt_get_offset  3
> +    domid_t domain;
> +    uint16_t pad1;
> +    uint32_t vcpu;
> +    uint16_t pad2;

pad2 should be a uint32_t? Or else there's a padding field added by
the compiler? (maybe I'm missing something, I haven't checked with
pahole).

> +
> +    /* OUT variable */
> +    uint64_aligned_t size;
> +    uint64_aligned_t offset;
> +};
> +typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
> +
> +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> +
>  struct xen_domctl {
>      uint32_t cmd;
>  #define XEN_DOMCTL_createdomain                   1
> @@ -1217,6 +1241,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_vuart_op                      81
>  #define XEN_DOMCTL_get_cpu_policy                82
>  #define XEN_DOMCTL_set_cpu_policy                83
> +#define XEN_DOMCTL_vmtrace_op                    84
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> @@ -1277,6 +1302,9 @@ struct xen_domctl {
>          struct xen_domctl_monitor_op        monitor_op;
>          struct xen_domctl_psr_alloc         psr_alloc;
>          struct xen_domctl_vuart_op          vuart_op;
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +        struct xen_domctl_vmtrace_op        vmtrace_op;
> +#endif

No need for the preprocessor conditionals here, the whole domctl.h is
only to be used by the tools or Xen, and is already properly guarded
(see the #error at the top of the file).

Thanks.