On Sat, Jan 30, 2021 at 02:58:42AM +0000, Andrew Cooper wrote:
> A guest's default number of grant frames is 64, and XENMEM_acquire_resource
> will reject an attempt to map more than 32 frames. This limit is caused by
> the size of mfn_list[] on the stack.
>
> Fix mapping of arbitrary size requests by looping over batches of 32 in
> acquire_resource(), and using hypercall continuations when necessary.
>
> To start with, break _acquire_resource() out of acquire_resource() to cope
> with type-specific dispatching, and update the return semantics to indicate
> the number of mfns returned. Update gnttab_acquire_resource() and x86's
> arch_acquire_resource() to match these new semantics.
>
> Have do_memory_op() pass start_extent into acquire_resource() so it can pick
> up where it left off after a continuation, and loop over batches of 32 until
> all the work is done, or a continuation needs to occur.
>
> compat_memory_op() is a bit more complicated, because it also has to marshal
> frame_list in the XLAT buffer. Have it account for continuation information
> itself and hide details from the upper layer, so it can marshal the buffer in
> chunks if necessary.
>
> With these fixes in place, it is now possible to map the whole grant table for
> a guest.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
Just one comment/question regarding a continuation below.
I have to admit I had a hard time reviewing this, all this compat code
plus the continuation stuff is quite hard to follow.
> ---
> CC: George Dunlap <george.dun...@eu.citrix.com>
> CC: Ian Jackson <i...@xenproject.org>
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Stefano Stabellini <sstabell...@kernel.org>
> CC: Wei Liu <w...@xen.org>
> CC: Julien Grall <jul...@xen.org>
> CC: Paul Durrant <p...@xen.org>
> CC: Michał Leszczyński <michal.leszczyn...@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudow...@cert.pl>
> CC: Tamas K Lengyel <ta...@tklengyel.com>
>
> v8:
> * nat => cmp change in the start_extent check.
> * Rebase over 'frame' and ARM/IOREQ series.
>
> v3:
> * Spelling fixes
> ---
> xen/common/compat/memory.c | 94 +++++++++++++++++++++++++++-------
> xen/common/grant_table.c | 3 ++
> xen/common/memory.c | 124
> +++++++++++++++++++++++++++++++++------------
> 3 files changed, 169 insertions(+), 52 deletions(-)
>
> diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
> index 834c5e19d1..4c9cd9c05a 100644
> --- 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 =
Could be made const static I think?
> + (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) /
> + sizeof(*xen_frame_list);
> +
> + if ( start_extent >= cmp.mar.nr_frames )
> + return -EINVAL;
> +
> + /*
> + * Adjust nat to account for work done on previous
> + * continuations, leaving cmp pristine. Hide the
> continaution
> + * from the native code to prevent double accounting.
> + */
> + nat.mar->nr_frames -= start_extent;
> + nat.mar->frame += start_extent;
> + cmd &= MEMOP_CMD_MASK;
> +
> + /*
> + * If there are two many frames to fit within the xlat
> buffer,
> + * we'll need to loop to marshal them all.
> + */
> + nat.mar->nr_frames = min(nat.mar->nr_frames,
> xlat_max_frames);
> +
> /*
> * frame_list is an input for translated guests, and an
> output
> * for untranslated guests. Only copy in for translated
> guests.
> @@ -444,14 +453,14 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> cmp.mar.nr_frames) ||
> __copy_from_compat_offset(
> compat_frame_list, cmp.mar.frame_list,
> - 0, cmp.mar.nr_frames) )
> + start_extent, nat.mar->nr_frames) )
> return -EFAULT;
>
> /*
> * Iterate backwards over compat_frame_list[] expanding
> * compat_pfn_t to xen_pfn_t in place.
> */
> - for ( int x = cmp.mar.nr_frames - 1; x >= 0; --x )
> + for ( int x = nat.mar->nr_frames - 1; x >= 0; --x )
> xen_frame_list[x] = compat_frame_list[x];
Unrelated question, but I don't really see the point of iterating
backwards, wouldn't it be easy to use use the existing 'i' loop
counter and for a for ( i = 0; i < nat.mar->nr_frames; i++ )?
(Not that you need to fix it here, just curious about why we use that
construct instead).
> }
> }
> @@ -600,9 +609,11 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> case XENMEM_acquire_resource:
> {
> DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
> + unsigned int done;
>
> if ( compat_handle_is_null(cmp.mar.frame_list) )
> {
> + ASSERT(split == 0 && rc == 0);
> if ( __copy_field_to_guest(
> guest_handle_cast(compat,
> compat_mem_acquire_resource_t),
> @@ -611,6 +622,21 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> break;
> }
>
> + if ( split < 0 )
> + {
> + /* Continuation occurred. */
> + ASSERT(rc != XENMEM_acquire_resource);
> + done = cmd >> MEMOP_EXTENT_SHIFT;
> + }
> + else
> + {
> + /* No continuation. */
> + ASSERT(rc == 0);
> + done = nat.mar->nr_frames;
> + }
> +
> + ASSERT(done <= nat.mar->nr_frames);
> +
> /*
> * frame_list is an input for translated guests, and an output
> for
> * untranslated guests. Only copy out for untranslated guests.
> @@ -626,7 +652,7 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> */
> BUILD_BUG_ON(sizeof(compat_pfn_t) > sizeof(xen_pfn_t));
>
> - for ( i = 0; i < cmp.mar.nr_frames; i++ )
> + for ( i = 0; i < done; i++ )
> {
> compat_pfn_t frame = xen_frame_list[i];
>
> @@ -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,
> + if ( __copy_to_compat_offset(cmp.mar.frame_list,
> start_extent,
> compat_frame_list,
> - cmp.mar.nr_frames) )
> + done) )
> return -EFAULT;
Is it fine to return with a possibly pending continuation already
encoded here?
Other places seem to crash the domain when that happens.
Thanks, Roger.
