Re: [PATCH v6 05/11] tools/libxl: add vmtrace_pt_size parameter

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

On Tue, Jul 07, 2020 at 09:39:44PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> 
> Allow to specify the size of per-vCPU trace buffer upon
> domain creation. This is zero by default (meaning: not enabled).
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> ---
>  docs/man/xl.cfg.5.pod.in             | 13 +++++++++++++
>  tools/golang/xenlight/helpers.gen.go |  2 ++
>  tools/golang/xenlight/types.gen.go   |  1 +
>  tools/libxl/libxl.h                  |  8 ++++++++
>  tools/libxl/libxl_create.c           |  1 +
>  tools/libxl/libxl_types.idl          |  4 ++++
>  tools/xl/xl_parse.c                  | 22 ++++++++++++++++++++++
>  7 files changed, 51 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 0532739c1f..ddef9b6014 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -683,6 +683,19 @@ If this option is not specified then it will default to 
> B<false>.
>  
>  =back
>  
> +=item B<processor_trace_buf_kb=KBYTES>
> +
> +Specifies the size of processor trace buffer that would be allocated
> +for each vCPU belonging to this domain. Disabled (i.e.
> +B<processor_trace_buf_kb=0> by default. This must be set to
> +non-zero value in order to be able to use processor tracing features
> +with this domain.
> +
> +B<NOTE>: In order to use Intel Processor Trace feature, this value
> +must be between 8 kB and 4 GB and it must be a power of 2.

I think the minimum that we could support is 4KB? (ie: one page?). Not
that it matters much, as I don't think anyone would use such small
buffers.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 9d3f05f399..748fde65ab 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -645,6 +645,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      # supported by x86 HVM and ARM support is planned.
>      ("altp2m", libxl_altp2m_mode),
>  
> +    # Size of preallocated processor trace buffers (in KBYTES).
> +    # Use zero value to disable this feature.
> +    ("processor_trace_buf_kb", integer),

MemKB should be used here instead of integer.

> +
>      ], dir=DIR_IN,
>         copy_deprecated_fn="libxl__domain_build_info_copy_deprecated",
>  )
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 61b4ef7b7e..87e373b413 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1861,6 +1861,28 @@ void parse_config_data(const char *config_source,
>          }
>      }
>  
> +    if (!xlu_cfg_get_long(config, "processor_trace_buf_kb", &l, 1) && l) {
> +        if (l & (l - 1)) {
> +            fprintf(stderr, "ERROR: processor_trace_buf_kb"
> +                            " - must be a power of 2\n");
> +            exit(1);
> +        }
> +
> +        if (l < 8) {
> +            fprintf(stderr, "ERROR: processor_trace_buf_kb"
> +                            " - value is too small\n");
> +            exit(1);
> +        }
> +
> +        if (l > 1024*1024*4) {
> +            fprintf(stderr, "ERROR: processor_trace_buf_kb"
> +                            " - value is too large\n");
> +            exit(1);

Those checks shouldn't be here, this is libxl common code, and those
limitations are specific to the Intel implementation. Those should be
inside of the hypervisor IMO, or if we really want to have them in
libxl for some reason they should be moved to libxl_x86.c

Thanks.