Jan Beulich
2009-Jul-02 07:23 UTC
[Xen-devel] do_grant_table_op()''s input array size limitation
Am I right in understanding that the restriction on 512 entries to be passed in is solely to cap the time that''s being spent in the hypervisor without being able to service softirq requests as well as while the domain lock is being held? If that''s the case, eliminating that restriction could be fairly strait forward by putting the majority of the function body in a loop or, perhaps even better, calling hypercall_preempt_check() every so many entries and using the continuation mechanism. Thanks, Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Jul-02 07:37 UTC
Re: [Xen-devel] do_grant_table_op()''s input array size limitation
On 02/07/2009 08:23, "Jan Beulich" <JBeulich@novell.com> wrote:> Am I right in understanding that the restriction on 512 entries to be passed > in is solely to cap the time that''s being spent in the hypervisor without > being > able to service softirq requests as well as while the domain lock is being > held? > If that''s the case, eliminating that restriction could be fairly strait > forward by > putting the majority of the function body in a loop or, perhaps even better, > calling hypercall_preempt_check() every so many entries and using the > continuation mechanism.Yes, true. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Jul-02 12:59 UTC
Re: [Xen-devel] do_grant_table_op()''s input array size limitation
>>> Keir Fraser <keir.fraser@eu.citrix.com> 02.07.09 09:37 >>> >On 02/07/2009 08:23, "Jan Beulich" <JBeulich@novell.com> wrote: > >> Am I right in understanding that the restriction on 512 entries to be passed >> in is solely to cap the time that''s being spent in the hypervisor without >> being >> able to service softirq requests as well as while the domain lock is being >> held? >> If that''s the case, eliminating that restriction could be fairly strait >> forward by >> putting the majority of the function body in a loop or, perhaps even better, >> calling hypercall_preempt_check() every so many entries and using the >> continuation mechanism. > >Yes, true.Would you have time to look over this (so far only compile tested) patch, to see if there''s anything obviously wrong (I admit I feel uncertain whenever it comes to touching the grant table code)? Thanks, Jan --- 2009-06-10.orig/xen/common/compat/grant_table.c 2008-08-01 08:48:42.000000000 +0200 +++ 2009-06-10/xen/common/compat/grant_table.c 2009-07-02 14:42:05.000000000 +0200 @@ -35,7 +35,9 @@ int compat_grant_table_op(unsigned int c { int rc = 0; unsigned int i; + XEN_GUEST_HANDLE(void) cnt_uop; + set_xen_guest_handle(cnt_uop, NULL); switch ( cmd ) { #define CASE(name) \ @@ -79,7 +81,7 @@ int compat_grant_table_op(unsigned int c return do_grant_table_op(cmd, cmp_uop, count); } - if ( count > 512 ) + if ( (int)count < 0 ) rc = -EINVAL; for ( i = 0; i < count && rc == 0; ) @@ -128,6 +130,7 @@ int compat_grant_table_op(unsigned int c rc = gnttab_setup_table(guest_handle_cast(nat.uop, gnttab_setup_table_t), 1); } } + ASSERT(rc <= 0); if ( rc == 0 ) { #define XLAT_gnttab_setup_table_HNDL_frame_list(_d_, _s_) \ @@ -163,12 +166,19 @@ int compat_grant_table_op(unsigned int c } if ( rc == 0 ) rc = gnttab_transfer(guest_handle_cast(nat.uop, gnttab_transfer_t), n); - if ( rc == 0 ) + if ( rc > 0 ) + { + ASSERT(rc < n); + i -= n - rc; + n = rc; + } + if ( rc >= 0 ) { XEN_GUEST_HANDLE(gnttab_transfer_compat_t) xfer; xfer = guest_handle_cast(cmp_uop, gnttab_transfer_compat_t); guest_handle_add_offset(xfer, i); + cnt_uop = guest_handle_cast(xfer, void); while ( n-- ) { guest_handle_add_offset(xfer, -1); @@ -201,12 +211,19 @@ int compat_grant_table_op(unsigned int c } if ( rc == 0 ) rc = gnttab_copy(guest_handle_cast(nat.uop, gnttab_copy_t), n); - if ( rc == 0 ) + if ( rc > 0 ) + { + ASSERT(rc < n); + i -= n - rc; + n = rc; + } + if ( rc >= 0 ) { XEN_GUEST_HANDLE(gnttab_copy_compat_t) copy; copy = guest_handle_cast(cmp_uop, gnttab_copy_compat_t); guest_handle_add_offset(copy, i); + cnt_uop = guest_handle_cast(copy, void); while ( n-- ) { guest_handle_add_offset(copy, -1); @@ -222,6 +239,14 @@ int compat_grant_table_op(unsigned int c } } + if ( rc > 0 ) + { + ASSERT(i < count); + ASSERT(!guest_handle_is_null(cnt_uop)); + rc = hypercall_create_continuation(__HYPERVISOR_grant_table_op, + "ihi", cmd, cnt_uop, count - i); + } + return rc; } --- 2009-06-10.orig/xen/common/grant_table.c 2009-06-03 12:22:45.000000000 +0200 +++ 2009-06-10/xen/common/grant_table.c 2009-07-02 14:42:33.000000000 +0200 @@ -29,6 +29,7 @@ #include <xen/lib.h> #include <xen/sched.h> #include <xen/mm.h> +#include <xen/event.h> #include <xen/trace.h> #include <xen/guest_access.h> #include <xen/domain_page.h> @@ -465,6 +466,8 @@ gnttab_map_grant_ref( for ( i = 0; i < count; i++ ) { + if (i && hypercall_preempt_check()) + return i; if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) return -EFAULT; __gnttab_map_grant_ref(&op); @@ -722,6 +725,9 @@ gnttab_unmap_grant_ref( count -= c; done += c; + + if (count && hypercall_preempt_check()) + return done; } return 0; @@ -781,6 +787,9 @@ gnttab_unmap_and_replace( count -= c; done += c; + + if (count && hypercall_preempt_check()) + return done; } return 0; @@ -1086,6 +1095,9 @@ gnttab_transfer( for ( i = 0; i < count; i++ ) { + if (i && hypercall_preempt_check()) + return i; + /* Read from caller address space. */ if ( unlikely(__copy_from_guest_offset(&gop, uop, i, 1)) ) { @@ -1478,6 +1490,8 @@ gnttab_copy( for ( i = 0; i < count; i++ ) { + if (i && hypercall_preempt_check()) + return i; if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) return -EFAULT; __gnttab_copy(&op); @@ -1494,7 +1508,7 @@ do_grant_table_op( long rc; struct domain *d = current->domain; - if ( count > 512 ) + if ( (int)count < 0 ) return -EINVAL; domain_lock(d); @@ -1509,6 +1523,11 @@ do_grant_table_op( if ( unlikely(!guest_handle_okay(map, count)) ) goto out; rc = gnttab_map_grant_ref(map, count); + if ( rc > 0 ) + { + guest_handle_add_offset(map, rc); + uop = guest_handle_cast(map, void); + } break; } case GNTTABOP_unmap_grant_ref: @@ -1518,6 +1537,11 @@ do_grant_table_op( if ( unlikely(!guest_handle_okay(unmap, count)) ) goto out; rc = gnttab_unmap_grant_ref(unmap, count); + if ( rc > 0 ) + { + guest_handle_add_offset(unmap, rc); + uop = guest_handle_cast(unmap, void); + } break; } case GNTTABOP_unmap_and_replace: @@ -1530,12 +1554,18 @@ do_grant_table_op( if ( unlikely(!replace_grant_supported()) ) goto out; rc = gnttab_unmap_and_replace(unmap, count); + if ( rc > 0 ) + { + guest_handle_add_offset(unmap, rc); + uop = guest_handle_cast(unmap, void); + } break; } case GNTTABOP_setup_table: { rc = gnttab_setup_table( guest_handle_cast(uop, gnttab_setup_table_t), count); + ASSERT(rc <= 0); break; } case GNTTABOP_transfer: @@ -1545,6 +1575,11 @@ do_grant_table_op( if ( unlikely(!guest_handle_okay(transfer, count)) ) goto out; rc = gnttab_transfer(transfer, count); + if ( rc > 0 ) + { + guest_handle_add_offset(transfer, rc); + uop = guest_handle_cast(transfer, void); + } break; } case GNTTABOP_copy: @@ -1554,12 +1589,18 @@ do_grant_table_op( if ( unlikely(!guest_handle_okay(copy, count)) ) goto out; rc = gnttab_copy(copy, count); + if ( rc > 0 ) + { + guest_handle_add_offset(copy, rc); + uop = guest_handle_cast(copy, void); + } break; } case GNTTABOP_query_size: { rc = gnttab_query_size( guest_handle_cast(uop, gnttab_query_size_t), count); + ASSERT(rc <= 0); break; } default: @@ -1569,6 +1610,13 @@ do_grant_table_op( out: domain_unlock(d); + + if ( rc > 0 ) + { + ASSERT(rc < count); + rc = hypercall_create_continuation(__HYPERVISOR_grant_table_op, + "ihi", cmd, uop, count - rc); + } return rc; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Jul-02 14:24 UTC
Re: [Xen-devel] do_grant_table_op()''s input array size limitation
On 02/07/2009 13:59, "Jan Beulich" <JBeulich@novell.com> wrote:>> Yes, true. > > Would you have time to look over this (so far only compile tested) patch, > to see if there''s anything obviously wrong (I admit I feel uncertain > whenever it comes to touching the grant table code)?I feel the same looking at the compat code shims. ;-) The grant_table.c changes look about right to me. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel