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

[Julien Grall <julien@xen.org>]

Hi,

On 05/07/2020 20:02, Michał Leszczyński wrote:
> ----- 5 lip 2020 o 20:54, Michał Leszczyński michal.leszczynski@cert.pl napisał(a):
> 
>> From: Michal Leszczynski <michal.leszczynski@cert.pl>
>>
>> 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@cert.pl>
>> ---
>> docs/man/xl.cfg.5.pod.in             | 11 +++++++++++
>> 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          |  2 ++
>> tools/xl/xl_parse.c                  | 22 ++++++++++++++++++++++
>> 7 files changed, 47 insertions(+)
>>
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index 0532739c1f..670759f6bd 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -278,6 +278,17 @@ memory=8096 will report significantly less memory available
>> for use
>> than a system with maxmem=8096 memory=8096 due to the memory overhead
>> of having to track the unused pages.
>>
>> +=item B<processor_trace_buffer_size=BYTES>
>> +
>> +Specifies the size of processor trace buffer that would be allocated
>> +for each vCPU belonging to this domain. Disabled (i.e.
>> +B<processor_trace_buffer_size=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>: The size value must be between 4 kB and 4 GB and it must
>> +be also a power of 2.

This seems to suggest that 4 kB is allowed. But looking at the code 
below, you are forbidding the value.

[...]

> As there were many different ideas about how the naming scheme should be
> and what kinds of values should be passed where, I would like to discuss
> this particular topic. Right now we have it pretty confusing:
> 
> * user sets "processor_trace_buffer_size" option in xl.cfg
> * domain creation hypercall uses "vmtrace_pt_order" (derived from above)

You don't only use the order in the hypercall but also the public 
interface of libxl.

> * hypervisor side stores "vmtrace_pt_size" (converted back to bytes)

My preference would be to use the size everywhere, but if one still 
prefer to use the order in the hypercall then the libxl interface should 
use the size.

See my comment in v4 for the rationale.

Cheers,

-- 
Julien Grall