Re: [PATCH v6 04/11] common: add vmtrace_pt_size domain parameter

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

On Tue, Jul 07, 2020 at 09:39:43PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> 
> Add vmtrace_pt_size domain parameter in live domain and
> vmtrace_pt_order parameter in xen_domctl_createdomain.
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> ---
>  xen/arch/x86/domain.c       | 6 ++++++
>  xen/common/domain.c         | 9 +++++++++
>  xen/include/public/domctl.h | 1 +
>  xen/include/xen/sched.h     | 3 +++
>  4 files changed, 19 insertions(+)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index fee6c3931a..b75017b28b 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -499,6 +499,12 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>           */
>          config->flags |= XEN_DOMCTL_CDF_oos_off;
>  
> +    if ( !hvm && config->processor_trace_buf_kb )
> +    {
> +        dprintk(XENLOG_INFO, "Processor trace is not supported on 
> non-HVM\n");
> +        return -EINVAL;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index a45cf023f7..e6e8f88da1 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -338,6 +338,12 @@ static int sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    if ( config->processor_trace_buf_kb && !vmtrace_supported )
> +    {
> +        dprintk(XENLOG_INFO, "Processor tracing is not supported\n");
> +        return -EINVAL;
> +    }
> +
>      return arch_sanitise_domain_config(config);
>  }
>  
> @@ -443,6 +449,9 @@ struct domain *domain_create(domid_t domid,
>          d->nr_pirqs = min(d->nr_pirqs, nr_irqs);
>  
>          radix_tree_init(&d->pirq_tree);
> +
> +        if ( config->processor_trace_buf_kb )
> +            d->processor_trace_buf_kb = config->processor_trace_buf_kb;
>      }
>  
>      if ( (err = arch_domain_create(d, config)) != 0 )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 59bdc28c89..7681675a94 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -92,6 +92,7 @@ struct xen_domctl_createdomain {
>      uint32_t max_evtchn_port;
>      int32_t max_grant_frames;
>      int32_t max_maptrack_frames;
> +    uint32_t processor_trace_buf_kb;
>  
>      struct xen_arch_domainconfig arch;
>  };
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ac53519d7f..c046e59886 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -457,6 +457,9 @@ struct domain
>      unsigned    pbuf_idx;
>      spinlock_t  pbuf_lock;
>  
> +    /* Used by vmtrace features */
> +    uint32_t    processor_trace_buf_kb;

Any reason for adding it here instead of doing it at the end of the
struct? Also I think this should be unsigned int, there's no reason to
use a specific width type here.

IMO it would be nice to have a Kconfig option for this, so that it can
be build time disabled, and for example disabled by default on Arm (or
on x86 if HVM is not built).

Most recently added features have followed this model for example
Argo, where a Kconfig option is added in order to build time disable
the added feature. Let me know if you need some tips about it, I'm
happy to help in order to try to get this in Kconfig.

Thanks, Roger.