Re: [PATCH v8 08/16] xen/domain: Add vmtrace_size domain creation parameter

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Michał Leszczyński" <michal.leszczynski@cert.pl>,
	"Jan Beulich" <JBeulich@suse.com>, "Wei Liu" <wl@xen.org>,
	"Anthony PERARD" <anthony.perard@citrix.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>
Subject: Re: [PATCH v8 08/16] xen/domain: Add vmtrace_size domain creation parameter
Date: Mon, 1 Feb 2021 11:51:40 +0100	[thread overview]
Message-ID: <YBfc+BaXLm5dSvkG@Air-de-Roger> (raw)
In-Reply-To: <20210130025852.12430-9-andrew.cooper3@citrix.com>

On Sat, Jan 30, 2021 at 02:58:44AM +0000, Andrew Cooper wrote:
> From: Michał Leszczyński <michal.leszczynski@cert.pl>
> 
> To use vmtrace, buffers of a suitable size need allocating, and different
> tasks will want different sizes.
> 
> Add a domain creation parameter, and audit it appropriately in the
> {arch_,}sanitise_domain_config() functions.
> 
> For now, the x86 specific auditing is tuned to Processor Trace running in
> Single Output mode, which requires a single contiguous range of memory.
> 
> The size is given an arbitrary limit of 64M which is expected to be enough for
> anticipated usecases, but not large enough to get into long-running-hypercall
> problems.
> 
> Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index d1e94d88cf..491b32812e 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -132,6 +132,71 @@ static void vcpu_info_reset(struct vcpu *v)
>      v->vcpu_info_mfn = INVALID_MFN;
>  }
>  
> +static void vmtrace_free_buffer(struct vcpu *v)
> +{
> +    const struct domain *d = v->domain;
> +    struct page_info *pg = v->vmtrace.pg;
> +    unsigned int i;
> +
> +    if ( !pg )
> +        return;
> +
> +    v->vmtrace.pg = NULL;
> +
> +    for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
> +    {
> +        put_page_alloc_ref(&pg[i]);
> +        put_page_and_type(&pg[i]);
> +    }
> +}
> +
> +static int vmtrace_alloc_buffer(struct vcpu *v)

You might as well make this return true/false, as the error code is
ignored by the caller (at least in this patch).

Thanks, Roger.