From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
"George Dunlap" <George.Dunlap@eu.citrix.com>,
"Ian Jackson" <iwj@xenproject.org>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Wei Liu" <wl@xen.org>, "Julien Grall" <julien@xen.org>,
"Paul Durrant" <paul@xen.org>,
"Michał Leszczyński" <michal.leszczynski@cert.pl>,
"Hubert Jasudowicz" <hubert.jasudowicz@cert.pl>,
"Tamas K Lengyel" <tamas@tklengyel.com>
Subject: Re: [PATCH v2 09/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource
Date: Mon, 28 Sep 2020 11:37:53 +0200 [thread overview]
Message-ID: <eb9768ad-b6cd-b01e-d689-63775f6e5914@suse.com> (raw)
In-Reply-To: <20200922182444.12350-10-andrew.cooper3@citrix.com>
On 22.09.2020 20:24, Andrew Cooper wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4632,7 +4632,6 @@ int arch_acquire_resource(struct domain *d, unsigned int type,
> if ( id != (unsigned int)ioservid )
> break;
>
> - rc = 0;
> for ( i = 0; i < nr_frames; i++ )
> {
> mfn_t mfn;
> @@ -4643,6 +4642,9 @@ int arch_acquire_resource(struct domain *d, unsigned int type,
>
> mfn_list[i] = mfn_x(mfn);
> }
> + if ( i == nr_frames )
> + /* Success. Passed nr_frames back to the caller. */
> + rc = nr_frames;
With this, shouldn't the return type of the function be changed to
"long"? I realize that's no an issue with XENMEM_resource_ioreq_server
specifically, but I mean the general case.
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -402,23 +402,10 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
> case XENMEM_acquire_resource:
> {
> xen_pfn_t *xen_frame_list = NULL;
> - unsigned int max_nr_frames;
>
> if ( copy_from_guest(&cmp.mar, compat, 1) )
> return -EFAULT;
>
> - /*
> - * The number of frames handled is currently limited to a
> - * small number by the underlying implementation, so the
> - * scratch space should be sufficient for bouncing the
> - * frame addresses.
> - */
> - max_nr_frames = (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) /
> - sizeof(*xen_frame_list);
> -
> - if ( cmp.mar.nr_frames > max_nr_frames )
> - return -E2BIG;
> -
> /* Marshal the frame list in the remainder of the xlat space. */
> if ( !compat_handle_is_null(cmp.mar.frame_list) )
> xen_frame_list = (xen_pfn_t *)(nat.mar + 1);
> @@ -432,6 +419,28 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>
> if ( xen_frame_list && cmp.mar.nr_frames )
> {
> + unsigned int xlat_max_frames =
> + (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) /
> + sizeof(*xen_frame_list);
> +
> + if ( start_extent >= nat.mar->nr_frames )
> + return -EINVAL;
Like for patch 2, I don't see why the == case should result in an
error, at the very least when start_extent is zero.
> @@ -611,6 +622,21 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
> break;
> }
>
> + if ( split < 0 )
> + {
> + /* Contintuation occured. */
Nit: Stray 't'. And missing 'r'?
> @@ -636,15 +662,45 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
> compat_frame_list[i] = frame;
> }
>
> - if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
> - compat_frame_list,
> - cmp.mar.nr_frames) )
> + if ( __copy_to_compat_offset(
> + cmp.mar.frame_list, start_extent,
> + compat_frame_list, done) )
> return -EFAULT;
> }
> - break;
> +
> + start_extent += done;
> +
> + /* Completely done. */
> + if ( start_extent == cmp.mar.nr_frames )
> + break;
> +
> + /*
> + * Done a "full" batch, but we were limited by space in the xlat
> + * area. Go around the loop again without necesserily returning
> + * to guest context.
> + */
> + if ( done == nat.mar->nr_frames )
> + {
> + split = 1;
> + break;
> + }
> +
> + /* Explicit continuation request from a higher level. */
> + if ( done < nat.mar->nr_frames )
> + return hypercall_create_continuation(
> + __HYPERVISOR_memory_op, "ih",
> + op | (start_extent << MEMOP_EXTENT_SHIFT), compat);
> +
> + /*
> + * Well... Somethings gone wrong with the two levels of chunking.
> + * My condolences to whomever next has to debug this mess.
> + */
Any suggestion how to overcome this "mess"?
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4105,6 +4105,9 @@ int gnttab_acquire_resource(
> for ( i = 0; i < nr_frames; ++i )
> mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
>
> + /* Success. Passed nr_frames back to the caller. */
Nit: "Pass"?
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1027,17 +1027,31 @@ static unsigned int resource_max_frames(struct domain *d,
> }
> }
>
> +/*
> + * Returns -errno on error, or positive in the range [1, nr_frames] on
> + * success. Returning less than nr_frames contitutes a request for a
> + * continuation.
> + */
> +static int _acquire_resource(
> + struct domain *d, unsigned int type, unsigned int id, unsigned long frame,
> + unsigned int nr_frames, xen_pfn_t mfn_list[])
As per the comment the return type may again want to be "long" here.
Albeit I realize the restriction to (UINT_MAX >> MEMOP_EXTENT_SHIFT)
makes this (and the other place above) only a latent issue for now,
so it may well be fine to be left as is.
> @@ -1087,26 +1098,47 @@ static int acquire_resource(
> goto out;
> }
>
> + /*
> + * Limiting nr_frames at (UINT_MAX >> MEMOP_EXTENT_SHIFT) isn't ideal. If
> + * it ever becomes a practical problem, we can switch to mutating
> + * xmar.{frame,nr_frames,frame_list} in guest memory.
> + */
For 64-bit, extending the limit to ULONG_MAX >> MEMOP_EXTENT_SHIFT
may also be an option.
> + rc = -EINVAL;
> + if ( start_extent >= xmar.nr_frames ||
Again, at least when start_extent is zero, == should not result in an
error.
> + xmar.nr_frames > (UINT_MAX >> MEMOP_EXTENT_SHIFT) )
> + goto out;
> +
> + /* Adjust for work done on previous continuations. */
> + xmar.nr_frames -= start_extent;
> + xmar.frame += start_extent;
> + guest_handle_add_offset(xmar.frame_list, start_extent);
> +
> do {
> - switch ( xmar.type )
> - {
> - case XENMEM_resource_grant_table:
> - rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, xmar.nr_frames,
> - mfn_list);
> - break;
> + /*
> + * Arbitrary size. Not too much stack space, and a reasonable stride
> + * for continutation checks.
Nit: Stray 't' again.
> @@ -1126,7 +1158,32 @@ static int acquire_resource(
> rc = -EIO;
> }
> }
> - } while ( 0 );
> +
> + if ( rc )
> + goto out;
> +
> + xmar.nr_frames -= done;
> + xmar.frame += done;
> + guest_handle_add_offset(xmar.frame_list, done);
> + start_extent += done;
> +
> + /*
> + * Explicit contination request from _acquire_resource(), or we've
Nit: Missing 'u' this time round.
Jan
next prev parent reply other threads:[~2020-09-28 9:38 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-22 18:24 [PATCH v2 00/11] Multiple fixes to XENMEM_acquire_resource Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 01/11] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 02/11] xen/gnttab: Rework resource acquisition Andrew Cooper
2020-09-24 9:51 ` Paul Durrant
2021-01-11 21:22 ` Andrew Cooper
2021-01-12 8:23 ` Jan Beulich
2021-01-12 20:06 ` Andrew Cooper
2021-01-12 8:29 ` Paul Durrant
2020-09-25 13:17 ` Jan Beulich
2021-01-11 21:22 ` Andrew Cooper
2021-01-12 8:15 ` Jan Beulich
2021-01-12 18:11 ` Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 03/11] xen/memory: Fix compat XENMEM_acquire_resource for size requests Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics Andrew Cooper
2020-09-24 10:06 ` Paul Durrant
2020-09-24 10:57 ` Andrew Cooper
2020-09-24 11:04 ` Paul Durrant
2020-09-25 15:56 ` Jan Beulich
2020-09-22 18:24 ` [PATCH v2 05/11] tools/foreignmem: Support querying the size of a resource Andrew Cooper
2021-01-08 17:52 ` Andrew Cooper
2021-01-11 10:50 ` Roger Pau Monné
2021-01-11 15:00 ` Andrew Cooper
2021-01-11 15:26 ` [PATCH v3 " Andrew Cooper
2021-01-11 15:54 ` Roger Pau Monné
2020-09-22 18:24 ` [PATCH v2 06/11] xen/memory: Clarify the XENMEM_acquire_resource ABI description Andrew Cooper
2020-09-24 10:08 ` Paul Durrant
2020-09-22 18:24 ` [PATCH v2 07/11] xen/memory: Improve compat XENMEM_acquire_resource handling Andrew Cooper
2020-09-24 10:16 ` Paul Durrant
2020-09-28 9:09 ` Jan Beulich
2021-01-08 18:57 ` Andrew Cooper
2021-01-11 14:25 ` Jan Beulich
2020-09-22 18:24 ` [PATCH v2 08/11] xen/memory: Indent part of acquire_resource() Andrew Cooper
2020-09-24 10:36 ` Paul Durrant
2020-09-22 18:24 ` [PATCH v2 09/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource Andrew Cooper
2020-09-24 10:47 ` Paul Durrant
2021-01-08 19:36 ` Andrew Cooper
2020-09-28 9:37 ` Jan Beulich [this message]
2021-01-11 20:05 ` Andrew Cooper
2021-01-11 22:36 ` Andrew Cooper
2021-01-12 8:39 ` Jan Beulich
2020-09-22 18:24 ` [PATCH v2 10/11] TESTING dom0 Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 11/11] TESTING XTF Andrew Cooper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eb9768ad-b6cd-b01e-d689-63775f6e5914@suse.com \
--to=jbeulich@suse.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=hubert.jasudowicz@cert.pl \
--cc=iwj@xenproject.org \
--cc=julien@xen.org \
--cc=michal.leszczynski@cert.pl \
--cc=paul@xen.org \
--cc=sstabellini@kernel.org \
--cc=tamas@tklengyel.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).