Re: [PATCH v6 08/11] x86/mm: add vmtrace_buf resource type

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: "Michał Leszczyński" <michal.leszczynski@cert.pl>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	luwei.kang@intel.com, tamas.lengyel@intel.com,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v6 08/11] x86/mm: add vmtrace_buf resource type
Date: Wed, 15 Jul 2020 19:20:11 +0200	[thread overview]
Message-ID: <20200715172011.GF7191@Air-de-Roger> (raw)
In-Reply-To: <2129d21e7ef7e960951a8baafab01d9392dff8f3.1594150543.git.michal.leszczynski@cert.pl>

On Tue, Jul 07, 2020 at 09:39:47PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@cert.pl>
> 
> Allow to map processor trace buffer using
> acquire_resource().
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>
> ---
>  xen/common/memory.c         | 31 +++++++++++++++++++++++++++++++
>  xen/include/public/memory.h |  1 +
>  2 files changed, 32 insertions(+)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index eb42f883df..c0a22eb60f 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1007,6 +1007,32 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
>  
> +static int acquire_vmtrace_buf(struct domain *d, unsigned int id,
> +                               uint64_t frame,
> +                               uint64_t nr_frames,

frame and nr_frames should be unsigned int, as I think they can't
overflow an unsigned integer?

> +                               xen_pfn_t mfn_list[])
> +{
> +    mfn_t mfn;
> +    unsigned int i;
> +    uint64_t size;
> +    struct vcpu *v = domain_vcpu(d, id);
> +
> +    if ( !v || !v->vmtrace.pt_buf )
> +        return -EINVAL;
> +
> +    mfn = page_to_mfn(v->vmtrace.pt_buf);
> +    size = v->domain->processor_trace_buf_kb * KB(1);

I think this should be the number of pages, and then you want to
directly access d, there's no need to do v->domain->...

> +
> +    if ( (frame > (size >> PAGE_SHIFT)) ||
> +         (nr_frames > ((size >> PAGE_SHIFT) - frame)) )

Isn't this off by one (should use >=)? If size is 8 pages, you can get
pages [0,7], but requesting page 8 would be out of the range?

> +        return -EINVAL;
> +
> +    for ( i = 0; i < nr_frames; i++ )
> +        mfn_list[i] = mfn_x(mfn_add(mfn, frame + i));

You could drop the mfn variable and just do:

mfn_list[i] = mfn_x(page_to_mfn(v->vmtrace.pt_buf[frame + i]));

Thanks.