Re: [PATCH 7/9] gnttab: no need to translate handle for gnttab_get_status_frames()

[Andrew Cooper]

On 26/08/2021 11:14, Jan Beulich wrote:
> Unlike for GNTTABOP_setup_table native and compat frame lists are arrays

"GNTTABOP_setup_table, native"

But I think it would also be clearer to follow with

"frame lists for GNTTABOP_get_status_frames are of".

> of the same type (uint64_t). Hence there's no need to translate the frame
> values. This then also renders unnecessary the limit_max parameter of
> gnttab_get_status_frames().
>
> Signed-off-by: Jan Beulich 

Reviewed-by: Andrew Cooper 

[PATCH 7/9] gnttab: no need to translate handle for gnttab_get_status_frames()

[Jan Beulich]

Unlike for GNTTABOP_setup_table native and compat frame lists are arrays
of the same type (uint64_t). Hence there's no need to translate the frame
values. This then also renders unnecessary the limit_max parameter of
gnttab_get_status_frames().

Signed-off-by: Jan Beulich 

--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -271,10 +271,7 @@ int compat_grant_table_op(unsigned int c
 }
 break;
 
-case GNTTABOP_get_status_frames: {
-unsigned int max_frame_list_size_in_pages =
-(COMPAT_ARG_XLAT_SIZE - sizeof(*nat.get_status)) /
-sizeof(*nat.get_status->frame_list.p);
+case GNTTABOP_get_status_frames:
 if ( count != 1)
 {
 rc = -EINVAL;
@@ -289,38 +286,25 @@ int compat_grant_table_op(unsigned int c
 }
 
 #define XLAT_gnttab_get_status_frames_HNDL_frame_list(_d_, _s_) \
-set_xen_guest_handle((_d_)->frame_list, (uint64_t 
*)(nat.get_status + 1))
+guest_from_compat_handle((_d_)->frame_list, (_s_)->frame_list)
 XLAT_gnttab_get_status_frames(nat.get_status, _status);
 #undef XLAT_gnttab_get_status_frames_HNDL_frame_list
 
 rc = gnttab_get_status_frames(
-guest_handle_cast(nat.uop, gnttab_get_status_frames_t),
-count, max_frame_list_size_in_pages);
+guest_handle_cast(nat.uop, gnttab_get_status_frames_t), count);
 if ( rc >= 0 )
 {
-#define XLAT_gnttab_get_status_frames_HNDL_frame_list(_d_, _s_) \
-do \
-{ \
-if ( (_s_)->status == GNTST_okay ) \
-{ \
-for ( i = 0; i < (_s_)->nr_frames; ++i ) \
-{ \
-uint64_t frame = (_s_)->frame_list.p[i]; \
-if ( __copy_to_compat_offset((_d_)->frame_list, \
- i, , 1) ) \
-(_s_)->status = GNTST_bad_virt_addr; \
-} \
-} \
-} while (0)
-XLAT_gnttab_get_status_frames(_status, nat.get_status);
-#undef XLAT_gnttab_get_status_frames_HNDL_frame_list
-if ( unlikely(__copy_to_guest(cmp_uop, _status, 1)) )
+XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_compat_t) get =
+guest_handle_cast(cmp_uop,
+  gnttab_get_status_frames_compat_t);
+
+if ( unlikely(__copy_field_to_guest(get, nat.get_status,
+status)) )
 rc = -EFAULT;
 else
 i = 1;
 }
 break;
-}
 
 default:
 domain_crash(current->domain);
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3242,7 +3242,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
 
 static long
 gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) 
uop,
- unsigned int count, unsigned int limit_max)
+ unsigned int count)
 {
 gnttab_get_status_frames_t op;
 struct domain *d;
@@ -3292,15 +3292,6 @@ gnttab_get_status_frames(XEN_GUEST_HANDL
 goto unlock;
 }
 
-if ( unlikely(limit_max < op.nr_frames) )
-{
-gdprintk(XENLOG_WARNING,
- "nr_status_frames for %pd is too large (%u,%u)\n",
- d, op.nr_frames, limit_max);
-op.status = GNTST_general_error;
-goto unlock;
-}
-
 for ( i = 0; i < op.nr_frames; i++ )
 {
 gmfn = gfn_x(gnttab_status_gfn(d, gt, i));
@@ -3664,8 +3655,7 @@ do_grant_table_op(
 
 case GNTTABOP_get_status_frames:
 rc = gnttab_get_status_frames(
-guest_handle_cast(uop, gnttab_get_status_frames_t), count,
-  UINT_MAX);
+guest_handle_cast(uop, gnttab_get_status_frames_t), count);
 break;
 
 case GNTTABOP_get_version: