> -----Original Message-----
> From: Andrew Cooper <andrew.coop...@citrix.com>
> Sent: 22 September 2020 19:25
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@eu.citrix.com>; Ian
> Jackson <i...@xenproject.org>; Jan Beulich <jbeul...@suse.com>; Stefano
> Stabellini
> <sstabell...@kernel.org>; Wei Liu <w...@xen.org>; Julien Grall
> <jul...@xen.org>; Paul Durrant
> <p...@xen.org>; Michał Leszczyński <michal.leszczyn...@cert.pl>; Hubert
> Jasudowicz
> <hubert.jasudow...@cert.pl>; Tamas K Lengyel <ta...@tklengyel.com>
> Subject: [PATCH v2 07/11] xen/memory: Improve compat XENMEM_acquire_resource
> handling
>
> The frame_list is an input, or an output, depending on whether the calling
> domain is translated or not. The array does not need marshalling in both
> directions.
>
> Furthermore, the copy-in loop was very inefficient, copying 4 bytes at at
> time. Rewrite it to copy in all nr_frames at once, and then expand
> compat_pfn_t to xen_pfn_t in place.
>
> Re-position the copy-in loop to simplify continuation support in a future
> patch, and reduce the scope of certain variables.
>
> No change in guest observed behaviour.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> ---
> 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>
> ---
> xen/common/compat/memory.c | 65
> ++++++++++++++++++++++++++++------------------
> 1 file changed, 40 insertions(+), 25 deletions(-)
>
> diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
> index ed92e05b08..834c5e19d1 100644
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -55,6 +55,8 @@ static int get_reserved_device_memory(xen_pfn_t start,
> xen_ulong_t nr,
>
> int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
> {
> + struct vcpu *curr = current;
> + struct domain *currd = curr->domain;
> int split, op = cmd & MEMOP_CMD_MASK;
> long rc;
> unsigned int start_extent = cmd >> MEMOP_EXTENT_SHIFT;
> @@ -399,7 +401,7 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
>
> case XENMEM_acquire_resource:
> {
> - xen_pfn_t *xen_frame_list;
> + xen_pfn_t *xen_frame_list = NULL;
> unsigned int max_nr_frames;
>
> if ( copy_from_guest(&cmp.mar, compat, 1) )
> @@ -417,28 +419,10 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> if ( cmp.mar.nr_frames > max_nr_frames )
> return -E2BIG;
>
> - if ( compat_handle_is_null(cmp.mar.frame_list) )
> - xen_frame_list = NULL;
> - else
> - {
> + /* 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);
>
> - if ( !compat_handle_okay(cmp.mar.frame_list,
> - cmp.mar.nr_frames) )
> - return -EFAULT;
> -
> - for ( i = 0; i < cmp.mar.nr_frames; i++ )
> - {
> - compat_pfn_t frame;
> -
> - if ( __copy_from_compat_offset(
> - &frame, cmp.mar.frame_list, i, 1) )
> - return -EFAULT;
> -
> - xen_frame_list[i] = frame;
> - }
> - }
> -
> #define XLAT_mem_acquire_resource_HNDL_frame_list(_d_, _s_) \
> set_xen_guest_handle((_d_)->frame_list, xen_frame_list)
>
> @@ -446,6 +430,31 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
>
> #undef XLAT_mem_acquire_resource_HNDL_frame_list
>
> + if ( xen_frame_list && cmp.mar.nr_frames )
> + {
> + /*
> + * frame_list is an input for translated guests, and an
> output
> + * for untranslated guests. Only copy in for translated
> guests.
> + */
> + if ( paging_mode_translate(currd) )
> + {
> + compat_pfn_t *compat_frame_list = (void *)xen_frame_list;
> +
> + if ( !compat_handle_okay(cmp.mar.frame_list,
> + cmp.mar.nr_frames) ||
> + __copy_from_compat_offset(
> + compat_frame_list, cmp.mar.frame_list,
> + 0, cmp.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 )
I know it is legal c99 but personally I still dislike declarations like this,
and I've not seen one elsewhere in the Xen code. But I'm not the maintainer,
so...
Reviewed-by: Paul Durrant <p...@xen.org>
> + xen_frame_list[x] = compat_frame_list[x];
> + }
> + }
> break;
> }
> default:
> @@ -590,8 +599,6 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
>
> case XENMEM_acquire_resource:
> {
> - const xen_pfn_t *xen_frame_list = (xen_pfn_t *)(nat.mar + 1);
> - compat_pfn_t *compat_frame_list = (compat_pfn_t *)(nat.mar + 1);
> DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
>
> if ( compat_handle_is_null(cmp.mar.frame_list) )
> @@ -601,9 +608,18 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> compat_mem_acquire_resource_t),
> nat.mar, nr_frames) )
> return -EFAULT;
> + break;
> }
> - else
> +
> + /*
> + * frame_list is an input for translated guests, and an output
> for
> + * untranslated guests. Only copy out for untranslated guests.
> + */
> + if ( !paging_mode_translate(currd) )
> {
> + const xen_pfn_t *xen_frame_list = (xen_pfn_t *)(nat.mar + 1);
> + compat_pfn_t *compat_frame_list = (compat_pfn_t *)(nat.mar +
> 1);
> +
> /*
> * NOTE: the smaller compat array overwrites the native
> * array.
> @@ -625,7 +641,6 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> cmp.mar.nr_frames) )
> return -EFAULT;
> }
> -
> break;
> }
>
> --
> 2.11.0
