* [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range}
@ 2013-12-18 14:29 Jan Beulich
2013-12-18 14:34 ` [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich
` (6 more replies)
0 siblings, 7 replies; 47+ messages in thread
From: Jan Beulich @ 2013-12-18 14:29 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini
The hypervisor isn't supposed to use the input structures for
storing continuation information - only fields explicitly used as
hypercall outputs should ever be updated.
Obviously this implies an ABI change, but since the previous
behavior was not intended to be that way I don't think we should
stick to the old behavior.
There's one caveat though - with the previous model, the caller
could - upon failure - use the updated structure to find out how
by progress was made. However, that wasn't intended afaict,
largely supported by this information depending on hypervisor
internals (i.e. the caller would have to know the order of request
processing and the meaning of the respective size fields, which
aren't simply saying "this much was processed").
Consequently, as a follow-up we may want to consider making
explicit (and straight forward) this progress indication on error.
1: move XENMEM_add_to_physmap handling framework to common code
2: fix XENMEM_add_to_physmap preemption handling
3: move XENMEM_add_to_physmap_range handling framework to common code
4: fix XENMEM_add_to_physmap_range preemption handling
Signed-off-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code 2013-12-18 14:29 [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich @ 2013-12-18 14:34 ` Jan Beulich 2013-12-18 17:52 ` Ian Campbell 2013-12-18 14:35 ` [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling Jan Beulich ` (5 subsequent siblings) 6 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-18 14:34 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 14198 bytes --] There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being supported through XENMEM_add_to_physmap_range is being dropped as being pointless. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues share_xen_page_with_guest(page, dom_xen, readonly); } -static int xenmem_add_to_physmap_one( +int xenmem_add_to_physmap_one( struct domain *d, uint16_t space, domid_t foreign_domid, @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - /* Foreign mapping is only supported by add_to_physmap_range */ - if ( xatp.space == XENMAPSPACE_gmfn_foreign ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID, - xatp.idx, xatp.gpfn); - - rcu_unlock_domain(d); - - return rc; - } - case XENMEM_add_to_physmap_range: { struct xen_add_to_physmap_range xatpr; --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4520,20 +4520,23 @@ static int handle_iomem_range(unsigned l return 0; } -static int xenmem_add_to_physmap_once( +int xenmem_add_to_physmap_one( struct domain *d, - const struct xen_add_to_physmap *xatp) + uint16_t space, + domid_t foreign_domid, + unsigned long idx, + xen_pfn_t gpfn) { struct page_info *page = NULL; unsigned long gfn = 0; /* gcc ... */ - unsigned long prev_mfn, mfn = 0, gpfn, idx; + unsigned long prev_mfn, mfn = 0, old_gpfn; int rc; p2m_type_t p2mt; - switch ( xatp->space ) + switch ( space ) { case XENMAPSPACE_shared_info: - if ( xatp->idx == 0 ) + if ( idx == 0 ) mfn = virt_to_mfn(d->shared_info); break; case XENMAPSPACE_grant_table: @@ -4542,9 +4545,8 @@ static int xenmem_add_to_physmap_once( if ( d->grant_table->gt_version == 0 ) d->grant_table->gt_version = 1; - idx = xatp->idx; if ( d->grant_table->gt_version == 2 && - (xatp->idx & XENMAPIDX_grant_table_status) ) + (idx & XENMAPIDX_grant_table_status) ) { idx &= ~XENMAPIDX_grant_table_status; if ( idx < nr_status_frames(d->grant_table) ) @@ -4566,9 +4568,9 @@ static int xenmem_add_to_physmap_once( case XENMAPSPACE_gmfn: { p2m_type_t p2mt; - gfn = xatp->idx; - idx = mfn_x(get_gfn_unshare(d, xatp->idx, &p2mt)); + gfn = idx; + idx = mfn_x(get_gfn_unshare(d, idx, &p2mt)); /* If the page is still shared, exit early */ if ( p2m_is_shared(p2mt) ) { @@ -4589,41 +4591,38 @@ static int xenmem_add_to_physmap_once( { if ( page ) put_page(page); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); return -EINVAL; } /* Remove previously mapped page if it was present. */ - prev_mfn = mfn_x(get_gfn(d, xatp->gpfn, &p2mt)); + prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt)); if ( mfn_valid(prev_mfn) ) { if ( is_xen_heap_mfn(prev_mfn) ) /* Xen heap frames are simply unhooked from this phys slot. */ - guest_physmap_remove_page(d, xatp->gpfn, prev_mfn, PAGE_ORDER_4K); + guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); else /* Normal domain memory is freed, to avoid leaking memory. */ - guest_remove_page(d, xatp->gpfn); + guest_remove_page(d, gpfn); } /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ - put_gfn(d, xatp->gpfn); + put_gfn(d, gpfn); /* Unmap from old location, if any. */ - gpfn = get_gpfn_from_mfn(mfn); - ASSERT( gpfn != SHARED_M2P_ENTRY ); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) - ASSERT( gpfn == gfn ); - if ( gpfn != INVALID_M2P_ENTRY ) - guest_physmap_remove_page(d, gpfn, mfn, PAGE_ORDER_4K); + old_gpfn = get_gpfn_from_mfn(mfn); + ASSERT( old_gpfn != SHARED_M2P_ENTRY ); + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) + ASSERT( old_gpfn == gfn ); + if ( old_gpfn != INVALID_M2P_ENTRY ) + guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K); /* Map at new location. */ - rc = guest_physmap_add_page(d, xatp->gpfn, mfn, PAGE_ORDER_4K); + rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */ - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); if ( page ) @@ -4632,90 +4631,12 @@ static int xenmem_add_to_physmap_once( return rc; } -static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) -{ - struct xen_add_to_physmap start_xatp; - int rc = 0; - - if ( xatp->space == XENMAPSPACE_gmfn_range ) - { - if ( need_iommu(d) ) - this_cpu(iommu_dont_flush_iotlb) = 1; - - start_xatp = *xatp; - while ( xatp->size > 0 ) - { - rc = xenmem_add_to_physmap_once(d, xatp); - if ( rc < 0 ) - break; - - xatp->idx++; - xatp->gpfn++; - xatp->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatp->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - break; - } - } - - if ( need_iommu(d) ) - { - this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); - } - - return rc; - } - - return xenmem_add_to_physmap_once(d, xatp); -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { int rc; switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) ) - { - rcu_unlock_domain(d); - return -EPERM; - } - - rc = xenmem_add_to_physmap(d, &xatp); - - rcu_unlock_domain(d); - - if ( xatp.space == XENMAPSPACE_gmfn_range ) - { - if ( rc && __copy_to_guest(arg, &xatp, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - } - - return rc; - } - case XENMEM_set_memory_map: { struct xen_foreign_memory_map fmap; --- a/xen/arch/x86/x86_64/compat/mm.c +++ b/xen/arch/x86/x86_64/compat/mm.c @@ -55,34 +55,6 @@ int compat_arch_memory_op(int op, XEN_GU switch ( op ) { - case XENMEM_add_to_physmap: - { - struct compat_add_to_physmap cmp; - struct xen_add_to_physmap *nat = COMPAT_ARG_XLAT_VIRT_BASE; - - if ( copy_from_guest(&cmp, arg, 1) ) - return -EFAULT; - - XLAT_add_to_physmap(nat, &cmp); - rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); - - if ( !rc || cmp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp, nat); - if ( __copy_to_guest(arg, &cmp, 1) ) - { - if ( rc == __HYPERVISOR_memory_op ) - hypercall_cancel_continuation(); - return -EFAULT; - } - - if ( rc == __HYPERVISOR_memory_op ) - hypercall_xlat_continuation(NULL, 0x2, nat, arg); - - break; - } - case XENMEM_set_memory_map: { struct compat_foreign_memory_map cmp; --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -26,11 +26,13 @@ int compat_memory_op(unsigned int cmd, X XEN_GUEST_HANDLE_PARAM(void) hnd; struct xen_memory_reservation *rsrv; struct xen_memory_exchange *xchg; + struct xen_add_to_physmap *atp; struct xen_remove_from_physmap *xrfp; } nat; union { struct compat_memory_reservation rsrv; struct compat_memory_exchange xchg; + struct compat_add_to_physmap atp; } cmp; set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); @@ -187,6 +189,14 @@ int compat_memory_op(unsigned int cmd, X nat.hnd = compat; break; + case XENMEM_add_to_physmap: + if ( copy_from_guest(&cmp.atp, compat, 1) ) + return -EFAULT; + + XLAT_add_to_physmap(nat.atp, &cmp.atp); + + break; + case XENMEM_remove_from_physmap: { struct compat_remove_from_physmap cmp; @@ -315,6 +325,16 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_remove_from_physmap: break; + case XENMEM_add_to_physmap: + if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) + break; + + XLAT_add_to_physmap(&cmp.atp, nat.atp); + if ( __copy_to_guest(compat, &cmp.atp, 1) ) + rc = -EFAULT; + + break; + default: domain_crash(current->domain); split = 0; --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA return rc; } +static int xenmem_add_to_physmap(struct domain *d, + struct xen_add_to_physmap *xatp) +{ + struct xen_add_to_physmap start_xatp; + int rc = 0; + + if ( xatp->space != XENMAPSPACE_gmfn_range ) + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + this_cpu(iommu_dont_flush_iotlb) = 1; +#endif + + start_xatp = *xatp; + while ( xatp->size > 0 ) + { + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + if ( rc < 0 ) + break; + + xatp->idx++; + xatp->gpfn++; + xatp->size--; + + /* Check for continuation if it's not the last interation. */ + if ( xatp->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + break; + } + } + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + { + this_cpu(iommu_dont_flush_iotlb) = 0; + iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + } +#endif + + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -673,6 +720,40 @@ long do_memory_op(unsigned long cmd, XEN break; + case XENMEM_add_to_physmap: + { + struct xen_add_to_physmap xatp; + + if ( copy_from_guest(&xatp, arg, 1) ) + return -EFAULT; + + d = rcu_lock_domain_by_any_id(xatp.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap(d, &xatp); + + rcu_unlock_domain(d); + + if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatp, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -356,6 +356,10 @@ static inline unsigned int get_order_fro void scrub_one_page(struct page_info *); +int xenmem_add_to_physmap_one(struct domain *d, uint16_t space, + domid_t foreign_domid, + unsigned long idx, xen_pfn_t gpfn); + /* Returns 1 on success, 0 on error, negative if the ring * for event propagation is full in the presence of paging */ int guest_remove_page(struct domain *d, unsigned long gmfn); [-- Attachment #2: xatp-common.patch --] [-- Type: text/plain, Size: 14258 bytes --] move XENMEM_add_to_physmap handling framework to common code There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being supported through XENMEM_add_to_physmap_range is being dropped as being pointless. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues share_xen_page_with_guest(page, dom_xen, readonly); } -static int xenmem_add_to_physmap_one( +int xenmem_add_to_physmap_one( struct domain *d, uint16_t space, domid_t foreign_domid, @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - /* Foreign mapping is only supported by add_to_physmap_range */ - if ( xatp.space == XENMAPSPACE_gmfn_foreign ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID, - xatp.idx, xatp.gpfn); - - rcu_unlock_domain(d); - - return rc; - } - case XENMEM_add_to_physmap_range: { struct xen_add_to_physmap_range xatpr; --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4520,20 +4520,23 @@ static int handle_iomem_range(unsigned l return 0; } -static int xenmem_add_to_physmap_once( +int xenmem_add_to_physmap_one( struct domain *d, - const struct xen_add_to_physmap *xatp) + uint16_t space, + domid_t foreign_domid, + unsigned long idx, + xen_pfn_t gpfn) { struct page_info *page = NULL; unsigned long gfn = 0; /* gcc ... */ - unsigned long prev_mfn, mfn = 0, gpfn, idx; + unsigned long prev_mfn, mfn = 0, old_gpfn; int rc; p2m_type_t p2mt; - switch ( xatp->space ) + switch ( space ) { case XENMAPSPACE_shared_info: - if ( xatp->idx == 0 ) + if ( idx == 0 ) mfn = virt_to_mfn(d->shared_info); break; case XENMAPSPACE_grant_table: @@ -4542,9 +4545,8 @@ static int xenmem_add_to_physmap_once( if ( d->grant_table->gt_version == 0 ) d->grant_table->gt_version = 1; - idx = xatp->idx; if ( d->grant_table->gt_version == 2 && - (xatp->idx & XENMAPIDX_grant_table_status) ) + (idx & XENMAPIDX_grant_table_status) ) { idx &= ~XENMAPIDX_grant_table_status; if ( idx < nr_status_frames(d->grant_table) ) @@ -4566,9 +4568,9 @@ static int xenmem_add_to_physmap_once( case XENMAPSPACE_gmfn: { p2m_type_t p2mt; - gfn = xatp->idx; - idx = mfn_x(get_gfn_unshare(d, xatp->idx, &p2mt)); + gfn = idx; + idx = mfn_x(get_gfn_unshare(d, idx, &p2mt)); /* If the page is still shared, exit early */ if ( p2m_is_shared(p2mt) ) { @@ -4589,41 +4591,38 @@ static int xenmem_add_to_physmap_once( { if ( page ) put_page(page); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); return -EINVAL; } /* Remove previously mapped page if it was present. */ - prev_mfn = mfn_x(get_gfn(d, xatp->gpfn, &p2mt)); + prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt)); if ( mfn_valid(prev_mfn) ) { if ( is_xen_heap_mfn(prev_mfn) ) /* Xen heap frames are simply unhooked from this phys slot. */ - guest_physmap_remove_page(d, xatp->gpfn, prev_mfn, PAGE_ORDER_4K); + guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); else /* Normal domain memory is freed, to avoid leaking memory. */ - guest_remove_page(d, xatp->gpfn); + guest_remove_page(d, gpfn); } /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ - put_gfn(d, xatp->gpfn); + put_gfn(d, gpfn); /* Unmap from old location, if any. */ - gpfn = get_gpfn_from_mfn(mfn); - ASSERT( gpfn != SHARED_M2P_ENTRY ); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) - ASSERT( gpfn == gfn ); - if ( gpfn != INVALID_M2P_ENTRY ) - guest_physmap_remove_page(d, gpfn, mfn, PAGE_ORDER_4K); + old_gpfn = get_gpfn_from_mfn(mfn); + ASSERT( old_gpfn != SHARED_M2P_ENTRY ); + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) + ASSERT( old_gpfn == gfn ); + if ( old_gpfn != INVALID_M2P_ENTRY ) + guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K); /* Map at new location. */ - rc = guest_physmap_add_page(d, xatp->gpfn, mfn, PAGE_ORDER_4K); + rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */ - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); if ( page ) @@ -4632,90 +4631,12 @@ static int xenmem_add_to_physmap_once( return rc; } -static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) -{ - struct xen_add_to_physmap start_xatp; - int rc = 0; - - if ( xatp->space == XENMAPSPACE_gmfn_range ) - { - if ( need_iommu(d) ) - this_cpu(iommu_dont_flush_iotlb) = 1; - - start_xatp = *xatp; - while ( xatp->size > 0 ) - { - rc = xenmem_add_to_physmap_once(d, xatp); - if ( rc < 0 ) - break; - - xatp->idx++; - xatp->gpfn++; - xatp->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatp->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - break; - } - } - - if ( need_iommu(d) ) - { - this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); - } - - return rc; - } - - return xenmem_add_to_physmap_once(d, xatp); -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { int rc; switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) ) - { - rcu_unlock_domain(d); - return -EPERM; - } - - rc = xenmem_add_to_physmap(d, &xatp); - - rcu_unlock_domain(d); - - if ( xatp.space == XENMAPSPACE_gmfn_range ) - { - if ( rc && __copy_to_guest(arg, &xatp, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - } - - return rc; - } - case XENMEM_set_memory_map: { struct xen_foreign_memory_map fmap; --- a/xen/arch/x86/x86_64/compat/mm.c +++ b/xen/arch/x86/x86_64/compat/mm.c @@ -55,34 +55,6 @@ int compat_arch_memory_op(int op, XEN_GU switch ( op ) { - case XENMEM_add_to_physmap: - { - struct compat_add_to_physmap cmp; - struct xen_add_to_physmap *nat = COMPAT_ARG_XLAT_VIRT_BASE; - - if ( copy_from_guest(&cmp, arg, 1) ) - return -EFAULT; - - XLAT_add_to_physmap(nat, &cmp); - rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); - - if ( !rc || cmp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp, nat); - if ( __copy_to_guest(arg, &cmp, 1) ) - { - if ( rc == __HYPERVISOR_memory_op ) - hypercall_cancel_continuation(); - return -EFAULT; - } - - if ( rc == __HYPERVISOR_memory_op ) - hypercall_xlat_continuation(NULL, 0x2, nat, arg); - - break; - } - case XENMEM_set_memory_map: { struct compat_foreign_memory_map cmp; --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -26,11 +26,13 @@ int compat_memory_op(unsigned int cmd, X XEN_GUEST_HANDLE_PARAM(void) hnd; struct xen_memory_reservation *rsrv; struct xen_memory_exchange *xchg; + struct xen_add_to_physmap *atp; struct xen_remove_from_physmap *xrfp; } nat; union { struct compat_memory_reservation rsrv; struct compat_memory_exchange xchg; + struct compat_add_to_physmap atp; } cmp; set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); @@ -187,6 +189,14 @@ int compat_memory_op(unsigned int cmd, X nat.hnd = compat; break; + case XENMEM_add_to_physmap: + if ( copy_from_guest(&cmp.atp, compat, 1) ) + return -EFAULT; + + XLAT_add_to_physmap(nat.atp, &cmp.atp); + + break; + case XENMEM_remove_from_physmap: { struct compat_remove_from_physmap cmp; @@ -315,6 +325,16 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_remove_from_physmap: break; + case XENMEM_add_to_physmap: + if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) + break; + + XLAT_add_to_physmap(&cmp.atp, nat.atp); + if ( __copy_to_guest(compat, &cmp.atp, 1) ) + rc = -EFAULT; + + break; + default: domain_crash(current->domain); split = 0; --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA return rc; } +static int xenmem_add_to_physmap(struct domain *d, + struct xen_add_to_physmap *xatp) +{ + struct xen_add_to_physmap start_xatp; + int rc = 0; + + if ( xatp->space != XENMAPSPACE_gmfn_range ) + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + this_cpu(iommu_dont_flush_iotlb) = 1; +#endif + + start_xatp = *xatp; + while ( xatp->size > 0 ) + { + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + if ( rc < 0 ) + break; + + xatp->idx++; + xatp->gpfn++; + xatp->size--; + + /* Check for continuation if it's not the last interation. */ + if ( xatp->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + break; + } + } + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + { + this_cpu(iommu_dont_flush_iotlb) = 0; + iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + } +#endif + + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -673,6 +720,40 @@ long do_memory_op(unsigned long cmd, XEN break; + case XENMEM_add_to_physmap: + { + struct xen_add_to_physmap xatp; + + if ( copy_from_guest(&xatp, arg, 1) ) + return -EFAULT; + + d = rcu_lock_domain_by_any_id(xatp.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap(d, &xatp); + + rcu_unlock_domain(d); + + if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatp, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -356,6 +356,10 @@ static inline unsigned int get_order_fro void scrub_one_page(struct page_info *); +int xenmem_add_to_physmap_one(struct domain *d, uint16_t space, + domid_t foreign_domid, + unsigned long idx, xen_pfn_t gpfn); + /* Returns 1 on success, 0 on error, negative if the ring * for event propagation is full in the presence of paging */ int guest_remove_page(struct domain *d, unsigned long gmfn); [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code 2013-12-18 14:34 ` [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich @ 2013-12-18 17:52 ` Ian Campbell 2013-12-20 7:48 ` Jan Beulich 0 siblings, 1 reply; 47+ messages in thread From: Ian Campbell @ 2013-12-18 17:52 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Wed, 2013-12-18 at 14:34 +0000, Jan Beulich wrote: > There's really nothing really architecture specific here; the > architecture specific handling is limited to > xenmem_add_to_physmap_one(). > > Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being > supported through XENMEM_add_to_physmap_range is being dropped as being > pointless. It's not pointless, dropping this restriction turns a comprehensible EINVAL result into an incorrect ESRCH result. Which is incorrect because no domain was specified so how can it not exist. I'd also rather avoid introducing unnecessary/legacy hypercalls to ARM if possible. > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues > share_xen_page_with_guest(page, dom_xen, readonly); > } > > -static int xenmem_add_to_physmap_one( > +int xenmem_add_to_physmap_one( > struct domain *d, > uint16_t space, > domid_t foreign_domid, > @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA > > switch ( op ) > { > - case XENMEM_add_to_physmap: > - { > - struct xen_add_to_physmap xatp; > - struct domain *d; > - > - if ( copy_from_guest(&xatp, arg, 1) ) > - return -EFAULT; > - > - /* Foreign mapping is only supported by add_to_physmap_range */ > - if ( xatp.space == XENMAPSPACE_gmfn_foreign ) > - return -EINVAL; > - > - d = rcu_lock_domain_by_any_id(xatp.domid); > - if ( d == NULL ) > - return -ESRCH; > - > - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); > - if ( rc ) > - { > - rcu_unlock_domain(d); > - return rc; > - } > - > - rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID, > - xatp.idx, xatp.gpfn); > - > - rcu_unlock_domain(d); > - > - return rc; > - } > - > case XENMEM_add_to_physmap_range: > { > struct xen_add_to_physmap_range xatpr; > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA > return rc; > } > > +static int xenmem_add_to_physmap(struct domain *d, > + struct xen_add_to_physmap *xatp) > +{ > + struct xen_add_to_physmap start_xatp; > + int rc = 0; > + > + if ( xatp->space != XENMAPSPACE_gmfn_range ) > + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, > + xatp->idx, xatp->gpfn); > + > +#ifdef HAS_PASSTHROUGH > + if ( need_iommu(d) ) > + this_cpu(iommu_dont_flush_iotlb) = 1; > +#endif > + > + start_xatp = *xatp; > + while ( xatp->size > 0 ) > + { > + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, > + xatp->idx, xatp->gpfn); ARM's xenmem_add_to_physmap_one does not support XENMAPSPACE_gmfn_range, so it will return ENOSYS and fail out. I suppose that is ok. But it makes the bulk of this function pointless. How about pulling the != XENMAPSPACE_gmfn_range check into do_memory_op either calling xenmem_add_to_phymap_one or a per arch gmfn_range function? > + if ( rc < 0 ) > + break; > + > + xatp->idx++; > + xatp->gpfn++; > + xatp->size--; > + > + /* Check for continuation if it's not the last interation. */ That's the second or third time this "interation" typo has got copied ;-) > + if ( xatp->size > 0 && hypercall_preempt_check() ) > + { > + rc = -EAGAIN; > + break; > + } > + } > + > +#ifdef HAS_PASSTHROUGH > + if ( need_iommu(d) ) > + { > + this_cpu(iommu_dont_flush_iotlb) = 0; > + iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); > + iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); > + } > +#endif > + > + return rc; > +} > + > long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > { > struct domain *d; > @@ -673,6 +720,40 @@ long do_memory_op(unsigned long cmd, XEN > > break; > > + case XENMEM_add_to_physmap: > + { > + struct xen_add_to_physmap xatp; > + > + if ( copy_from_guest(&xatp, arg, 1) ) > + return -EFAULT; > + > + d = rcu_lock_domain_by_any_id(xatp.domid); > + if ( d == NULL ) > + return -ESRCH; > + > + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); > + if ( rc ) > + { > + rcu_unlock_domain(d); > + return rc; > + } > + > + rc = xenmem_add_to_physmap(d, &xatp); > + > + rcu_unlock_domain(d); > + > + if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) > + { > + if ( !__copy_to_guest(arg, &xatp, 1) ) > + rc = hypercall_create_continuation( > + __HYPERVISOR_memory_op, "ih", op, arg); > + else > + rc = -EFAULT; > + } > + > + return rc; > + } > + > case XENMEM_remove_from_physmap: > { > struct xen_remove_from_physmap xrfp; ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code 2013-12-18 17:52 ` Ian Campbell @ 2013-12-20 7:48 ` Jan Beulich 2013-12-20 8:58 ` Ian Campbell 0 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 7:48 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini >>> On 18.12.13 at 18:52, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Wed, 2013-12-18 at 14:34 +0000, Jan Beulich wrote: >> There's really nothing really architecture specific here; the >> architecture specific handling is limited to >> xenmem_add_to_physmap_one(). >> >> Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being >> supported through XENMEM_add_to_physmap_range is being dropped as being >> pointless. > > It's not pointless, dropping this restriction turns a comprehensible > EINVAL result into an incorrect ESRCH result. Which is incorrect because > no domain was specified so how can it not exist. How can there have been no domain specified for a XENMAPSPACE_gmfn_foreign? If the field was left uninitialized, -ESRCH would be as valid as -EINVAL, and I don't think we specify for any hypercall that between multiple applicable error code a specific one would be preferred over others. > I'd also rather avoid introducing unnecessary/legacy hypercalls to ARM > if possible. I can understand that, but please also take the perspective that moving the architecture independent bits out should have been done much earlier, and (see below) adding a conditional here doesn't seem too attractive to me. >> +static int xenmem_add_to_physmap(struct domain *d, >> + struct xen_add_to_physmap *xatp) >> +{ >> + struct xen_add_to_physmap start_xatp; >> + int rc = 0; >> + >> + if ( xatp->space != XENMAPSPACE_gmfn_range ) >> + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, >> + xatp->idx, xatp->gpfn); >> + >> +#ifdef HAS_PASSTHROUGH >> + if ( need_iommu(d) ) >> + this_cpu(iommu_dont_flush_iotlb) = 1; >> +#endif >> + >> + start_xatp = *xatp; >> + while ( xatp->size > 0 ) >> + { >> + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, >> + xatp->idx, xatp->gpfn); > > ARM's xenmem_add_to_physmap_one does not support XENMAPSPACE_gmfn_range, > so it will return ENOSYS and fail out. I suppose that is ok. But it > makes the bulk of this function pointless. > > How about pulling the != XENMAPSPACE_gmfn_range check into do_memory_op > either calling xenmem_add_to_phymap_one or a per arch gmfn_range > function? No, I think having just one arch specific function dealing with everything, and having the common code really take care of as much as is common is the right thing (and considering the review by Tim and the ack from Keir, I think others agree). Which would leave the option of adding a #ifdef CONFIG_ARM (or #ifndef CONFIG_X86) in the common code - not really nice. >> + if ( rc < 0 ) >> + break; >> + >> + xatp->idx++; >> + xatp->gpfn++; >> + xatp->size--; >> + >> + /* Check for continuation if it's not the last interation. */ > > That's the second or third time this "interation" typo has got > copied ;-) Fixed - I didn't look that closely at the comments I was moving around. Albeit I had noticed and fixed the missing stops... Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code 2013-12-20 7:48 ` Jan Beulich @ 2013-12-20 8:58 ` Ian Campbell 2013-12-20 9:12 ` Jan Beulich 0 siblings, 1 reply; 47+ messages in thread From: Ian Campbell @ 2013-12-20 8:58 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 07:48 +0000, Jan Beulich wrote: > >>> On 18.12.13 at 18:52, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Wed, 2013-12-18 at 14:34 +0000, Jan Beulich wrote: > >> There's really nothing really architecture specific here; the > >> architecture specific handling is limited to > >> xenmem_add_to_physmap_one(). > >> > >> Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being > >> supported through XENMEM_add_to_physmap_range is being dropped as being > >> pointless. > > > > It's not pointless, dropping this restriction turns a comprehensible > > EINVAL result into an incorrect ESRCH result. Which is incorrect because > > no domain was specified so how can it not exist. > > How can there have been no domain specified for a > XENMAPSPACE_gmfn_foreign? The arguments to XENMEM_add_to_physmap have no field which specifies a foreign domain. (this is one of the reasons we added the _range version with the field in the first place). The domid parameter to add_to_physmap one is why hardcoded to DOMID_INVALID in all the add_to_physmap (!range) cases. > If the field was left uninitialized, > -ESRCH would be as valid as -EINVAL, and I don't think we > specify for any hypercall that between multiple applicable > error code a specific one would be preferred over others. I think given the above -ESRCH is invalid, since there is no field to have been left uninitialised. > > I'd also rather avoid introducing unnecessary/legacy hypercalls to ARM > > if possible. > > I can understand that, but please also take the perspective that > moving the architecture independent bits out should have been > done much earlier, and (see below) adding a conditional here > doesn't seem too attractive to me. I don't think this should be a per arch conditional, if that's what you mean, just a blanket if ( ) return ENOSYS. > >> +static int xenmem_add_to_physmap(struct domain *d, > >> + struct xen_add_to_physmap *xatp) > >> +{ > >> + struct xen_add_to_physmap start_xatp; > >> + int rc = 0; > >> + > >> + if ( xatp->space != XENMAPSPACE_gmfn_range ) > >> + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, > >> + xatp->idx, xatp->gpfn); > >> + > >> +#ifdef HAS_PASSTHROUGH > >> + if ( need_iommu(d) ) > >> + this_cpu(iommu_dont_flush_iotlb) = 1; > >> +#endif > >> + > >> + start_xatp = *xatp; > >> + while ( xatp->size > 0 ) > >> + { > >> + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, > >> + xatp->idx, xatp->gpfn); > > > > ARM's xenmem_add_to_physmap_one does not support XENMAPSPACE_gmfn_range, > > so it will return ENOSYS and fail out. I suppose that is ok. But it > > makes the bulk of this function pointless. > > > > How about pulling the != XENMAPSPACE_gmfn_range check into do_memory_op > > either calling xenmem_add_to_phymap_one or a per arch gmfn_range > > function? > > No, I think having just one arch specific function dealing with > everything, and having the common code really take care of > as much as is common is the right thing (and considering the > review by Tim and the ack from Keir, I think others agree). > > Which would leave the option of adding a #ifdef CONFIG_ARM > (or #ifndef CONFIG_X86) in the common code - not really nice. (note that this is a different case to the above gmfn_range here vs gmfn_foreign there) The #ifndef would be the better of those two, since I'd expect no other port to be added with this legacy interface. Another option would be HAVE_FOO. But in any case leaving this is not too harmful since the end result is correct -ENOSYS anyway. Ian. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code 2013-12-20 8:58 ` Ian Campbell @ 2013-12-20 9:12 ` Jan Beulich 2013-12-20 9:55 ` Ian Campbell 0 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 9:12 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini >>> On 20.12.13 at 09:58, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Fri, 2013-12-20 at 07:48 +0000, Jan Beulich wrote: >> >>> On 18.12.13 at 18:52, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > On Wed, 2013-12-18 at 14:34 +0000, Jan Beulich wrote: >> >> There's really nothing really architecture specific here; the >> >> architecture specific handling is limited to >> >> xenmem_add_to_physmap_one(). >> >> >> >> Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being >> >> supported through XENMEM_add_to_physmap_range is being dropped as being >> >> pointless. >> > >> > It's not pointless, dropping this restriction turns a comprehensible >> > EINVAL result into an incorrect ESRCH result. Which is incorrect because >> > no domain was specified so how can it not exist. >> >> How can there have been no domain specified for a >> XENMAPSPACE_gmfn_foreign? > > The arguments to XENMEM_add_to_physmap have no field which specifies a > foreign domain. (this is one of the reasons we added the _range version > with the field in the first place). > > The domid parameter to add_to_physmap one is why hardcoded to > DOMID_INVALID in all the add_to_physmap (!range) cases. > >> If the field was left uninitialized, >> -ESRCH would be as valid as -EINVAL, and I don't think we >> specify for any hypercall that between multiple applicable >> error code a specific one would be preferred over others. > > I think given the above -ESRCH is invalid, since there is no field to > have been left uninitialised. Oh, I finally understand. The thing that mainly confused me here is the wording of the original comment: "Foreign mapping is only supported by add_to_physmap_range". Whereas it should have said "Foreign mapping is only possible through add_to_physmap_range". I'll re-add that with the adjusted comment. Will that make the patch acceptable to you? Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code 2013-12-20 9:12 ` Jan Beulich @ 2013-12-20 9:55 ` Ian Campbell 0 siblings, 0 replies; 47+ messages in thread From: Ian Campbell @ 2013-12-20 9:55 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 09:12 +0000, Jan Beulich wrote: > >>> On 20.12.13 at 09:58, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Fri, 2013-12-20 at 07:48 +0000, Jan Beulich wrote: > >> >>> On 18.12.13 at 18:52, Ian Campbell <Ian.Campbell@citrix.com> wrote: > >> > On Wed, 2013-12-18 at 14:34 +0000, Jan Beulich wrote: > >> >> There's really nothing really architecture specific here; the > >> >> architecture specific handling is limited to > >> >> xenmem_add_to_physmap_one(). > >> >> > >> >> Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being > >> >> supported through XENMEM_add_to_physmap_range is being dropped as being > >> >> pointless. > >> > > >> > It's not pointless, dropping this restriction turns a comprehensible > >> > EINVAL result into an incorrect ESRCH result. Which is incorrect because > >> > no domain was specified so how can it not exist. > >> > >> How can there have been no domain specified for a > >> XENMAPSPACE_gmfn_foreign? > > > > The arguments to XENMEM_add_to_physmap have no field which specifies a > > foreign domain. (this is one of the reasons we added the _range version > > with the field in the first place). > > > > The domid parameter to add_to_physmap one is why hardcoded to > > DOMID_INVALID in all the add_to_physmap (!range) cases. > > > >> If the field was left uninitialized, > >> -ESRCH would be as valid as -EINVAL, and I don't think we > >> specify for any hypercall that between multiple applicable > >> error code a specific one would be preferred over others. > > > > I think given the above -ESRCH is invalid, since there is no field to > > have been left uninitialised. > > Oh, I finally understand. The thing that mainly confused me here > is the wording of the original comment: "Foreign mapping is only > supported by add_to_physmap_range". Whereas it should have > said "Foreign mapping is only possible through > add_to_physmap_range". I'll re-add that with the adjusted > comment. Yes, thanks. > Will that make the patch acceptable to you? I believe so, yes, I'll look at v3 ASAP. Ian. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-18 14:29 [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich 2013-12-18 14:34 ` [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich @ 2013-12-18 14:35 ` Jan Beulich 2013-12-18 15:48 ` Tim Deegan 2013-12-18 17:59 ` Ian Campbell 2013-12-18 14:35 ` [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich ` (4 subsequent siblings) 6 siblings, 2 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-18 14:35 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 5309 bytes --] Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -190,6 +190,9 @@ int compat_memory_op(unsigned int cmd, X break; case XENMEM_add_to_physmap: + BUILD_BUG_ON((typeof(cmp.atp.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + if ( copy_from_guest(&cmp.atp, compat, 1) ) return -EFAULT; @@ -322,17 +325,8 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_current_reservation: case XENMEM_maximum_reservation: case XENMEM_maximum_gpfn: - case XENMEM_remove_from_physmap: - break; - case XENMEM_add_to_physmap: - if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp.atp, nat.atp); - if ( __copy_to_guest(compat, &cmp.atp, 1) ) - rc = -EFAULT; - + case XENMEM_remove_from_physmap: break; default: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -543,22 +543,32 @@ static long memory_exchange(XEN_GUEST_HA } static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) + struct xen_add_to_physmap *xatp, + unsigned int start) { - struct xen_add_to_physmap start_xatp; - int rc = 0; + unsigned int done = 0; + long rc = 0; if ( xatp->space != XENMAPSPACE_gmfn_range ) + { + ASSERT(!start); return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); + } + + if ( xatp->size < start ) + return -EILSEQ; + + xatp->idx += start; + xatp->gpfn += start; + xatp->size -= start; #ifdef HAS_PASSTHROUGH if ( need_iommu(d) ) this_cpu(iommu_dont_flush_iotlb) = 1; #endif - start_xatp = *xatp; - while ( xatp->size > 0 ) + while ( xatp->size > done ) { rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); @@ -567,12 +577,11 @@ static int xenmem_add_to_physmap(struct xatp->idx++; xatp->gpfn++; - xatp->size--; /* Check for continuation if it's not the last interation. */ - if ( xatp->size > 0 && hypercall_preempt_check() ) + if ( xatp->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; break; } } @@ -581,8 +590,8 @@ static int xenmem_add_to_physmap(struct if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, xatp->idx - done, done); + iommu_iotlb_flush(d, xatp->gpfn - done, done); } #endif @@ -594,10 +603,10 @@ long do_memory_op(unsigned long cmd, XEN struct domain *d; long rc; unsigned int address_bits; - unsigned long start_extent; struct xen_memory_reservation reservation; struct memop_args args; domid_t domid; + unsigned long start_extent = cmd >> MEMOP_EXTENT_SHIFT; int op = cmd & MEMOP_CMD_MASK; switch ( op ) @@ -605,8 +614,6 @@ long do_memory_op(unsigned long cmd, XEN case XENMEM_increase_reservation: case XENMEM_decrease_reservation: case XENMEM_populate_physmap: - start_extent = cmd >> MEMOP_EXTENT_SHIFT; - if ( copy_from_guest(&reservation, arg, 1) ) return start_extent; @@ -724,6 +731,12 @@ long do_memory_op(unsigned long cmd, XEN { struct xen_add_to_physmap xatp; + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for faked input. */ + if ( start_extent != (typeof(xatp.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; @@ -738,18 +751,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap(d, &xatp); + rc = xenmem_add_to_physmap(d, &xatp, start_extent); rcu_unlock_domain(d); - if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatp, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( xatp.space == XENMAPSPACE_gmfn_range && rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #2: xatp-preemption.patch --] [-- Type: text/plain, Size: 5354 bytes --] fix XENMEM_add_to_physmap preemption handling Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -190,6 +190,9 @@ int compat_memory_op(unsigned int cmd, X break; case XENMEM_add_to_physmap: + BUILD_BUG_ON((typeof(cmp.atp.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + if ( copy_from_guest(&cmp.atp, compat, 1) ) return -EFAULT; @@ -322,17 +325,8 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_current_reservation: case XENMEM_maximum_reservation: case XENMEM_maximum_gpfn: - case XENMEM_remove_from_physmap: - break; - case XENMEM_add_to_physmap: - if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp.atp, nat.atp); - if ( __copy_to_guest(compat, &cmp.atp, 1) ) - rc = -EFAULT; - + case XENMEM_remove_from_physmap: break; default: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -543,22 +543,32 @@ static long memory_exchange(XEN_GUEST_HA } static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) + struct xen_add_to_physmap *xatp, + unsigned int start) { - struct xen_add_to_physmap start_xatp; - int rc = 0; + unsigned int done = 0; + long rc = 0; if ( xatp->space != XENMAPSPACE_gmfn_range ) + { + ASSERT(!start); return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); + } + + if ( xatp->size < start ) + return -EILSEQ; + + xatp->idx += start; + xatp->gpfn += start; + xatp->size -= start; #ifdef HAS_PASSTHROUGH if ( need_iommu(d) ) this_cpu(iommu_dont_flush_iotlb) = 1; #endif - start_xatp = *xatp; - while ( xatp->size > 0 ) + while ( xatp->size > done ) { rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); @@ -567,12 +577,11 @@ static int xenmem_add_to_physmap(struct xatp->idx++; xatp->gpfn++; - xatp->size--; /* Check for continuation if it's not the last interation. */ - if ( xatp->size > 0 && hypercall_preempt_check() ) + if ( xatp->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; break; } } @@ -581,8 +590,8 @@ static int xenmem_add_to_physmap(struct if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, xatp->idx - done, done); + iommu_iotlb_flush(d, xatp->gpfn - done, done); } #endif @@ -594,10 +603,10 @@ long do_memory_op(unsigned long cmd, XEN struct domain *d; long rc; unsigned int address_bits; - unsigned long start_extent; struct xen_memory_reservation reservation; struct memop_args args; domid_t domid; + unsigned long start_extent = cmd >> MEMOP_EXTENT_SHIFT; int op = cmd & MEMOP_CMD_MASK; switch ( op ) @@ -605,8 +614,6 @@ long do_memory_op(unsigned long cmd, XEN case XENMEM_increase_reservation: case XENMEM_decrease_reservation: case XENMEM_populate_physmap: - start_extent = cmd >> MEMOP_EXTENT_SHIFT; - if ( copy_from_guest(&reservation, arg, 1) ) return start_extent; @@ -724,6 +731,12 @@ long do_memory_op(unsigned long cmd, XEN { struct xen_add_to_physmap xatp; + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for faked input. */ + if ( start_extent != (typeof(xatp.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; @@ -738,18 +751,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap(d, &xatp); + rc = xenmem_add_to_physmap(d, &xatp, start_extent); rcu_unlock_domain(d); - if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatp, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( xatp.space == XENMAPSPACE_gmfn_range && rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-18 14:35 ` [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling Jan Beulich @ 2013-12-18 15:48 ` Tim Deegan 2013-12-18 16:03 ` Jan Beulich 2013-12-18 17:59 ` Ian Campbell 1 sibling, 1 reply; 47+ messages in thread From: Tim Deegan @ 2013-12-18 15:48 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Stefano Stabellini At 14:35 +0000 on 18 Dec (1387373707), Jan Beulich wrote: > Just like for all other hypercalls we shouldn't be modifying the input > structure - all of the fields are, even if not explicitly documented, > just inputs. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > @@ -543,22 +543,32 @@ static long memory_exchange(XEN_GUEST_HA > } > > static int xenmem_add_to_physmap(struct domain *d, > - struct xen_add_to_physmap *xatp) > + struct xen_add_to_physmap *xatp, > + unsigned int start) > { > - struct xen_add_to_physmap start_xatp; > - int rc = 0; > + unsigned int done = 0; > + long rc = 0; > > if ( xatp->space != XENMAPSPACE_gmfn_range ) > + { > + ASSERT(!start); I don't think you've enforced this in the caller; you only check that the guest hasn't supplied an over-sized start-extent. I think it's fine just to ignore start for singleton operations anyway. Cheers, Tim. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-18 15:48 ` Tim Deegan @ 2013-12-18 16:03 ` Jan Beulich 2013-12-18 16:04 ` Tim Deegan 0 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-18 16:03 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel, Keir Fraser, Ian Campbell, Stefano Stabellini >>> On 18.12.13 at 16:48, Tim Deegan <tim@xen.org> wrote: > At 14:35 +0000 on 18 Dec (1387373707), Jan Beulich wrote: >> Just like for all other hypercalls we shouldn't be modifying the input >> structure - all of the fields are, even if not explicitly documented, >> just inputs. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> @@ -543,22 +543,32 @@ static long memory_exchange(XEN_GUEST_HA >> } >> >> static int xenmem_add_to_physmap(struct domain *d, >> - struct xen_add_to_physmap *xatp) >> + struct xen_add_to_physmap *xatp, >> + unsigned int start) >> { >> - struct xen_add_to_physmap start_xatp; >> - int rc = 0; >> + unsigned int done = 0; >> + long rc = 0; >> >> if ( xatp->space != XENMAPSPACE_gmfn_range ) >> + { >> + ASSERT(!start); > > I don't think you've enforced this in the caller; you only check that > the guest hasn't supplied an over-sized start-extent. I think it's > fine just to ignore start for singleton operations anyway. Right, if at all I should be returning an error here. But that should perhaps either done uniformly at once for all mem ops, or not at all. I'll just drop that change - makes the patch smaller :-) Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-18 16:03 ` Jan Beulich @ 2013-12-18 16:04 ` Tim Deegan 0 siblings, 0 replies; 47+ messages in thread From: Tim Deegan @ 2013-12-18 16:04 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Stefano Stabellini At 16:03 +0000 on 18 Dec (1387378993), Jan Beulich wrote: > >>> On 18.12.13 at 16:48, Tim Deegan <tim@xen.org> wrote: > > At 14:35 +0000 on 18 Dec (1387373707), Jan Beulich wrote: > >> Just like for all other hypercalls we shouldn't be modifying the input > >> structure - all of the fields are, even if not explicitly documented, > >> just inputs. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > >> @@ -543,22 +543,32 @@ static long memory_exchange(XEN_GUEST_HA > >> } > >> > >> static int xenmem_add_to_physmap(struct domain *d, > >> - struct xen_add_to_physmap *xatp) > >> + struct xen_add_to_physmap *xatp, > >> + unsigned int start) > >> { > >> - struct xen_add_to_physmap start_xatp; > >> - int rc = 0; > >> + unsigned int done = 0; > >> + long rc = 0; > >> > >> if ( xatp->space != XENMAPSPACE_gmfn_range ) > >> + { > >> + ASSERT(!start); > > > > I don't think you've enforced this in the caller; you only check that > > the guest hasn't supplied an over-sized start-extent. I think it's > > fine just to ignore start for singleton operations anyway. > > Right, if at all I should be returning an error here. But that should > perhaps either done uniformly at once for all mem ops, or not at > all. > > I'll just drop that change - makes the patch smaller :-) Grand so. :) With that dropped, 1/4 and 2/4 are Reviewed-by: Tim Deegan <tim@xen.org> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-18 14:35 ` [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling Jan Beulich 2013-12-18 15:48 ` Tim Deegan @ 2013-12-18 17:59 ` Ian Campbell 2013-12-20 7:52 ` Jan Beulich 1 sibling, 1 reply; 47+ messages in thread From: Ian Campbell @ 2013-12-18 17:59 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Wed, 2013-12-18 at 14:35 +0000, Jan Beulich wrote: > J > /* Check for continuation if it's not the last interation. */ > - if ( xatp->size > 0 && hypercall_preempt_check() ) > + if ( xatp->size > ++done && hypercall_preempt_check() ) Hiding the loop increment inside the preemption checking is a bit subtle (it took me a while to find it). Can't it go either before or after this loop? With a suitable +/- 1 to the rc below if necessary. > @@ -724,6 +731,12 @@ long do_memory_op(unsigned long cmd, XEN > { > struct xen_add_to_physmap xatp; > > + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); What does "typeof(xatp.size)-1" evaluate to? typeof can't return an int, cant it? > + > + /* Check for faked input. */ Faked as in "malicious" or faked as in "something we made up for continuation purposes" ? ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-18 17:59 ` Ian Campbell @ 2013-12-20 7:52 ` Jan Beulich 2013-12-20 9:02 ` Ian Campbell 0 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 7:52 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini >>> On 18.12.13 at 18:59, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Wed, 2013-12-18 at 14:35 +0000, Jan Beulich wrote: >> J >> /* Check for continuation if it's not the last interation. */ >> - if ( xatp->size > 0 && hypercall_preempt_check() ) >> + if ( xatp->size > ++done && hypercall_preempt_check() ) > > Hiding the loop increment inside the preemption checking is a bit subtle > (it took me a while to find it). Can't it go either before or after this > loop? With a suitable +/- 1 to the rc below if necessary. It could, but I find it quite natural to be together with the check. >> @@ -724,6 +731,12 @@ long do_memory_op(unsigned long cmd, XEN >> { >> struct xen_add_to_physmap xatp; >> >> + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); > > What does "typeof(xatp.size)-1" evaluate to? typeof can't return an int, > cant it? Properly parenthesized this is ((uint16_t)-1), i.e. 0xffff (typeof() doesn't produce a value, but a type). >> + >> + /* Check for faked input. */ > > Faked as in "malicious" or faked as in "something we made up for > continuation purposes" ? As in "malicious" (or buggy), i.e. the caller passing an out of range "cmd" to HYPERVISOR_memory_op. Do you have any better wording in mind? Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-20 7:52 ` Jan Beulich @ 2013-12-20 9:02 ` Ian Campbell 2013-12-20 9:16 ` Jan Beulich 0 siblings, 1 reply; 47+ messages in thread From: Ian Campbell @ 2013-12-20 9:02 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 07:52 +0000, Jan Beulich wrote: > >>> On 18.12.13 at 18:59, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Wed, 2013-12-18 at 14:35 +0000, Jan Beulich wrote: > >> J > >> /* Check for continuation if it's not the last interation. */ > >> - if ( xatp->size > 0 && hypercall_preempt_check() ) > >> + if ( xatp->size > ++done && hypercall_preempt_check() ) > > > > Hiding the loop increment inside the preemption checking is a bit subtle > > (it took me a while to find it). Can't it go either before or after this > > loop? With a suitable +/- 1 to the rc below if necessary. > > It could, but I find it quite natural to be together with the check. If it were the loop termination check or something then I would agree, but this is effectively some other arbitrary check within the loop (even if it does happen to exit, it's not the primary loop termination condition). > >> @@ -724,6 +731,12 @@ long do_memory_op(unsigned long cmd, XEN > >> { > >> struct xen_add_to_physmap xatp; > >> > >> + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); > > > > What does "typeof(xatp.size)-1" evaluate to? typeof can't return an int, > > cant it? > > Properly parenthesized this is ((uint16_t)-1), i.e. 0xffff Ah! Yes, I misread that rather badly. > (typeof() doesn't produce a value, but a type). I know, that's why I was so confused when it looked like it was being treated as a value! > >> + > >> + /* Check for faked input. */ > > > > Faked as in "malicious" or faked as in "something we made up for > > continuation purposes" ? > > As in "malicious" (or buggy), i.e. the caller passing an out of range > "cmd" to HYPERVISOR_memory_op. Do you have any better wording > in mind? "Check for malicious or buggy input" would do the job. (with or without "or buggy") Ian. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-20 9:02 ` Ian Campbell @ 2013-12-20 9:16 ` Jan Beulich 0 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 9:16 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini >>> On 20.12.13 at 10:02, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Fri, 2013-12-20 at 07:52 +0000, Jan Beulich wrote: >> >>> On 18.12.13 at 18:59, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > On Wed, 2013-12-18 at 14:35 +0000, Jan Beulich wrote: >> >> + >> >> + /* Check for faked input. */ >> > >> > Faked as in "malicious" or faked as in "something we made up for >> > continuation purposes" ? >> >> As in "malicious" (or buggy), i.e. the caller passing an out of range >> "cmd" to HYPERVISOR_memory_op. Do you have any better wording >> in mind? > > "Check for malicious or buggy input" would do the job. (with or without > "or buggy") Fine with me - changed that way. Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-18 14:29 [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich 2013-12-18 14:34 ` [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich 2013-12-18 14:35 ` [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling Jan Beulich @ 2013-12-18 14:35 ` Jan Beulich 2013-12-18 15:59 ` Tim Deegan 2013-12-19 10:48 ` Ian Campbell 2013-12-18 14:36 ` [PATCH 4/4] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich ` (3 subsequent siblings) 6 siblings, 2 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-18 14:35 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 6096 bytes --] There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). Note the restriction of XENMAPSPACE_gmfn_range only being supported through XENMEM_add_to_physmap is being dropped as being pointless. This further eliminates the erroneous bailing from xenmem_add_to_physmap_range() if xenmem_add_to_physmap_one() fails. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1056,103 +1056,10 @@ int xenmem_add_to_physmap_one( return rc; } -static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) -{ - int rc; - - while ( xatpr->size > 0 ) - { - xen_ulong_t idx; - xen_pfn_t gpfn; - - if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - rc = xenmem_add_to_physmap_one(d, xatpr->space, - xatpr->foreign_domid, - idx, gpfn); - - if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( rc < 0 ) - goto out; - - guest_handle_add_offset(xatpr->idxs, 1); - guest_handle_add_offset(xatpr->gpfns, 1); - guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - goto out; - } - } - - rc = 0; - -out: - return rc; - -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { - int rc; - switch ( op ) { - case XENMEM_add_to_physmap_range: - { - struct xen_add_to_physmap_range xatpr; - struct domain *d; - - if ( copy_from_guest(&xatpr, arg, 1) ) - return -EFAULT; - - /* This mapspace is redundant for this hypercall */ - if ( xatpr.space == XENMAPSPACE_gmfn_range ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatpr.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_range(d, &xatpr); - - rcu_unlock_domain(d); - - if ( rc && copy_to_guest(arg, &xatpr, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - - return rc; - } /* XXX: memsharing not working yet */ case XENMEM_get_sharing_shared_pages: case XENMEM_get_sharing_freed_pages: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -598,6 +598,57 @@ static int xenmem_add_to_physmap(struct return rc; } +static int xenmem_add_to_physmap_range(struct domain *d, + struct xen_add_to_physmap_range *xatpr) +{ + int rc; + + while ( xatpr->size > 0 ) + { + xen_ulong_t idx; + xen_pfn_t gpfn; + + if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + rc = xenmem_add_to_physmap_one(d, xatpr->space, + xatpr->foreign_domid, + idx, gpfn); + + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) + { + rc = -EFAULT; + goto out; + } + + guest_handle_add_offset(xatpr->idxs, 1); + guest_handle_add_offset(xatpr->gpfns, 1); + guest_handle_add_offset(xatpr->errs, 1); + xatpr->size--; + + /* Check for continuation if it's not the last interation. */ + if ( xatpr->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + goto out; + } + } + + rc = 0; + +out: + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -763,6 +814,44 @@ long do_memory_op(unsigned long cmd, XEN return rc; } + case XENMEM_add_to_physmap_range: + { + struct xen_add_to_physmap_range xatpr; + struct domain *d; + + if ( copy_from_guest(&xatpr, arg, 1) || + !guest_handle_okay(xatpr.idxs, xatpr.size) || + !guest_handle_okay(xatpr.gpfns, xatpr.size) || + !guest_handle_okay(xatpr.errs, xatpr.size) ) + return -EFAULT; + + d = rcu_lock_domain_by_any_id(xatpr.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap_range(d, &xatpr); + + rcu_unlock_domain(d); + + if ( rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatpr, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; [-- Attachment #2: xatpr-common.patch --] [-- Type: text/plain, Size: 6162 bytes --] move XENMEM_add_to_physmap_range handling framework to common code There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). Note the restriction of XENMAPSPACE_gmfn_range only being supported through XENMEM_add_to_physmap is being dropped as being pointless. This further eliminates the erroneous bailing from xenmem_add_to_physmap_range() if xenmem_add_to_physmap_one() fails. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1056,103 +1056,10 @@ int xenmem_add_to_physmap_one( return rc; } -static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) -{ - int rc; - - while ( xatpr->size > 0 ) - { - xen_ulong_t idx; - xen_pfn_t gpfn; - - if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - rc = xenmem_add_to_physmap_one(d, xatpr->space, - xatpr->foreign_domid, - idx, gpfn); - - if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( rc < 0 ) - goto out; - - guest_handle_add_offset(xatpr->idxs, 1); - guest_handle_add_offset(xatpr->gpfns, 1); - guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - goto out; - } - } - - rc = 0; - -out: - return rc; - -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { - int rc; - switch ( op ) { - case XENMEM_add_to_physmap_range: - { - struct xen_add_to_physmap_range xatpr; - struct domain *d; - - if ( copy_from_guest(&xatpr, arg, 1) ) - return -EFAULT; - - /* This mapspace is redundant for this hypercall */ - if ( xatpr.space == XENMAPSPACE_gmfn_range ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatpr.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_range(d, &xatpr); - - rcu_unlock_domain(d); - - if ( rc && copy_to_guest(arg, &xatpr, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - - return rc; - } /* XXX: memsharing not working yet */ case XENMEM_get_sharing_shared_pages: case XENMEM_get_sharing_freed_pages: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -598,6 +598,57 @@ static int xenmem_add_to_physmap(struct return rc; } +static int xenmem_add_to_physmap_range(struct domain *d, + struct xen_add_to_physmap_range *xatpr) +{ + int rc; + + while ( xatpr->size > 0 ) + { + xen_ulong_t idx; + xen_pfn_t gpfn; + + if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + rc = xenmem_add_to_physmap_one(d, xatpr->space, + xatpr->foreign_domid, + idx, gpfn); + + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) + { + rc = -EFAULT; + goto out; + } + + guest_handle_add_offset(xatpr->idxs, 1); + guest_handle_add_offset(xatpr->gpfns, 1); + guest_handle_add_offset(xatpr->errs, 1); + xatpr->size--; + + /* Check for continuation if it's not the last interation. */ + if ( xatpr->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + goto out; + } + } + + rc = 0; + +out: + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -763,6 +814,44 @@ long do_memory_op(unsigned long cmd, XEN return rc; } + case XENMEM_add_to_physmap_range: + { + struct xen_add_to_physmap_range xatpr; + struct domain *d; + + if ( copy_from_guest(&xatpr, arg, 1) || + !guest_handle_okay(xatpr.idxs, xatpr.size) || + !guest_handle_okay(xatpr.gpfns, xatpr.size) || + !guest_handle_okay(xatpr.errs, xatpr.size) ) + return -EFAULT; + + d = rcu_lock_domain_by_any_id(xatpr.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap_range(d, &xatpr); + + rcu_unlock_domain(d); + + if ( rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatpr, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-18 14:35 ` [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich @ 2013-12-18 15:59 ` Tim Deegan 2013-12-18 16:08 ` Jan Beulich 2013-12-19 10:48 ` Ian Campbell 1 sibling, 1 reply; 47+ messages in thread From: Tim Deegan @ 2013-12-18 15:59 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Stefano Stabellini At 14:35 +0000 on 18 Dec (1387373737), Jan Beulich wrote: > There's really nothing really architecture specific here; the > architecture specific handling is limited to > xenmem_add_to_physmap_one(). > > Note the restriction of XENMAPSPACE_gmfn_range only being supported > through XENMEM_add_to_physmap is being dropped as being pointless. Hmmm. With that restriction removed, this: > + rc = xenmem_add_to_physmap_one(d, xatpr->space, > + xatpr->foreign_domid, > + idx, gpfn); > + > + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) > + { > + rc = -EFAULT; > + goto out; > + } > + > + guest_handle_add_offset(xatpr->idxs, 1); > + guest_handle_add_offset(xatpr->gpfns, 1); > + guest_handle_add_offset(xatpr->errs, 1); > + xatpr->size--; > + > + /* Check for continuation if it's not the last interation. */ > + if ( xatpr->size > 0 && hypercall_preempt_check() ) > + { > + rc = -EAGAIN; > + goto out; > + } will save the partial result of a preempted XENMAPSPACE_gmfn_range sub-op into the errors array, and then the whole operation may or may not return -EAGAIN. I guess the caller now has enough information to figure out what happened and restart, but given that the rest of this interface nicely hides preemption from the caller, it's a bit of an ugly corner case. Tim. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-18 15:59 ` Tim Deegan @ 2013-12-18 16:08 ` Jan Beulich 2013-12-18 16:14 ` Tim Deegan 0 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-18 16:08 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel, Keir Fraser, Ian Campbell, Stefano Stabellini >>> On 18.12.13 at 16:59, Tim Deegan <tim@xen.org> wrote: > At 14:35 +0000 on 18 Dec (1387373737), Jan Beulich wrote: >> There's really nothing really architecture specific here; the >> architecture specific handling is limited to >> xenmem_add_to_physmap_one(). >> >> Note the restriction of XENMAPSPACE_gmfn_range only being supported >> through XENMEM_add_to_physmap is being dropped as being pointless. > > Hmmm. With that restriction removed, this: > >> + rc = xenmem_add_to_physmap_one(d, xatpr->space, >> + xatpr->foreign_domid, >> + idx, gpfn); >> + >> + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) >> + { >> + rc = -EFAULT; >> + goto out; >> + } >> + >> + guest_handle_add_offset(xatpr->idxs, 1); >> + guest_handle_add_offset(xatpr->gpfns, 1); >> + guest_handle_add_offset(xatpr->errs, 1); >> + xatpr->size--; >> + >> + /* Check for continuation if it's not the last interation. */ >> + if ( xatpr->size > 0 && hypercall_preempt_check() ) >> + { >> + rc = -EAGAIN; >> + goto out; >> + } > > will save the partial result of a preempted XENMAPSPACE_gmfn_range > sub-op into the errors array, and then the whole operation may or may > not return -EAGAIN. > > I guess the caller now has enough information to figure out what > happened and restart, but given that the rest of this interface nicely > hides preemption from the caller, it's a bit of an ugly corner case. I'm afraid I don't follow: XENMAPSPACE_gmfn_range and XENMAPSPACE_gmfn will be treated equally now - there simply is no range being specified for the individual add_to_physmap_range elements. After all, xenmem_add_to_physmap_one() - as its name says - really only processes a single page. Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-18 16:08 ` Jan Beulich @ 2013-12-18 16:14 ` Tim Deegan 0 siblings, 0 replies; 47+ messages in thread From: Tim Deegan @ 2013-12-18 16:14 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Campbell, Stefano Stabellini At 16:08 +0000 on 18 Dec (1387379324), Jan Beulich wrote: > >>> On 18.12.13 at 16:59, Tim Deegan <tim@xen.org> wrote: > > At 14:35 +0000 on 18 Dec (1387373737), Jan Beulich wrote: > >> There's really nothing really architecture specific here; the > >> architecture specific handling is limited to > >> xenmem_add_to_physmap_one(). > >> > >> Note the restriction of XENMAPSPACE_gmfn_range only being supported > >> through XENMEM_add_to_physmap is being dropped as being pointless. > > > > Hmmm. With that restriction removed, this: > > > >> + rc = xenmem_add_to_physmap_one(d, xatpr->space, > >> + xatpr->foreign_domid, > >> + idx, gpfn); > >> + > >> + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) > >> + { > >> + rc = -EFAULT; > >> + goto out; > >> + } > >> + > >> + guest_handle_add_offset(xatpr->idxs, 1); > >> + guest_handle_add_offset(xatpr->gpfns, 1); > >> + guest_handle_add_offset(xatpr->errs, 1); > >> + xatpr->size--; > >> + > >> + /* Check for continuation if it's not the last interation. */ > >> + if ( xatpr->size > 0 && hypercall_preempt_check() ) > >> + { > >> + rc = -EAGAIN; > >> + goto out; > >> + } > > > > will save the partial result of a preempted XENMAPSPACE_gmfn_range > > sub-op into the errors array, and then the whole operation may or may > > not return -EAGAIN. > > > > I guess the caller now has enough information to figure out what > > happened and restart, but given that the rest of this interface nicely > > hides preemption from the caller, it's a bit of an ugly corner case. > > I'm afraid I don't follow: XENMAPSPACE_gmfn_range and > XENMAPSPACE_gmfn will be treated equally now - there simply > is no range being specified for the individual > add_to_physmap_range elements. Oh of course. Then it's just a slightly odd API, but I guess no odder this way than before, and with slightly less code. :) So Reviewed-by: Tim Deegan <tim@xen.org> for 3/4 and 4/4 as they stand. Cheers, Tim. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-18 14:35 ` [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich 2013-12-18 15:59 ` Tim Deegan @ 2013-12-19 10:48 ` Ian Campbell 2013-12-20 7:58 ` Jan Beulich 1 sibling, 1 reply; 47+ messages in thread From: Ian Campbell @ 2013-12-19 10:48 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan, Stefano Stabellini On Wed, 2013-12-18 at 14:35 +0000, Jan Beulich wrote: > There's really nothing really architecture specific here; the > architecture specific handling is limited to > xenmem_add_to_physmap_one(). > > Note the restriction of XENMAPSPACE_gmfn_range only being supported > through XENMEM_add_to_physmap is being dropped as being pointless. Note that this will still not work on ARM where add_to_physmap_one() does not support the gmfn_range and will still return ENOSYS. If you are dropping this restriction then xen/include/public/memory.h will need updating (likewise the first patch). Personally I think that having two redundant ways of doing the same thing is the more pointless of the two. Especially given the need to think a bit carefully about what XENMEM_add_to_physmap_range XENMAPSPACE_gmfn_range might mean... > This further eliminates the erroneous bailing from > xenmem_add_to_physmap_range() if xenmem_add_to_physmap_one() fails. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -1056,103 +1056,10 @@ int xenmem_add_to_physmap_one( > return rc; > } > > -static int xenmem_add_to_physmap_range(struct domain *d, > - struct xen_add_to_physmap_range *xatpr) > -{ > - int rc; > - > - while ( xatpr->size > 0 ) > - { > - xen_ulong_t idx; > - xen_pfn_t gpfn; > - > - if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) > - { > - rc = -EFAULT; > - goto out; > - } > - > - if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) > - { > - rc = -EFAULT; > - goto out; > - } > - > - rc = xenmem_add_to_physmap_one(d, xatpr->space, > - xatpr->foreign_domid, > - idx, gpfn); > - > - if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) > - { > - rc = -EFAULT; > - goto out; > - } > - > - if ( rc < 0 ) > - goto out; > - > - guest_handle_add_offset(xatpr->idxs, 1); > - guest_handle_add_offset(xatpr->gpfns, 1); > - guest_handle_add_offset(xatpr->errs, 1); > - xatpr->size--; > - > - /* Check for continuation if it's not the last interation */ > - if ( xatpr->size > 0 && hypercall_preempt_check() ) > - { > - rc = -EAGAIN; > - goto out; > - } > - } > - > - rc = 0; > - > -out: > - return rc; > - > -} > - > long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) > { > - int rc; > - > switch ( op ) > { > - case XENMEM_add_to_physmap_range: > - { > - struct xen_add_to_physmap_range xatpr; > - struct domain *d; > - > - if ( copy_from_guest(&xatpr, arg, 1) ) > - return -EFAULT; > - > - /* This mapspace is redundant for this hypercall */ > - if ( xatpr.space == XENMAPSPACE_gmfn_range ) > - return -EINVAL; > - > - d = rcu_lock_domain_by_any_id(xatpr.domid); > - if ( d == NULL ) > - return -ESRCH; > - > - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); > - if ( rc ) > - { > - rcu_unlock_domain(d); > - return rc; > - } > - > - rc = xenmem_add_to_physmap_range(d, &xatpr); > - > - rcu_unlock_domain(d); > - > - if ( rc && copy_to_guest(arg, &xatpr, 1) ) > - rc = -EFAULT; > - > - if ( rc == -EAGAIN ) > - rc = hypercall_create_continuation( > - __HYPERVISOR_memory_op, "ih", op, arg); > - > - return rc; > - } > /* XXX: memsharing not working yet */ > case XENMEM_get_sharing_shared_pages: > case XENMEM_get_sharing_freed_pages: > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -598,6 +598,57 @@ static int xenmem_add_to_physmap(struct > return rc; > } > > +static int xenmem_add_to_physmap_range(struct domain *d, > + struct xen_add_to_physmap_range *xatpr) > +{ > + int rc; > + > + while ( xatpr->size > 0 ) > + { > + xen_ulong_t idx; > + xen_pfn_t gpfn; > + > + if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) > + { > + rc = -EFAULT; > + goto out; > + } > + > + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) > + { > + rc = -EFAULT; > + goto out; > + } > + > + rc = xenmem_add_to_physmap_one(d, xatpr->space, > + xatpr->foreign_domid, > + idx, gpfn); > + > + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) > + { > + rc = -EFAULT; > + goto out; > + } > + > + guest_handle_add_offset(xatpr->idxs, 1); > + guest_handle_add_offset(xatpr->gpfns, 1); > + guest_handle_add_offset(xatpr->errs, 1); > + xatpr->size--; > + > + /* Check for continuation if it's not the last interation. */ > + if ( xatpr->size > 0 && hypercall_preempt_check() ) > + { > + rc = -EAGAIN; > + goto out; > + } > + } > + > + rc = 0; > + > +out: > + return rc; > +} > + > long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > { > struct domain *d; > @@ -763,6 +814,44 @@ long do_memory_op(unsigned long cmd, XEN > return rc; > } > > + case XENMEM_add_to_physmap_range: > + { > + struct xen_add_to_physmap_range xatpr; > + struct domain *d; > + > + if ( copy_from_guest(&xatpr, arg, 1) || > + !guest_handle_okay(xatpr.idxs, xatpr.size) || > + !guest_handle_okay(xatpr.gpfns, xatpr.size) || > + !guest_handle_okay(xatpr.errs, xatpr.size) ) > + return -EFAULT; > + > + d = rcu_lock_domain_by_any_id(xatpr.domid); > + if ( d == NULL ) > + return -ESRCH; > + > + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); > + if ( rc ) > + { > + rcu_unlock_domain(d); > + return rc; > + } > + > + rc = xenmem_add_to_physmap_range(d, &xatpr); > + > + rcu_unlock_domain(d); > + > + if ( rc == -EAGAIN ) > + { > + if ( !__copy_to_guest(arg, &xatpr, 1) ) > + rc = hypercall_create_continuation( > + __HYPERVISOR_memory_op, "ih", op, arg); > + else > + rc = -EFAULT; > + } > + > + return rc; > + } > + > case XENMEM_remove_from_physmap: > { > struct xen_remove_from_physmap xrfp; > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-19 10:48 ` Ian Campbell @ 2013-12-20 7:58 ` Jan Beulich 2013-12-20 9:09 ` Ian Campbell 0 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 7:58 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Tim Deegan, KeirFraser, Stefano Stabellini >>> On 19.12.13 at 11:48, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Wed, 2013-12-18 at 14:35 +0000, Jan Beulich wrote: >> There's really nothing really architecture specific here; the >> architecture specific handling is limited to >> xenmem_add_to_physmap_one(). >> >> Note the restriction of XENMAPSPACE_gmfn_range only being supported >> through XENMEM_add_to_physmap is being dropped as being pointless. > > Note that this will still not work on ARM where add_to_physmap_one() > does not support the gmfn_range and will still return ENOSYS. > > If you are dropping this restriction then xen/include/public/memory.h > will need updating (likewise the first patch). I don't think so - we can very well say certain combination are unsupported even if they happen to work. That leaves us with the option of dropping back to the earlier state should there be a need. > Personally I think that having two redundant ways of doing the same > thing is the more pointless of the two. Especially given the need to > think a bit carefully about what XENMEM_add_to_physmap_range > XENMAPSPACE_gmfn_range might mean... Yes, that combination is bogus whichever perspective you take. To some degree also because "range" in the former name is really wrong - the operation doesn't deal with a range, but with a group (batch) of individual pages. However, I meanwhile realized that from an interface perspective (I'm not proposing to implement this right now!) the combination could actually be made work - the errs array (being an output only at present) could be re-used to serve as input to pass the sizes. The main issue of implementing this would be how to deal with the two necessary layers of preemption. So I'm going to re-add the check. Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-20 7:58 ` Jan Beulich @ 2013-12-20 9:09 ` Ian Campbell 2013-12-20 9:27 ` Jan Beulich 0 siblings, 1 reply; 47+ messages in thread From: Ian Campbell @ 2013-12-20 9:09 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, KeirFraser, Stefano Stabellini On Fri, 2013-12-20 at 07:58 +0000, Jan Beulich wrote: > >>> On 19.12.13 at 11:48, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > Personally I think that having two redundant ways of doing the same > > thing is the more pointless of the two. Especially given the need to > > think a bit carefully about what XENMEM_add_to_physmap_range > > XENMAPSPACE_gmfn_range might mean... > > Yes, that combination is bogus whichever perspective you take. > To some degree also because "range" in the former name is really > wrong - the operation doesn't deal with a range, but with a group > (batch) of individual pages. That's true. I suppose there would be no harm in #define XENMEM_add_to_physmap_batch 23 #if __XEN_INTERFACE_VERSION__ < xxx #define XENMEM_add_to_physmap_range XENMEM_add_to_physmap_batch. #endif > However, I meanwhile realized that from an interface perspective > (I'm not proposing to implement this right now!) the combination > could actually be made work - the errs array (being an output only > at present) could be re-used to serve as input to pass the sizes. > The main issue of implementing this would be how to deal with the > two necessary layers of preemption. That's pretty cunning! > So I'm going to re-add the check. Given the above I'm doubly in favour of doing so. Ian. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-20 9:09 ` Ian Campbell @ 2013-12-20 9:27 ` Jan Beulich 0 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 9:27 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Tim Deegan, KeirFraser, Stefano Stabellini >>> On 20.12.13 at 10:09, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Fri, 2013-12-20 at 07:58 +0000, Jan Beulich wrote: >> >>> On 19.12.13 at 11:48, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > Personally I think that having two redundant ways of doing the same >> > thing is the more pointless of the two. Especially given the need to >> > think a bit carefully about what XENMEM_add_to_physmap_range >> > XENMAPSPACE_gmfn_range might mean... >> >> Yes, that combination is bogus whichever perspective you take. >> To some degree also because "range" in the former name is really >> wrong - the operation doesn't deal with a range, but with a group >> (batch) of individual pages. > > That's true. I suppose there would be no harm in > #define XENMEM_add_to_physmap_batch 23 > #if __XEN_INTERFACE_VERSION__ < xxx > #define XENMEM_add_to_physmap_range XENMEM_add_to_physmap_batch. > #endif Indeed - patch being prepared. Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 4/4] fix XENMEM_add_to_physmap_range preemption handling 2013-12-18 14:29 [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich ` (2 preceding siblings ...) 2013-12-18 14:35 ` [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich @ 2013-12-18 14:36 ` Jan Beulich 2013-12-18 14:43 ` [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Ian Campbell ` (2 subsequent siblings) 6 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-18 14:36 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 3036 bytes --] Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs (the one OUT one really refers to the memory pointed to by that handle rather than the handle itself). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -599,11 +599,21 @@ static int xenmem_add_to_physmap(struct } static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) + struct xen_add_to_physmap_range *xatpr, + unsigned int start) { + unsigned int done = 0; int rc; - while ( xatpr->size > 0 ) + if ( xatpr->size < start ) + return -EILSEQ; + + guest_handle_add_offset(xatpr->idxs, start); + guest_handle_add_offset(xatpr->gpfns, start); + guest_handle_add_offset(xatpr->errs, start); + xatpr->size -= start; + + while ( xatpr->size > done ) { xen_ulong_t idx; xen_pfn_t gpfn; @@ -633,12 +643,11 @@ static int xenmem_add_to_physmap_range(s guest_handle_add_offset(xatpr->idxs, 1); guest_handle_add_offset(xatpr->gpfns, 1); guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; /* Check for continuation if it's not the last interation. */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) + if ( xatpr->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; goto out; } } @@ -819,6 +828,13 @@ long do_memory_op(unsigned long cmd, XEN struct xen_add_to_physmap_range xatpr; struct domain *d; + BUILD_BUG_ON((typeof(xatpr.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for faked input. */ + if ( start_extent != (typeof(xatpr.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatpr, arg, 1) || !guest_handle_okay(xatpr.idxs, xatpr.size) || !guest_handle_okay(xatpr.gpfns, xatpr.size) || @@ -836,18 +852,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap_range(d, &xatpr); + rc = xenmem_add_to_physmap_range(d, &xatpr, start_extent); rcu_unlock_domain(d); - if ( rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatpr, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #2: xatpr-preemption.patch --] [-- Type: text/plain, Size: 3085 bytes --] fix XENMEM_add_to_physmap_range preemption handling Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs (the one OUT one really refers to the memory pointed to by that handle rather than the handle itself). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -599,11 +599,21 @@ static int xenmem_add_to_physmap(struct } static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) + struct xen_add_to_physmap_range *xatpr, + unsigned int start) { + unsigned int done = 0; int rc; - while ( xatpr->size > 0 ) + if ( xatpr->size < start ) + return -EILSEQ; + + guest_handle_add_offset(xatpr->idxs, start); + guest_handle_add_offset(xatpr->gpfns, start); + guest_handle_add_offset(xatpr->errs, start); + xatpr->size -= start; + + while ( xatpr->size > done ) { xen_ulong_t idx; xen_pfn_t gpfn; @@ -633,12 +643,11 @@ static int xenmem_add_to_physmap_range(s guest_handle_add_offset(xatpr->idxs, 1); guest_handle_add_offset(xatpr->gpfns, 1); guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; /* Check for continuation if it's not the last interation. */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) + if ( xatpr->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; goto out; } } @@ -819,6 +828,13 @@ long do_memory_op(unsigned long cmd, XEN struct xen_add_to_physmap_range xatpr; struct domain *d; + BUILD_BUG_ON((typeof(xatpr.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for faked input. */ + if ( start_extent != (typeof(xatpr.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatpr, arg, 1) || !guest_handle_okay(xatpr.idxs, xatpr.size) || !guest_handle_okay(xatpr.gpfns, xatpr.size) || @@ -836,18 +852,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap_range(d, &xatpr); + rc = xenmem_add_to_physmap_range(d, &xatpr, start_extent); rcu_unlock_domain(d); - if ( rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatpr, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} 2013-12-18 14:29 [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich ` (3 preceding siblings ...) 2013-12-18 14:36 ` [PATCH 4/4] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich @ 2013-12-18 14:43 ` Ian Campbell 2013-12-18 15:04 ` Jan Beulich 2013-12-18 17:15 ` Keir Fraser 2013-12-20 8:22 ` [PATCH v2 " Jan Beulich 6 siblings, 1 reply; 47+ messages in thread From: Ian Campbell @ 2013-12-18 14:43 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Wed, 2013-12-18 at 14:29 +0000, Jan Beulich wrote: > The hypervisor isn't supposed to use the input structures for > storing continuation information - only fields explicitly used as > hypercall outputs should ever be updated. > > Obviously this implies an ABI change, but since the previous > behavior was not intended to be that way I don't think we should > stick to the old behavior. FWIW I don't think adding const to a function parameter (which is kinda-sorta what we are doing here) constitutes a change to the ABI as such (or not a meaningful one), since a non-const can always be const'ed on call. > There's one caveat though - with the previous model, the caller > could - upon failure - use the updated structure to find out how > by progress was made. However, that wasn't intended afaict, > largely supported by this information depending on hypervisor > internals (i.e. the caller would have to know the order of request > processing and the meaning of the respective size fields, which > aren't simply saying "this much was processed"). IIRC the long standing user of the original physmap range functionality was hvmloader (something to do with batching IOMMU TLB flushes?). If hvmloader isn't introspecting hypervisor internals in this way I doubt anyone else is. I suppose this series is proposed for 4.5 rather than 4.4 since you didn't say otherwise? > > Consequently, as a follow-up we may want to consider making > explicit (and straight forward) this progress indication on error. > > 1: move XENMEM_add_to_physmap handling framework to common code > 2: fix XENMEM_add_to_physmap preemption handling > 3: move XENMEM_add_to_physmap_range handling framework to common code > 4: fix XENMEM_add_to_physmap_range preemption handling > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} 2013-12-18 14:43 ` [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Ian Campbell @ 2013-12-18 15:04 ` Jan Beulich 0 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-18 15:04 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini >>> On 18.12.13 at 15:43, Ian Campbell <Ian.Campbell@citrix.com> wrote: > I suppose this series is proposed for 4.5 rather than 4.4 since you > didn't say otherwise? Actually I'd be wanting this in 4.4 (and applicable parts even backported) since it's a bug fix (and as such I didn't think I'd need to say this explicitly). Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} 2013-12-18 14:29 [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich ` (4 preceding siblings ...) 2013-12-18 14:43 ` [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Ian Campbell @ 2013-12-18 17:15 ` Keir Fraser 2013-12-20 8:22 ` [PATCH v2 " Jan Beulich 6 siblings, 0 replies; 47+ messages in thread From: Keir Fraser @ 2013-12-18 17:15 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini On 18/12/2013 14:29, "Jan Beulich" <JBeulich@suse.com> wrote: > The hypervisor isn't supposed to use the input structures for > storing continuation information - only fields explicitly used as > hypercall outputs should ever be updated. > > Obviously this implies an ABI change, but since the previous > behavior was not intended to be that way I don't think we should > stick to the old behavior. > > There's one caveat though - with the previous model, the caller > could - upon failure - use the updated structure to find out how > by progress was made. However, that wasn't intended afaict, > largely supported by this information depending on hypervisor > internals (i.e. the caller would have to know the order of request > processing and the meaning of the respective size fields, which > aren't simply saying "this much was processed"). > > Consequently, as a follow-up we may want to consider making > explicit (and straight forward) this progress indication on error. > > 1: move XENMEM_add_to_physmap handling framework to common code > 2: fix XENMEM_add_to_physmap preemption handling > 3: move XENMEM_add_to_physmap_range handling framework to common code > 4: fix XENMEM_add_to_physmap_range preemption handling > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Nice to have this fixed! Acked-by: Keir Fraser <keir@xen.org> ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} 2013-12-18 14:29 [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich ` (5 preceding siblings ...) 2013-12-18 17:15 ` Keir Fraser @ 2013-12-20 8:22 ` Jan Beulich 2013-12-20 8:28 ` [PATCH v2 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich ` (4 more replies) 6 siblings, 5 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 8:22 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini >>> On 18.12.13 at 15:29, "Jan Beulich" <JBeulich@suse.com> wrote: The hypervisor isn't supposed to use the input structures for storing continuation information - only fields explicitly used as hypercall outputs should ever be updated. Obviously this implies an ABI change, but since the previous behavior was not intended to be that way I don't think we should stick to the old behavior. There's one caveat though - with the previous model, the caller could - upon failure - use the updated structure to find out how by progress was made. However, that wasn't intended afaict, largely supported by this information depending on hypervisor internals (i.e. the caller would have to know the order of request processing and the meaning of the respective size fields, which aren't simply saying "this much was processed"). Consequently, as a follow-up we may want to consider making explicit (and straight forward) this progress indication on error. 1: move XENMEM_add_to_physmap handling framework to common code 2: fix XENMEM_add_to_physmap preemption handling 3: move XENMEM_add_to_physmap_range handling framework to common code 4: fix XENMEM_add_to_physmap_range preemption handling Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: Apart from the removal of a bogus ASSERT() as requested by Tim, only a couple minor/cosmetic changes (see individual patches), hence I'm retaining the tags that were already given for v1. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 1/4] move XENMEM_add_to_physmap handling framework to common code 2013-12-20 8:22 ` [PATCH v2 " Jan Beulich @ 2013-12-20 8:28 ` Jan Beulich 2013-12-20 8:29 ` [PATCH v2 2/4] fix XENMEM_add_to_physmap preemption handling Jan Beulich ` (3 subsequent siblings) 4 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 8:28 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 14274 bytes --] There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being supported through XENMEM_add_to_physmap_range is being dropped as being pointless. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues share_xen_page_with_guest(page, dom_xen, readonly); } -static int xenmem_add_to_physmap_one( +int xenmem_add_to_physmap_one( struct domain *d, uint16_t space, domid_t foreign_domid, @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - /* Foreign mapping is only supported by add_to_physmap_range */ - if ( xatp.space == XENMAPSPACE_gmfn_foreign ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID, - xatp.idx, xatp.gpfn); - - rcu_unlock_domain(d); - - return rc; - } - case XENMEM_add_to_physmap_range: { struct xen_add_to_physmap_range xatpr; --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4520,20 +4520,23 @@ static int handle_iomem_range(unsigned l return 0; } -static int xenmem_add_to_physmap_once( +int xenmem_add_to_physmap_one( struct domain *d, - const struct xen_add_to_physmap *xatp) + uint16_t space, + domid_t foreign_domid, + unsigned long idx, + xen_pfn_t gpfn) { struct page_info *page = NULL; unsigned long gfn = 0; /* gcc ... */ - unsigned long prev_mfn, mfn = 0, gpfn, idx; + unsigned long prev_mfn, mfn = 0, old_gpfn; int rc; p2m_type_t p2mt; - switch ( xatp->space ) + switch ( space ) { case XENMAPSPACE_shared_info: - if ( xatp->idx == 0 ) + if ( idx == 0 ) mfn = virt_to_mfn(d->shared_info); break; case XENMAPSPACE_grant_table: @@ -4542,9 +4545,8 @@ static int xenmem_add_to_physmap_once( if ( d->grant_table->gt_version == 0 ) d->grant_table->gt_version = 1; - idx = xatp->idx; if ( d->grant_table->gt_version == 2 && - (xatp->idx & XENMAPIDX_grant_table_status) ) + (idx & XENMAPIDX_grant_table_status) ) { idx &= ~XENMAPIDX_grant_table_status; if ( idx < nr_status_frames(d->grant_table) ) @@ -4566,9 +4568,9 @@ static int xenmem_add_to_physmap_once( case XENMAPSPACE_gmfn: { p2m_type_t p2mt; - gfn = xatp->idx; - idx = mfn_x(get_gfn_unshare(d, xatp->idx, &p2mt)); + gfn = idx; + idx = mfn_x(get_gfn_unshare(d, idx, &p2mt)); /* If the page is still shared, exit early */ if ( p2m_is_shared(p2mt) ) { @@ -4589,41 +4591,38 @@ static int xenmem_add_to_physmap_once( { if ( page ) put_page(page); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); return -EINVAL; } /* Remove previously mapped page if it was present. */ - prev_mfn = mfn_x(get_gfn(d, xatp->gpfn, &p2mt)); + prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt)); if ( mfn_valid(prev_mfn) ) { if ( is_xen_heap_mfn(prev_mfn) ) /* Xen heap frames are simply unhooked from this phys slot. */ - guest_physmap_remove_page(d, xatp->gpfn, prev_mfn, PAGE_ORDER_4K); + guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); else /* Normal domain memory is freed, to avoid leaking memory. */ - guest_remove_page(d, xatp->gpfn); + guest_remove_page(d, gpfn); } /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ - put_gfn(d, xatp->gpfn); + put_gfn(d, gpfn); /* Unmap from old location, if any. */ - gpfn = get_gpfn_from_mfn(mfn); - ASSERT( gpfn != SHARED_M2P_ENTRY ); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) - ASSERT( gpfn == gfn ); - if ( gpfn != INVALID_M2P_ENTRY ) - guest_physmap_remove_page(d, gpfn, mfn, PAGE_ORDER_4K); + old_gpfn = get_gpfn_from_mfn(mfn); + ASSERT( old_gpfn != SHARED_M2P_ENTRY ); + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) + ASSERT( old_gpfn == gfn ); + if ( old_gpfn != INVALID_M2P_ENTRY ) + guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K); /* Map at new location. */ - rc = guest_physmap_add_page(d, xatp->gpfn, mfn, PAGE_ORDER_4K); + rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */ - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); if ( page ) @@ -4632,90 +4631,12 @@ static int xenmem_add_to_physmap_once( return rc; } -static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) -{ - struct xen_add_to_physmap start_xatp; - int rc = 0; - - if ( xatp->space == XENMAPSPACE_gmfn_range ) - { - if ( need_iommu(d) ) - this_cpu(iommu_dont_flush_iotlb) = 1; - - start_xatp = *xatp; - while ( xatp->size > 0 ) - { - rc = xenmem_add_to_physmap_once(d, xatp); - if ( rc < 0 ) - break; - - xatp->idx++; - xatp->gpfn++; - xatp->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatp->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - break; - } - } - - if ( need_iommu(d) ) - { - this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); - } - - return rc; - } - - return xenmem_add_to_physmap_once(d, xatp); -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { int rc; switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) ) - { - rcu_unlock_domain(d); - return -EPERM; - } - - rc = xenmem_add_to_physmap(d, &xatp); - - rcu_unlock_domain(d); - - if ( xatp.space == XENMAPSPACE_gmfn_range ) - { - if ( rc && __copy_to_guest(arg, &xatp, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - } - - return rc; - } - case XENMEM_set_memory_map: { struct xen_foreign_memory_map fmap; --- a/xen/arch/x86/x86_64/compat/mm.c +++ b/xen/arch/x86/x86_64/compat/mm.c @@ -55,34 +55,6 @@ int compat_arch_memory_op(int op, XEN_GU switch ( op ) { - case XENMEM_add_to_physmap: - { - struct compat_add_to_physmap cmp; - struct xen_add_to_physmap *nat = COMPAT_ARG_XLAT_VIRT_BASE; - - if ( copy_from_guest(&cmp, arg, 1) ) - return -EFAULT; - - XLAT_add_to_physmap(nat, &cmp); - rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); - - if ( !rc || cmp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp, nat); - if ( __copy_to_guest(arg, &cmp, 1) ) - { - if ( rc == __HYPERVISOR_memory_op ) - hypercall_cancel_continuation(); - return -EFAULT; - } - - if ( rc == __HYPERVISOR_memory_op ) - hypercall_xlat_continuation(NULL, 0x2, nat, arg); - - break; - } - case XENMEM_set_memory_map: { struct compat_foreign_memory_map cmp; --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -26,11 +26,13 @@ int compat_memory_op(unsigned int cmd, X XEN_GUEST_HANDLE_PARAM(void) hnd; struct xen_memory_reservation *rsrv; struct xen_memory_exchange *xchg; + struct xen_add_to_physmap *atp; struct xen_remove_from_physmap *xrfp; } nat; union { struct compat_memory_reservation rsrv; struct compat_memory_exchange xchg; + struct compat_add_to_physmap atp; } cmp; set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); @@ -187,6 +189,14 @@ int compat_memory_op(unsigned int cmd, X nat.hnd = compat; break; + case XENMEM_add_to_physmap: + if ( copy_from_guest(&cmp.atp, compat, 1) ) + return -EFAULT; + + XLAT_add_to_physmap(nat.atp, &cmp.atp); + + break; + case XENMEM_remove_from_physmap: { struct compat_remove_from_physmap cmp; @@ -315,6 +325,16 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_remove_from_physmap: break; + case XENMEM_add_to_physmap: + if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) + break; + + XLAT_add_to_physmap(&cmp.atp, nat.atp); + if ( __copy_to_guest(compat, &cmp.atp, 1) ) + rc = -EFAULT; + + break; + default: domain_crash(current->domain); split = 0; --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA return rc; } +static int xenmem_add_to_physmap(struct domain *d, + struct xen_add_to_physmap *xatp) +{ + struct xen_add_to_physmap start_xatp; + int rc = 0; + + if ( xatp->space != XENMAPSPACE_gmfn_range ) + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + this_cpu(iommu_dont_flush_iotlb) = 1; +#endif + + start_xatp = *xatp; + while ( xatp->size > 0 ) + { + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + if ( rc < 0 ) + break; + + xatp->idx++; + xatp->gpfn++; + xatp->size--; + + /* Check for continuation if it's not the last iteration. */ + if ( xatp->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + break; + } + } + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + { + this_cpu(iommu_dont_flush_iotlb) = 0; + iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + } +#endif + + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -673,6 +720,40 @@ long do_memory_op(unsigned long cmd, XEN break; + case XENMEM_add_to_physmap: + { + struct xen_add_to_physmap xatp; + + if ( copy_from_guest(&xatp, arg, 1) ) + return -EFAULT; + + d = rcu_lock_domain_by_any_id(xatp.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap(d, &xatp); + + rcu_unlock_domain(d); + + if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatp, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -356,6 +356,10 @@ static inline unsigned int get_order_fro void scrub_one_page(struct page_info *); +int xenmem_add_to_physmap_one(struct domain *d, uint16_t space, + domid_t foreign_domid, + unsigned long idx, xen_pfn_t gpfn); + /* Returns 1 on success, 0 on error, negative if the ring * for event propagation is full in the presence of paging */ int guest_remove_page(struct domain *d, unsigned long gmfn); [-- Attachment #2: xatp-common.patch --] [-- Type: text/plain, Size: 14334 bytes --] move XENMEM_add_to_physmap handling framework to common code There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being supported through XENMEM_add_to_physmap_range is being dropped as being pointless. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues share_xen_page_with_guest(page, dom_xen, readonly); } -static int xenmem_add_to_physmap_one( +int xenmem_add_to_physmap_one( struct domain *d, uint16_t space, domid_t foreign_domid, @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - /* Foreign mapping is only supported by add_to_physmap_range */ - if ( xatp.space == XENMAPSPACE_gmfn_foreign ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID, - xatp.idx, xatp.gpfn); - - rcu_unlock_domain(d); - - return rc; - } - case XENMEM_add_to_physmap_range: { struct xen_add_to_physmap_range xatpr; --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4520,20 +4520,23 @@ static int handle_iomem_range(unsigned l return 0; } -static int xenmem_add_to_physmap_once( +int xenmem_add_to_physmap_one( struct domain *d, - const struct xen_add_to_physmap *xatp) + uint16_t space, + domid_t foreign_domid, + unsigned long idx, + xen_pfn_t gpfn) { struct page_info *page = NULL; unsigned long gfn = 0; /* gcc ... */ - unsigned long prev_mfn, mfn = 0, gpfn, idx; + unsigned long prev_mfn, mfn = 0, old_gpfn; int rc; p2m_type_t p2mt; - switch ( xatp->space ) + switch ( space ) { case XENMAPSPACE_shared_info: - if ( xatp->idx == 0 ) + if ( idx == 0 ) mfn = virt_to_mfn(d->shared_info); break; case XENMAPSPACE_grant_table: @@ -4542,9 +4545,8 @@ static int xenmem_add_to_physmap_once( if ( d->grant_table->gt_version == 0 ) d->grant_table->gt_version = 1; - idx = xatp->idx; if ( d->grant_table->gt_version == 2 && - (xatp->idx & XENMAPIDX_grant_table_status) ) + (idx & XENMAPIDX_grant_table_status) ) { idx &= ~XENMAPIDX_grant_table_status; if ( idx < nr_status_frames(d->grant_table) ) @@ -4566,9 +4568,9 @@ static int xenmem_add_to_physmap_once( case XENMAPSPACE_gmfn: { p2m_type_t p2mt; - gfn = xatp->idx; - idx = mfn_x(get_gfn_unshare(d, xatp->idx, &p2mt)); + gfn = idx; + idx = mfn_x(get_gfn_unshare(d, idx, &p2mt)); /* If the page is still shared, exit early */ if ( p2m_is_shared(p2mt) ) { @@ -4589,41 +4591,38 @@ static int xenmem_add_to_physmap_once( { if ( page ) put_page(page); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); return -EINVAL; } /* Remove previously mapped page if it was present. */ - prev_mfn = mfn_x(get_gfn(d, xatp->gpfn, &p2mt)); + prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt)); if ( mfn_valid(prev_mfn) ) { if ( is_xen_heap_mfn(prev_mfn) ) /* Xen heap frames are simply unhooked from this phys slot. */ - guest_physmap_remove_page(d, xatp->gpfn, prev_mfn, PAGE_ORDER_4K); + guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); else /* Normal domain memory is freed, to avoid leaking memory. */ - guest_remove_page(d, xatp->gpfn); + guest_remove_page(d, gpfn); } /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ - put_gfn(d, xatp->gpfn); + put_gfn(d, gpfn); /* Unmap from old location, if any. */ - gpfn = get_gpfn_from_mfn(mfn); - ASSERT( gpfn != SHARED_M2P_ENTRY ); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) - ASSERT( gpfn == gfn ); - if ( gpfn != INVALID_M2P_ENTRY ) - guest_physmap_remove_page(d, gpfn, mfn, PAGE_ORDER_4K); + old_gpfn = get_gpfn_from_mfn(mfn); + ASSERT( old_gpfn != SHARED_M2P_ENTRY ); + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) + ASSERT( old_gpfn == gfn ); + if ( old_gpfn != INVALID_M2P_ENTRY ) + guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K); /* Map at new location. */ - rc = guest_physmap_add_page(d, xatp->gpfn, mfn, PAGE_ORDER_4K); + rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */ - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); if ( page ) @@ -4632,90 +4631,12 @@ static int xenmem_add_to_physmap_once( return rc; } -static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) -{ - struct xen_add_to_physmap start_xatp; - int rc = 0; - - if ( xatp->space == XENMAPSPACE_gmfn_range ) - { - if ( need_iommu(d) ) - this_cpu(iommu_dont_flush_iotlb) = 1; - - start_xatp = *xatp; - while ( xatp->size > 0 ) - { - rc = xenmem_add_to_physmap_once(d, xatp); - if ( rc < 0 ) - break; - - xatp->idx++; - xatp->gpfn++; - xatp->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatp->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - break; - } - } - - if ( need_iommu(d) ) - { - this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); - } - - return rc; - } - - return xenmem_add_to_physmap_once(d, xatp); -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { int rc; switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) ) - { - rcu_unlock_domain(d); - return -EPERM; - } - - rc = xenmem_add_to_physmap(d, &xatp); - - rcu_unlock_domain(d); - - if ( xatp.space == XENMAPSPACE_gmfn_range ) - { - if ( rc && __copy_to_guest(arg, &xatp, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - } - - return rc; - } - case XENMEM_set_memory_map: { struct xen_foreign_memory_map fmap; --- a/xen/arch/x86/x86_64/compat/mm.c +++ b/xen/arch/x86/x86_64/compat/mm.c @@ -55,34 +55,6 @@ int compat_arch_memory_op(int op, XEN_GU switch ( op ) { - case XENMEM_add_to_physmap: - { - struct compat_add_to_physmap cmp; - struct xen_add_to_physmap *nat = COMPAT_ARG_XLAT_VIRT_BASE; - - if ( copy_from_guest(&cmp, arg, 1) ) - return -EFAULT; - - XLAT_add_to_physmap(nat, &cmp); - rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); - - if ( !rc || cmp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp, nat); - if ( __copy_to_guest(arg, &cmp, 1) ) - { - if ( rc == __HYPERVISOR_memory_op ) - hypercall_cancel_continuation(); - return -EFAULT; - } - - if ( rc == __HYPERVISOR_memory_op ) - hypercall_xlat_continuation(NULL, 0x2, nat, arg); - - break; - } - case XENMEM_set_memory_map: { struct compat_foreign_memory_map cmp; --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -26,11 +26,13 @@ int compat_memory_op(unsigned int cmd, X XEN_GUEST_HANDLE_PARAM(void) hnd; struct xen_memory_reservation *rsrv; struct xen_memory_exchange *xchg; + struct xen_add_to_physmap *atp; struct xen_remove_from_physmap *xrfp; } nat; union { struct compat_memory_reservation rsrv; struct compat_memory_exchange xchg; + struct compat_add_to_physmap atp; } cmp; set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); @@ -187,6 +189,14 @@ int compat_memory_op(unsigned int cmd, X nat.hnd = compat; break; + case XENMEM_add_to_physmap: + if ( copy_from_guest(&cmp.atp, compat, 1) ) + return -EFAULT; + + XLAT_add_to_physmap(nat.atp, &cmp.atp); + + break; + case XENMEM_remove_from_physmap: { struct compat_remove_from_physmap cmp; @@ -315,6 +325,16 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_remove_from_physmap: break; + case XENMEM_add_to_physmap: + if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) + break; + + XLAT_add_to_physmap(&cmp.atp, nat.atp); + if ( __copy_to_guest(compat, &cmp.atp, 1) ) + rc = -EFAULT; + + break; + default: domain_crash(current->domain); split = 0; --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA return rc; } +static int xenmem_add_to_physmap(struct domain *d, + struct xen_add_to_physmap *xatp) +{ + struct xen_add_to_physmap start_xatp; + int rc = 0; + + if ( xatp->space != XENMAPSPACE_gmfn_range ) + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + this_cpu(iommu_dont_flush_iotlb) = 1; +#endif + + start_xatp = *xatp; + while ( xatp->size > 0 ) + { + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + if ( rc < 0 ) + break; + + xatp->idx++; + xatp->gpfn++; + xatp->size--; + + /* Check for continuation if it's not the last iteration. */ + if ( xatp->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + break; + } + } + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + { + this_cpu(iommu_dont_flush_iotlb) = 0; + iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + } +#endif + + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -673,6 +720,40 @@ long do_memory_op(unsigned long cmd, XEN break; + case XENMEM_add_to_physmap: + { + struct xen_add_to_physmap xatp; + + if ( copy_from_guest(&xatp, arg, 1) ) + return -EFAULT; + + d = rcu_lock_domain_by_any_id(xatp.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap(d, &xatp); + + rcu_unlock_domain(d); + + if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatp, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -356,6 +356,10 @@ static inline unsigned int get_order_fro void scrub_one_page(struct page_info *); +int xenmem_add_to_physmap_one(struct domain *d, uint16_t space, + domid_t foreign_domid, + unsigned long idx, xen_pfn_t gpfn); + /* Returns 1 on success, 0 on error, negative if the ring * for event propagation is full in the presence of paging */ int guest_remove_page(struct domain *d, unsigned long gmfn); [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 2/4] fix XENMEM_add_to_physmap preemption handling 2013-12-20 8:22 ` [PATCH v2 " Jan Beulich 2013-12-20 8:28 ` [PATCH v2 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich @ 2013-12-20 8:29 ` Jan Beulich 2013-12-20 8:29 ` [PATCH v2 3/4] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich ` (2 subsequent siblings) 4 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 8:29 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 5373 bytes --] Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: drop bogus ASSERT() --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -190,6 +190,9 @@ int compat_memory_op(unsigned int cmd, X break; case XENMEM_add_to_physmap: + BUILD_BUG_ON((typeof(cmp.atp.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + if ( copy_from_guest(&cmp.atp, compat, 1) ) return -EFAULT; @@ -322,17 +325,8 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_current_reservation: case XENMEM_maximum_reservation: case XENMEM_maximum_gpfn: - case XENMEM_remove_from_physmap: - break; - case XENMEM_add_to_physmap: - if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp.atp, nat.atp); - if ( __copy_to_guest(compat, &cmp.atp, 1) ) - rc = -EFAULT; - + case XENMEM_remove_from_physmap: break; default: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -543,22 +543,29 @@ static long memory_exchange(XEN_GUEST_HA } static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) + struct xen_add_to_physmap *xatp, + unsigned int start) { - struct xen_add_to_physmap start_xatp; - int rc = 0; + unsigned int done = 0; + long rc = 0; if ( xatp->space != XENMAPSPACE_gmfn_range ) return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); + if ( xatp->size < start ) + return -EILSEQ; + + xatp->idx += start; + xatp->gpfn += start; + xatp->size -= start; + #ifdef HAS_PASSTHROUGH if ( need_iommu(d) ) this_cpu(iommu_dont_flush_iotlb) = 1; #endif - start_xatp = *xatp; - while ( xatp->size > 0 ) + while ( xatp->size > done ) { rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); @@ -567,12 +577,11 @@ static int xenmem_add_to_physmap(struct xatp->idx++; xatp->gpfn++; - xatp->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatp->size > 0 && hypercall_preempt_check() ) + if ( xatp->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; break; } } @@ -581,8 +590,8 @@ static int xenmem_add_to_physmap(struct if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, xatp->idx - done, done); + iommu_iotlb_flush(d, xatp->gpfn - done, done); } #endif @@ -594,10 +603,10 @@ long do_memory_op(unsigned long cmd, XEN struct domain *d; long rc; unsigned int address_bits; - unsigned long start_extent; struct xen_memory_reservation reservation; struct memop_args args; domid_t domid; + unsigned long start_extent = cmd >> MEMOP_EXTENT_SHIFT; int op = cmd & MEMOP_CMD_MASK; switch ( op ) @@ -605,8 +614,6 @@ long do_memory_op(unsigned long cmd, XEN case XENMEM_increase_reservation: case XENMEM_decrease_reservation: case XENMEM_populate_physmap: - start_extent = cmd >> MEMOP_EXTENT_SHIFT; - if ( copy_from_guest(&reservation, arg, 1) ) return start_extent; @@ -724,6 +731,12 @@ long do_memory_op(unsigned long cmd, XEN { struct xen_add_to_physmap xatp; + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for faked input. */ + if ( start_extent != (typeof(xatp.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; @@ -738,18 +751,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap(d, &xatp); + rc = xenmem_add_to_physmap(d, &xatp, start_extent); rcu_unlock_domain(d); - if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatp, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( xatp.space == XENMAPSPACE_gmfn_range && rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #2: xatp-preemption.patch --] [-- Type: text/plain, Size: 5418 bytes --] fix XENMEM_add_to_physmap preemption handling Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: drop bogus ASSERT() --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -190,6 +190,9 @@ int compat_memory_op(unsigned int cmd, X break; case XENMEM_add_to_physmap: + BUILD_BUG_ON((typeof(cmp.atp.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + if ( copy_from_guest(&cmp.atp, compat, 1) ) return -EFAULT; @@ -322,17 +325,8 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_current_reservation: case XENMEM_maximum_reservation: case XENMEM_maximum_gpfn: - case XENMEM_remove_from_physmap: - break; - case XENMEM_add_to_physmap: - if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp.atp, nat.atp); - if ( __copy_to_guest(compat, &cmp.atp, 1) ) - rc = -EFAULT; - + case XENMEM_remove_from_physmap: break; default: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -543,22 +543,29 @@ static long memory_exchange(XEN_GUEST_HA } static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) + struct xen_add_to_physmap *xatp, + unsigned int start) { - struct xen_add_to_physmap start_xatp; - int rc = 0; + unsigned int done = 0; + long rc = 0; if ( xatp->space != XENMAPSPACE_gmfn_range ) return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); + if ( xatp->size < start ) + return -EILSEQ; + + xatp->idx += start; + xatp->gpfn += start; + xatp->size -= start; + #ifdef HAS_PASSTHROUGH if ( need_iommu(d) ) this_cpu(iommu_dont_flush_iotlb) = 1; #endif - start_xatp = *xatp; - while ( xatp->size > 0 ) + while ( xatp->size > done ) { rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); @@ -567,12 +577,11 @@ static int xenmem_add_to_physmap(struct xatp->idx++; xatp->gpfn++; - xatp->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatp->size > 0 && hypercall_preempt_check() ) + if ( xatp->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; break; } } @@ -581,8 +590,8 @@ static int xenmem_add_to_physmap(struct if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, xatp->idx - done, done); + iommu_iotlb_flush(d, xatp->gpfn - done, done); } #endif @@ -594,10 +603,10 @@ long do_memory_op(unsigned long cmd, XEN struct domain *d; long rc; unsigned int address_bits; - unsigned long start_extent; struct xen_memory_reservation reservation; struct memop_args args; domid_t domid; + unsigned long start_extent = cmd >> MEMOP_EXTENT_SHIFT; int op = cmd & MEMOP_CMD_MASK; switch ( op ) @@ -605,8 +614,6 @@ long do_memory_op(unsigned long cmd, XEN case XENMEM_increase_reservation: case XENMEM_decrease_reservation: case XENMEM_populate_physmap: - start_extent = cmd >> MEMOP_EXTENT_SHIFT; - if ( copy_from_guest(&reservation, arg, 1) ) return start_extent; @@ -724,6 +731,12 @@ long do_memory_op(unsigned long cmd, XEN { struct xen_add_to_physmap xatp; + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for faked input. */ + if ( start_extent != (typeof(xatp.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; @@ -738,18 +751,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap(d, &xatp); + rc = xenmem_add_to_physmap(d, &xatp, start_extent); rcu_unlock_domain(d); - if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatp, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( xatp.space == XENMAPSPACE_gmfn_range && rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 3/4] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-20 8:22 ` [PATCH v2 " Jan Beulich 2013-12-20 8:28 ` [PATCH v2 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich 2013-12-20 8:29 ` [PATCH v2 2/4] fix XENMEM_add_to_physmap preemption handling Jan Beulich @ 2013-12-20 8:29 ` Jan Beulich 2013-12-20 8:30 ` [PATCH v2 4/4] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich 2013-12-20 10:35 ` [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich 4 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 8:29 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 6303 bytes --] There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). This further eliminates the erroneous bailing from xenmem_add_to_physmap_range() if xenmem_add_to_physmap_one() fails. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: Restore restriction of XENMAPSPACE_gmfn_range only being supported through XENMEM_add_to_physmap. --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1057,103 +1057,10 @@ int xenmem_add_to_physmap_one( return rc; } -static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) -{ - int rc; - - while ( xatpr->size > 0 ) - { - xen_ulong_t idx; - xen_pfn_t gpfn; - - if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - rc = xenmem_add_to_physmap_one(d, xatpr->space, - xatpr->foreign_domid, - idx, gpfn); - - if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( rc < 0 ) - goto out; - - guest_handle_add_offset(xatpr->idxs, 1); - guest_handle_add_offset(xatpr->gpfns, 1); - guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - goto out; - } - } - - rc = 0; - -out: - return rc; - -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { - int rc; - switch ( op ) { - case XENMEM_add_to_physmap_range: - { - struct xen_add_to_physmap_range xatpr; - struct domain *d; - - if ( copy_from_guest(&xatpr, arg, 1) ) - return -EFAULT; - - /* This mapspace is redundant for this hypercall */ - if ( xatpr.space == XENMAPSPACE_gmfn_range ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatpr.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_range(d, &xatpr); - - rcu_unlock_domain(d); - - if ( rc && copy_to_guest(arg, &xatpr, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - - return rc; - } /* XXX: memsharing not working yet */ case XENMEM_get_sharing_shared_pages: case XENMEM_get_sharing_freed_pages: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -595,6 +595,57 @@ static int xenmem_add_to_physmap(struct return rc; } +static int xenmem_add_to_physmap_range(struct domain *d, + struct xen_add_to_physmap_range *xatpr) +{ + int rc; + + while ( xatpr->size > 0 ) + { + xen_ulong_t idx; + xen_pfn_t gpfn; + + if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + rc = xenmem_add_to_physmap_one(d, xatpr->space, + xatpr->foreign_domid, + idx, gpfn); + + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) + { + rc = -EFAULT; + goto out; + } + + guest_handle_add_offset(xatpr->idxs, 1); + guest_handle_add_offset(xatpr->gpfns, 1); + guest_handle_add_offset(xatpr->errs, 1); + xatpr->size--; + + /* Check for continuation if it's not the last iteration. */ + if ( xatpr->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + goto out; + } + } + + rc = 0; + +out: + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -760,6 +811,48 @@ long do_memory_op(unsigned long cmd, XEN return rc; } + case XENMEM_add_to_physmap_range: + { + struct xen_add_to_physmap_range xatpr; + struct domain *d; + + if ( copy_from_guest(&xatpr, arg, 1) || + !guest_handle_okay(xatpr.idxs, xatpr.size) || + !guest_handle_okay(xatpr.gpfns, xatpr.size) || + !guest_handle_okay(xatpr.errs, xatpr.size) ) + return -EFAULT; + + /* This mapspace is unsupported for this hypercall. */ + if ( xatpr.space == XENMAPSPACE_gmfn_range ) + return -EOPNOTSUPP; + + d = rcu_lock_domain_by_any_id(xatpr.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap_range(d, &xatpr); + + rcu_unlock_domain(d); + + if ( rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatpr, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; [-- Attachment #2: xatpr-common.patch --] [-- Type: text/plain, Size: 6369 bytes --] move XENMEM_add_to_physmap_range handling framework to common code There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). This further eliminates the erroneous bailing from xenmem_add_to_physmap_range() if xenmem_add_to_physmap_one() fails. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: Restore restriction of XENMAPSPACE_gmfn_range only being supported through XENMEM_add_to_physmap. --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1057,103 +1057,10 @@ int xenmem_add_to_physmap_one( return rc; } -static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) -{ - int rc; - - while ( xatpr->size > 0 ) - { - xen_ulong_t idx; - xen_pfn_t gpfn; - - if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - rc = xenmem_add_to_physmap_one(d, xatpr->space, - xatpr->foreign_domid, - idx, gpfn); - - if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( rc < 0 ) - goto out; - - guest_handle_add_offset(xatpr->idxs, 1); - guest_handle_add_offset(xatpr->gpfns, 1); - guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - goto out; - } - } - - rc = 0; - -out: - return rc; - -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { - int rc; - switch ( op ) { - case XENMEM_add_to_physmap_range: - { - struct xen_add_to_physmap_range xatpr; - struct domain *d; - - if ( copy_from_guest(&xatpr, arg, 1) ) - return -EFAULT; - - /* This mapspace is redundant for this hypercall */ - if ( xatpr.space == XENMAPSPACE_gmfn_range ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatpr.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_range(d, &xatpr); - - rcu_unlock_domain(d); - - if ( rc && copy_to_guest(arg, &xatpr, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - - return rc; - } /* XXX: memsharing not working yet */ case XENMEM_get_sharing_shared_pages: case XENMEM_get_sharing_freed_pages: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -595,6 +595,57 @@ static int xenmem_add_to_physmap(struct return rc; } +static int xenmem_add_to_physmap_range(struct domain *d, + struct xen_add_to_physmap_range *xatpr) +{ + int rc; + + while ( xatpr->size > 0 ) + { + xen_ulong_t idx; + xen_pfn_t gpfn; + + if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + rc = xenmem_add_to_physmap_one(d, xatpr->space, + xatpr->foreign_domid, + idx, gpfn); + + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) + { + rc = -EFAULT; + goto out; + } + + guest_handle_add_offset(xatpr->idxs, 1); + guest_handle_add_offset(xatpr->gpfns, 1); + guest_handle_add_offset(xatpr->errs, 1); + xatpr->size--; + + /* Check for continuation if it's not the last iteration. */ + if ( xatpr->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + goto out; + } + } + + rc = 0; + +out: + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -760,6 +811,48 @@ long do_memory_op(unsigned long cmd, XEN return rc; } + case XENMEM_add_to_physmap_range: + { + struct xen_add_to_physmap_range xatpr; + struct domain *d; + + if ( copy_from_guest(&xatpr, arg, 1) || + !guest_handle_okay(xatpr.idxs, xatpr.size) || + !guest_handle_okay(xatpr.gpfns, xatpr.size) || + !guest_handle_okay(xatpr.errs, xatpr.size) ) + return -EFAULT; + + /* This mapspace is unsupported for this hypercall. */ + if ( xatpr.space == XENMAPSPACE_gmfn_range ) + return -EOPNOTSUPP; + + d = rcu_lock_domain_by_any_id(xatpr.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap_range(d, &xatpr); + + rcu_unlock_domain(d); + + if ( rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatpr, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 4/4] fix XENMEM_add_to_physmap_range preemption handling 2013-12-20 8:22 ` [PATCH v2 " Jan Beulich ` (2 preceding siblings ...) 2013-12-20 8:29 ` [PATCH v2 3/4] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich @ 2013-12-20 8:30 ` Jan Beulich 2013-12-20 10:35 ` [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich 4 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 8:30 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 3112 bytes --] Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs (the one OUT one really refers to the memory pointed to by that handle rather than the handle itself). Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -596,11 +596,21 @@ static int xenmem_add_to_physmap(struct } static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) + struct xen_add_to_physmap_range *xatpr, + unsigned int start) { + unsigned int done = 0; int rc; - while ( xatpr->size > 0 ) + if ( xatpr->size < start ) + return -EILSEQ; + + guest_handle_add_offset(xatpr->idxs, start); + guest_handle_add_offset(xatpr->gpfns, start); + guest_handle_add_offset(xatpr->errs, start); + xatpr->size -= start; + + while ( xatpr->size > done ) { xen_ulong_t idx; xen_pfn_t gpfn; @@ -630,12 +640,11 @@ static int xenmem_add_to_physmap_range(s guest_handle_add_offset(xatpr->idxs, 1); guest_handle_add_offset(xatpr->gpfns, 1); guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) + if ( xatpr->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; goto out; } } @@ -816,6 +825,13 @@ long do_memory_op(unsigned long cmd, XEN struct xen_add_to_physmap_range xatpr; struct domain *d; + BUILD_BUG_ON((typeof(xatpr.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for faked input. */ + if ( start_extent != (typeof(xatpr.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatpr, arg, 1) || !guest_handle_okay(xatpr.idxs, xatpr.size) || !guest_handle_okay(xatpr.gpfns, xatpr.size) || @@ -837,18 +853,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap_range(d, &xatpr); + rc = xenmem_add_to_physmap_range(d, &xatpr, start_extent); rcu_unlock_domain(d); - if ( rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatpr, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #2: xatpr-preemption.patch --] [-- Type: text/plain, Size: 3161 bytes --] fix XENMEM_add_to_physmap_range preemption handling Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs (the one OUT one really refers to the memory pointed to by that handle rather than the handle itself). Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -596,11 +596,21 @@ static int xenmem_add_to_physmap(struct } static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) + struct xen_add_to_physmap_range *xatpr, + unsigned int start) { + unsigned int done = 0; int rc; - while ( xatpr->size > 0 ) + if ( xatpr->size < start ) + return -EILSEQ; + + guest_handle_add_offset(xatpr->idxs, start); + guest_handle_add_offset(xatpr->gpfns, start); + guest_handle_add_offset(xatpr->errs, start); + xatpr->size -= start; + + while ( xatpr->size > done ) { xen_ulong_t idx; xen_pfn_t gpfn; @@ -630,12 +640,11 @@ static int xenmem_add_to_physmap_range(s guest_handle_add_offset(xatpr->idxs, 1); guest_handle_add_offset(xatpr->gpfns, 1); guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) + if ( xatpr->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; goto out; } } @@ -816,6 +825,13 @@ long do_memory_op(unsigned long cmd, XEN struct xen_add_to_physmap_range xatpr; struct domain *d; + BUILD_BUG_ON((typeof(xatpr.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for faked input. */ + if ( start_extent != (typeof(xatpr.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatpr, arg, 1) || !guest_handle_okay(xatpr.idxs, xatpr.size) || !guest_handle_okay(xatpr.gpfns, xatpr.size) || @@ -837,18 +853,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap_range(d, &xatpr); + rc = xenmem_add_to_physmap_range(d, &xatpr, start_extent); rcu_unlock_domain(d); - if ( rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatpr, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} 2013-12-20 8:22 ` [PATCH v2 " Jan Beulich ` (3 preceding siblings ...) 2013-12-20 8:30 ` [PATCH v2 4/4] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich @ 2013-12-20 10:35 ` Jan Beulich 2013-12-20 10:41 ` [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code Jan Beulich ` (4 more replies) 4 siblings, 5 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 10:35 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini The hypervisor isn't supposed to use the input structures for storing continuation information - only fields explicitly used as hypercall outputs should ever be updated. Obviously this implies an ABI change, but since the previous behavior was not intended to be that way I don't think we should stick to the old behavior. There's one caveat though - with the previous model, the caller could - upon failure - use the updated structure to find out how by progress was made. However, that wasn't intended afaict, largely supported by this information depending on hypervisor internals (i.e. the caller would have to know the order of request processing and the meaning of the respective size fields, which aren't simply saying "this much was processed"). Consequently, as a follow-up we may want to consider making explicit (and straight forward) this progress indication on error. 1: move XENMEM_add_to_physmap handling framework to common code 2: fix XENMEM_add_to_physmap preemption handling 3: move XENMEM_add_to_physmap_range handling framework to common code 4: fix XENMEM_add_to_physmap_range preemption handling 5: rename XENMEM_add_to_physmap_{range => batch} Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v3: Retain restriction of XENMAPSPACE_gmfn_foreign not being possible via XENMEM_add_to_physmap (patch 1). Apart from patch 5 being new, other patches unchanged . v2: Apart from the removal of a bogus ASSERT() as requested by Tim, only a couple minor/cosmetic changes (see individual patches), hence I'm retaining the tags that were already given for v1. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code 2013-12-20 10:35 ` [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich @ 2013-12-20 10:41 ` Jan Beulich 2013-12-20 10:45 ` Ian Campbell 2013-12-20 12:26 ` Andrew Cooper 2013-12-20 10:42 ` [PATCH v3 2/5] fix XENMEM_add_to_physmap preemption handling Jan Beulich ` (3 subsequent siblings) 4 siblings, 2 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 10:41 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 14415 bytes --] There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v3: Restriction of XENMAPSPACE_gmfn_foreign only being possible via XENMEM_add_to_physmap_range no longer being dropped. --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues share_xen_page_with_guest(page, dom_xen, readonly); } -static int xenmem_add_to_physmap_one( +int xenmem_add_to_physmap_one( struct domain *d, uint16_t space, domid_t foreign_domid, @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - /* Foreign mapping is only supported by add_to_physmap_range */ - if ( xatp.space == XENMAPSPACE_gmfn_foreign ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID, - xatp.idx, xatp.gpfn); - - rcu_unlock_domain(d); - - return rc; - } - case XENMEM_add_to_physmap_range: { struct xen_add_to_physmap_range xatpr; --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4520,20 +4520,23 @@ static int handle_iomem_range(unsigned l return 0; } -static int xenmem_add_to_physmap_once( +int xenmem_add_to_physmap_one( struct domain *d, - const struct xen_add_to_physmap *xatp) + uint16_t space, + domid_t foreign_domid, + unsigned long idx, + xen_pfn_t gpfn) { struct page_info *page = NULL; unsigned long gfn = 0; /* gcc ... */ - unsigned long prev_mfn, mfn = 0, gpfn, idx; + unsigned long prev_mfn, mfn = 0, old_gpfn; int rc; p2m_type_t p2mt; - switch ( xatp->space ) + switch ( space ) { case XENMAPSPACE_shared_info: - if ( xatp->idx == 0 ) + if ( idx == 0 ) mfn = virt_to_mfn(d->shared_info); break; case XENMAPSPACE_grant_table: @@ -4542,9 +4545,8 @@ static int xenmem_add_to_physmap_once( if ( d->grant_table->gt_version == 0 ) d->grant_table->gt_version = 1; - idx = xatp->idx; if ( d->grant_table->gt_version == 2 && - (xatp->idx & XENMAPIDX_grant_table_status) ) + (idx & XENMAPIDX_grant_table_status) ) { idx &= ~XENMAPIDX_grant_table_status; if ( idx < nr_status_frames(d->grant_table) ) @@ -4566,9 +4568,9 @@ static int xenmem_add_to_physmap_once( case XENMAPSPACE_gmfn: { p2m_type_t p2mt; - gfn = xatp->idx; - idx = mfn_x(get_gfn_unshare(d, xatp->idx, &p2mt)); + gfn = idx; + idx = mfn_x(get_gfn_unshare(d, idx, &p2mt)); /* If the page is still shared, exit early */ if ( p2m_is_shared(p2mt) ) { @@ -4589,41 +4591,38 @@ static int xenmem_add_to_physmap_once( { if ( page ) put_page(page); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); return -EINVAL; } /* Remove previously mapped page if it was present. */ - prev_mfn = mfn_x(get_gfn(d, xatp->gpfn, &p2mt)); + prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt)); if ( mfn_valid(prev_mfn) ) { if ( is_xen_heap_mfn(prev_mfn) ) /* Xen heap frames are simply unhooked from this phys slot. */ - guest_physmap_remove_page(d, xatp->gpfn, prev_mfn, PAGE_ORDER_4K); + guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); else /* Normal domain memory is freed, to avoid leaking memory. */ - guest_remove_page(d, xatp->gpfn); + guest_remove_page(d, gpfn); } /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ - put_gfn(d, xatp->gpfn); + put_gfn(d, gpfn); /* Unmap from old location, if any. */ - gpfn = get_gpfn_from_mfn(mfn); - ASSERT( gpfn != SHARED_M2P_ENTRY ); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) - ASSERT( gpfn == gfn ); - if ( gpfn != INVALID_M2P_ENTRY ) - guest_physmap_remove_page(d, gpfn, mfn, PAGE_ORDER_4K); + old_gpfn = get_gpfn_from_mfn(mfn); + ASSERT( old_gpfn != SHARED_M2P_ENTRY ); + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) + ASSERT( old_gpfn == gfn ); + if ( old_gpfn != INVALID_M2P_ENTRY ) + guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K); /* Map at new location. */ - rc = guest_physmap_add_page(d, xatp->gpfn, mfn, PAGE_ORDER_4K); + rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */ - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); if ( page ) @@ -4632,90 +4631,12 @@ static int xenmem_add_to_physmap_once( return rc; } -static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) -{ - struct xen_add_to_physmap start_xatp; - int rc = 0; - - if ( xatp->space == XENMAPSPACE_gmfn_range ) - { - if ( need_iommu(d) ) - this_cpu(iommu_dont_flush_iotlb) = 1; - - start_xatp = *xatp; - while ( xatp->size > 0 ) - { - rc = xenmem_add_to_physmap_once(d, xatp); - if ( rc < 0 ) - break; - - xatp->idx++; - xatp->gpfn++; - xatp->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatp->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - break; - } - } - - if ( need_iommu(d) ) - { - this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); - } - - return rc; - } - - return xenmem_add_to_physmap_once(d, xatp); -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { int rc; switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) ) - { - rcu_unlock_domain(d); - return -EPERM; - } - - rc = xenmem_add_to_physmap(d, &xatp); - - rcu_unlock_domain(d); - - if ( xatp.space == XENMAPSPACE_gmfn_range ) - { - if ( rc && __copy_to_guest(arg, &xatp, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - } - - return rc; - } - case XENMEM_set_memory_map: { struct xen_foreign_memory_map fmap; --- a/xen/arch/x86/x86_64/compat/mm.c +++ b/xen/arch/x86/x86_64/compat/mm.c @@ -55,34 +55,6 @@ int compat_arch_memory_op(int op, XEN_GU switch ( op ) { - case XENMEM_add_to_physmap: - { - struct compat_add_to_physmap cmp; - struct xen_add_to_physmap *nat = COMPAT_ARG_XLAT_VIRT_BASE; - - if ( copy_from_guest(&cmp, arg, 1) ) - return -EFAULT; - - XLAT_add_to_physmap(nat, &cmp); - rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); - - if ( !rc || cmp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp, nat); - if ( __copy_to_guest(arg, &cmp, 1) ) - { - if ( rc == __HYPERVISOR_memory_op ) - hypercall_cancel_continuation(); - return -EFAULT; - } - - if ( rc == __HYPERVISOR_memory_op ) - hypercall_xlat_continuation(NULL, 0x2, nat, arg); - - break; - } - case XENMEM_set_memory_map: { struct compat_foreign_memory_map cmp; --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -26,11 +26,13 @@ int compat_memory_op(unsigned int cmd, X XEN_GUEST_HANDLE_PARAM(void) hnd; struct xen_memory_reservation *rsrv; struct xen_memory_exchange *xchg; + struct xen_add_to_physmap *atp; struct xen_remove_from_physmap *xrfp; } nat; union { struct compat_memory_reservation rsrv; struct compat_memory_exchange xchg; + struct compat_add_to_physmap atp; } cmp; set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); @@ -187,6 +189,14 @@ int compat_memory_op(unsigned int cmd, X nat.hnd = compat; break; + case XENMEM_add_to_physmap: + if ( copy_from_guest(&cmp.atp, compat, 1) ) + return -EFAULT; + + XLAT_add_to_physmap(nat.atp, &cmp.atp); + + break; + case XENMEM_remove_from_physmap: { struct compat_remove_from_physmap cmp; @@ -315,6 +325,16 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_remove_from_physmap: break; + case XENMEM_add_to_physmap: + if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) + break; + + XLAT_add_to_physmap(&cmp.atp, nat.atp); + if ( __copy_to_guest(compat, &cmp.atp, 1) ) + rc = -EFAULT; + + break; + default: domain_crash(current->domain); split = 0; --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA return rc; } +static int xenmem_add_to_physmap(struct domain *d, + struct xen_add_to_physmap *xatp) +{ + struct xen_add_to_physmap start_xatp; + int rc = 0; + + if ( xatp->space != XENMAPSPACE_gmfn_range ) + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + this_cpu(iommu_dont_flush_iotlb) = 1; +#endif + + start_xatp = *xatp; + while ( xatp->size > 0 ) + { + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + if ( rc < 0 ) + break; + + xatp->idx++; + xatp->gpfn++; + xatp->size--; + + /* Check for continuation if it's not the last iteration. */ + if ( xatp->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + break; + } + } + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + { + this_cpu(iommu_dont_flush_iotlb) = 0; + iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + } +#endif + + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -673,6 +720,44 @@ long do_memory_op(unsigned long cmd, XEN break; + case XENMEM_add_to_physmap: + { + struct xen_add_to_physmap xatp; + + if ( copy_from_guest(&xatp, arg, 1) ) + return -EFAULT; + + /* Foreign mapping is only possible via add_to_physmap_range. */ + if ( xatp.space == XENMAPSPACE_gmfn_foreign ) + return -ENOSYS; + + d = rcu_lock_domain_by_any_id(xatp.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap(d, &xatp); + + rcu_unlock_domain(d); + + if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatp, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -356,6 +356,10 @@ static inline unsigned int get_order_fro void scrub_one_page(struct page_info *); +int xenmem_add_to_physmap_one(struct domain *d, uint16_t space, + domid_t foreign_domid, + unsigned long idx, xen_pfn_t gpfn); + /* Returns 1 on success, 0 on error, negative if the ring * for event propagation is full in the presence of paging */ int guest_remove_page(struct domain *d, unsigned long gmfn); [-- Attachment #2: xatp-common.patch --] [-- Type: text/plain, Size: 14475 bytes --] move XENMEM_add_to_physmap handling framework to common code There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v3: Restriction of XENMAPSPACE_gmfn_foreign only being possible via XENMEM_add_to_physmap_range no longer being dropped. --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues share_xen_page_with_guest(page, dom_xen, readonly); } -static int xenmem_add_to_physmap_one( +int xenmem_add_to_physmap_one( struct domain *d, uint16_t space, domid_t foreign_domid, @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - /* Foreign mapping is only supported by add_to_physmap_range */ - if ( xatp.space == XENMAPSPACE_gmfn_foreign ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID, - xatp.idx, xatp.gpfn); - - rcu_unlock_domain(d); - - return rc; - } - case XENMEM_add_to_physmap_range: { struct xen_add_to_physmap_range xatpr; --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4520,20 +4520,23 @@ static int handle_iomem_range(unsigned l return 0; } -static int xenmem_add_to_physmap_once( +int xenmem_add_to_physmap_one( struct domain *d, - const struct xen_add_to_physmap *xatp) + uint16_t space, + domid_t foreign_domid, + unsigned long idx, + xen_pfn_t gpfn) { struct page_info *page = NULL; unsigned long gfn = 0; /* gcc ... */ - unsigned long prev_mfn, mfn = 0, gpfn, idx; + unsigned long prev_mfn, mfn = 0, old_gpfn; int rc; p2m_type_t p2mt; - switch ( xatp->space ) + switch ( space ) { case XENMAPSPACE_shared_info: - if ( xatp->idx == 0 ) + if ( idx == 0 ) mfn = virt_to_mfn(d->shared_info); break; case XENMAPSPACE_grant_table: @@ -4542,9 +4545,8 @@ static int xenmem_add_to_physmap_once( if ( d->grant_table->gt_version == 0 ) d->grant_table->gt_version = 1; - idx = xatp->idx; if ( d->grant_table->gt_version == 2 && - (xatp->idx & XENMAPIDX_grant_table_status) ) + (idx & XENMAPIDX_grant_table_status) ) { idx &= ~XENMAPIDX_grant_table_status; if ( idx < nr_status_frames(d->grant_table) ) @@ -4566,9 +4568,9 @@ static int xenmem_add_to_physmap_once( case XENMAPSPACE_gmfn: { p2m_type_t p2mt; - gfn = xatp->idx; - idx = mfn_x(get_gfn_unshare(d, xatp->idx, &p2mt)); + gfn = idx; + idx = mfn_x(get_gfn_unshare(d, idx, &p2mt)); /* If the page is still shared, exit early */ if ( p2m_is_shared(p2mt) ) { @@ -4589,41 +4591,38 @@ static int xenmem_add_to_physmap_once( { if ( page ) put_page(page); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); return -EINVAL; } /* Remove previously mapped page if it was present. */ - prev_mfn = mfn_x(get_gfn(d, xatp->gpfn, &p2mt)); + prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt)); if ( mfn_valid(prev_mfn) ) { if ( is_xen_heap_mfn(prev_mfn) ) /* Xen heap frames are simply unhooked from this phys slot. */ - guest_physmap_remove_page(d, xatp->gpfn, prev_mfn, PAGE_ORDER_4K); + guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); else /* Normal domain memory is freed, to avoid leaking memory. */ - guest_remove_page(d, xatp->gpfn); + guest_remove_page(d, gpfn); } /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ - put_gfn(d, xatp->gpfn); + put_gfn(d, gpfn); /* Unmap from old location, if any. */ - gpfn = get_gpfn_from_mfn(mfn); - ASSERT( gpfn != SHARED_M2P_ENTRY ); - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) - ASSERT( gpfn == gfn ); - if ( gpfn != INVALID_M2P_ENTRY ) - guest_physmap_remove_page(d, gpfn, mfn, PAGE_ORDER_4K); + old_gpfn = get_gpfn_from_mfn(mfn); + ASSERT( old_gpfn != SHARED_M2P_ENTRY ); + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) + ASSERT( old_gpfn == gfn ); + if ( old_gpfn != INVALID_M2P_ENTRY ) + guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K); /* Map at new location. */ - rc = guest_physmap_add_page(d, xatp->gpfn, mfn, PAGE_ORDER_4K); + rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */ - if ( xatp->space == XENMAPSPACE_gmfn || - xatp->space == XENMAPSPACE_gmfn_range ) + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) put_gfn(d, gfn); if ( page ) @@ -4632,90 +4631,12 @@ static int xenmem_add_to_physmap_once( return rc; } -static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) -{ - struct xen_add_to_physmap start_xatp; - int rc = 0; - - if ( xatp->space == XENMAPSPACE_gmfn_range ) - { - if ( need_iommu(d) ) - this_cpu(iommu_dont_flush_iotlb) = 1; - - start_xatp = *xatp; - while ( xatp->size > 0 ) - { - rc = xenmem_add_to_physmap_once(d, xatp); - if ( rc < 0 ) - break; - - xatp->idx++; - xatp->gpfn++; - xatp->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatp->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - break; - } - } - - if ( need_iommu(d) ) - { - this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); - } - - return rc; - } - - return xenmem_add_to_physmap_once(d, xatp); -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { int rc; switch ( op ) { - case XENMEM_add_to_physmap: - { - struct xen_add_to_physmap xatp; - struct domain *d; - - if ( copy_from_guest(&xatp, arg, 1) ) - return -EFAULT; - - d = rcu_lock_domain_by_any_id(xatp.domid); - if ( d == NULL ) - return -ESRCH; - - if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) ) - { - rcu_unlock_domain(d); - return -EPERM; - } - - rc = xenmem_add_to_physmap(d, &xatp); - - rcu_unlock_domain(d); - - if ( xatp.space == XENMAPSPACE_gmfn_range ) - { - if ( rc && __copy_to_guest(arg, &xatp, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - } - - return rc; - } - case XENMEM_set_memory_map: { struct xen_foreign_memory_map fmap; --- a/xen/arch/x86/x86_64/compat/mm.c +++ b/xen/arch/x86/x86_64/compat/mm.c @@ -55,34 +55,6 @@ int compat_arch_memory_op(int op, XEN_GU switch ( op ) { - case XENMEM_add_to_physmap: - { - struct compat_add_to_physmap cmp; - struct xen_add_to_physmap *nat = COMPAT_ARG_XLAT_VIRT_BASE; - - if ( copy_from_guest(&cmp, arg, 1) ) - return -EFAULT; - - XLAT_add_to_physmap(nat, &cmp); - rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); - - if ( !rc || cmp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp, nat); - if ( __copy_to_guest(arg, &cmp, 1) ) - { - if ( rc == __HYPERVISOR_memory_op ) - hypercall_cancel_continuation(); - return -EFAULT; - } - - if ( rc == __HYPERVISOR_memory_op ) - hypercall_xlat_continuation(NULL, 0x2, nat, arg); - - break; - } - case XENMEM_set_memory_map: { struct compat_foreign_memory_map cmp; --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -26,11 +26,13 @@ int compat_memory_op(unsigned int cmd, X XEN_GUEST_HANDLE_PARAM(void) hnd; struct xen_memory_reservation *rsrv; struct xen_memory_exchange *xchg; + struct xen_add_to_physmap *atp; struct xen_remove_from_physmap *xrfp; } nat; union { struct compat_memory_reservation rsrv; struct compat_memory_exchange xchg; + struct compat_add_to_physmap atp; } cmp; set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); @@ -187,6 +189,14 @@ int compat_memory_op(unsigned int cmd, X nat.hnd = compat; break; + case XENMEM_add_to_physmap: + if ( copy_from_guest(&cmp.atp, compat, 1) ) + return -EFAULT; + + XLAT_add_to_physmap(nat.atp, &cmp.atp); + + break; + case XENMEM_remove_from_physmap: { struct compat_remove_from_physmap cmp; @@ -315,6 +325,16 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_remove_from_physmap: break; + case XENMEM_add_to_physmap: + if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) + break; + + XLAT_add_to_physmap(&cmp.atp, nat.atp); + if ( __copy_to_guest(compat, &cmp.atp, 1) ) + rc = -EFAULT; + + break; + default: domain_crash(current->domain); split = 0; --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA return rc; } +static int xenmem_add_to_physmap(struct domain *d, + struct xen_add_to_physmap *xatp) +{ + struct xen_add_to_physmap start_xatp; + int rc = 0; + + if ( xatp->space != XENMAPSPACE_gmfn_range ) + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + this_cpu(iommu_dont_flush_iotlb) = 1; +#endif + + start_xatp = *xatp; + while ( xatp->size > 0 ) + { + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + xatp->idx, xatp->gpfn); + if ( rc < 0 ) + break; + + xatp->idx++; + xatp->gpfn++; + xatp->size--; + + /* Check for continuation if it's not the last iteration. */ + if ( xatp->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + break; + } + } + +#ifdef HAS_PASSTHROUGH + if ( need_iommu(d) ) + { + this_cpu(iommu_dont_flush_iotlb) = 0; + iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + } +#endif + + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -673,6 +720,44 @@ long do_memory_op(unsigned long cmd, XEN break; + case XENMEM_add_to_physmap: + { + struct xen_add_to_physmap xatp; + + if ( copy_from_guest(&xatp, arg, 1) ) + return -EFAULT; + + /* Foreign mapping is only possible via add_to_physmap_range. */ + if ( xatp.space == XENMAPSPACE_gmfn_foreign ) + return -ENOSYS; + + d = rcu_lock_domain_by_any_id(xatp.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap(d, &xatp); + + rcu_unlock_domain(d); + + if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatp, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -356,6 +356,10 @@ static inline unsigned int get_order_fro void scrub_one_page(struct page_info *); +int xenmem_add_to_physmap_one(struct domain *d, uint16_t space, + domid_t foreign_domid, + unsigned long idx, xen_pfn_t gpfn); + /* Returns 1 on success, 0 on error, negative if the ring * for event propagation is full in the presence of paging */ int guest_remove_page(struct domain *d, unsigned long gmfn); [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code 2013-12-20 10:41 ` [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code Jan Beulich @ 2013-12-20 10:45 ` Ian Campbell 2013-12-20 12:26 ` Andrew Cooper 1 sibling, 0 replies; 47+ messages in thread From: Ian Campbell @ 2013-12-20 10:45 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 10:41 +0000, Jan Beulich wrote: > There's really nothing really architecture specific here; the > architecture specific handling is limited to > xenmem_add_to_physmap_one(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Tim Deegan <tim@xen.org> > Acked-by: Keir Fraser <keir@xen.org> Acked-by: Ian Campbell <ian.campbell@citrix.com> (nb: I didn't look at the x86 bits at all...) > --- > v3: Restriction of XENMAPSPACE_gmfn_foreign only being possible via > XENMEM_add_to_physmap_range no longer being dropped. Thanks. Ian. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code 2013-12-20 10:41 ` [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code Jan Beulich 2013-12-20 10:45 ` Ian Campbell @ 2013-12-20 12:26 ` Andrew Cooper 2013-12-20 12:45 ` Jan Beulich 1 sibling, 1 reply; 47+ messages in thread From: Andrew Cooper @ 2013-12-20 12:26 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Keir Fraser, Tim Deegan, Ian Campbell, Stefano Stabellini [-- Attachment #1.1: Type: text/plain, Size: 15181 bytes --] On 20/12/13 10:41, Jan Beulich wrote: > There's really nothing really architecture specific here; the > architecture specific handling is limited to > xenmem_add_to_physmap_one(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Tim Deegan <tim@xen.org> > Acked-by: Keir Fraser <keir@xen.org> > --- > v3: Restriction of XENMAPSPACE_gmfn_foreign only being possible via > XENMEM_add_to_physmap_range no longer being dropped. > > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues > share_xen_page_with_guest(page, dom_xen, readonly); > } > > -static int xenmem_add_to_physmap_one( > +int xenmem_add_to_physmap_one( > struct domain *d, > uint16_t space, > domid_t foreign_domid, > @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA > > switch ( op ) > { > - case XENMEM_add_to_physmap: > - { > - struct xen_add_to_physmap xatp; > - struct domain *d; > - > - if ( copy_from_guest(&xatp, arg, 1) ) > - return -EFAULT; > - > - /* Foreign mapping is only supported by add_to_physmap_range */ > - if ( xatp.space == XENMAPSPACE_gmfn_foreign ) > - return -EINVAL; > - > - d = rcu_lock_domain_by_any_id(xatp.domid); > - if ( d == NULL ) > - return -ESRCH; > - > - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); > - if ( rc ) > - { > - rcu_unlock_domain(d); > - return rc; > - } > - > - rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID, > - xatp.idx, xatp.gpfn); > - > - rcu_unlock_domain(d); > - > - return rc; > - } > - > case XENMEM_add_to_physmap_range: > { > struct xen_add_to_physmap_range xatpr; > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -4520,20 +4520,23 @@ static int handle_iomem_range(unsigned l > return 0; > } > > -static int xenmem_add_to_physmap_once( > +int xenmem_add_to_physmap_one( > struct domain *d, > - const struct xen_add_to_physmap *xatp) > + uint16_t space, > + domid_t foreign_domid, > + unsigned long idx, > + xen_pfn_t gpfn) > { > struct page_info *page = NULL; > unsigned long gfn = 0; /* gcc ... */ > - unsigned long prev_mfn, mfn = 0, gpfn, idx; > + unsigned long prev_mfn, mfn = 0, old_gpfn; > int rc; > p2m_type_t p2mt; > > - switch ( xatp->space ) > + switch ( space ) > { > case XENMAPSPACE_shared_info: > - if ( xatp->idx == 0 ) > + if ( idx == 0 ) > mfn = virt_to_mfn(d->shared_info); > break; > case XENMAPSPACE_grant_table: > @@ -4542,9 +4545,8 @@ static int xenmem_add_to_physmap_once( > if ( d->grant_table->gt_version == 0 ) > d->grant_table->gt_version = 1; > > - idx = xatp->idx; > if ( d->grant_table->gt_version == 2 && > - (xatp->idx & XENMAPIDX_grant_table_status) ) > + (idx & XENMAPIDX_grant_table_status) ) > { > idx &= ~XENMAPIDX_grant_table_status; > if ( idx < nr_status_frames(d->grant_table) ) > @@ -4566,9 +4568,9 @@ static int xenmem_add_to_physmap_once( > case XENMAPSPACE_gmfn: > { > p2m_type_t p2mt; > - gfn = xatp->idx; > > - idx = mfn_x(get_gfn_unshare(d, xatp->idx, &p2mt)); > + gfn = idx; > + idx = mfn_x(get_gfn_unshare(d, idx, &p2mt)); > /* If the page is still shared, exit early */ > if ( p2m_is_shared(p2mt) ) > { > @@ -4589,41 +4591,38 @@ static int xenmem_add_to_physmap_once( > { > if ( page ) > put_page(page); > - if ( xatp->space == XENMAPSPACE_gmfn || > - xatp->space == XENMAPSPACE_gmfn_range ) > + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) > put_gfn(d, gfn); > return -EINVAL; > } > > /* Remove previously mapped page if it was present. */ > - prev_mfn = mfn_x(get_gfn(d, xatp->gpfn, &p2mt)); > + prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt)); > if ( mfn_valid(prev_mfn) ) > { > if ( is_xen_heap_mfn(prev_mfn) ) > /* Xen heap frames are simply unhooked from this phys slot. */ > - guest_physmap_remove_page(d, xatp->gpfn, prev_mfn, PAGE_ORDER_4K); > + guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); > else > /* Normal domain memory is freed, to avoid leaking memory. */ > - guest_remove_page(d, xatp->gpfn); > + guest_remove_page(d, gpfn); > } > /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ > - put_gfn(d, xatp->gpfn); > + put_gfn(d, gpfn); > > /* Unmap from old location, if any. */ > - gpfn = get_gpfn_from_mfn(mfn); > - ASSERT( gpfn != SHARED_M2P_ENTRY ); > - if ( xatp->space == XENMAPSPACE_gmfn || > - xatp->space == XENMAPSPACE_gmfn_range ) > - ASSERT( gpfn == gfn ); > - if ( gpfn != INVALID_M2P_ENTRY ) > - guest_physmap_remove_page(d, gpfn, mfn, PAGE_ORDER_4K); > + old_gpfn = get_gpfn_from_mfn(mfn); > + ASSERT( old_gpfn != SHARED_M2P_ENTRY ); > + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) > + ASSERT( old_gpfn == gfn ); > + if ( old_gpfn != INVALID_M2P_ENTRY ) > + guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K); > > /* Map at new location. */ > - rc = guest_physmap_add_page(d, xatp->gpfn, mfn, PAGE_ORDER_4K); > + rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); > > /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */ > - if ( xatp->space == XENMAPSPACE_gmfn || > - xatp->space == XENMAPSPACE_gmfn_range ) > + if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range ) > put_gfn(d, gfn); > > if ( page ) > @@ -4632,90 +4631,12 @@ static int xenmem_add_to_physmap_once( > return rc; > } > > -static int xenmem_add_to_physmap(struct domain *d, > - struct xen_add_to_physmap *xatp) > -{ > - struct xen_add_to_physmap start_xatp; > - int rc = 0; > - > - if ( xatp->space == XENMAPSPACE_gmfn_range ) > - { > - if ( need_iommu(d) ) > - this_cpu(iommu_dont_flush_iotlb) = 1; > - > - start_xatp = *xatp; > - while ( xatp->size > 0 ) > - { > - rc = xenmem_add_to_physmap_once(d, xatp); > - if ( rc < 0 ) > - break; > - > - xatp->idx++; > - xatp->gpfn++; > - xatp->size--; > - > - /* Check for continuation if it's not the last interation */ > - if ( xatp->size > 0 && hypercall_preempt_check() ) > - { > - rc = -EAGAIN; > - break; > - } > - } > - > - if ( need_iommu(d) ) > - { > - this_cpu(iommu_dont_flush_iotlb) = 0; > - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); > - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); > - } > - > - return rc; > - } > - > - return xenmem_add_to_physmap_once(d, xatp); > -} > - > long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) > { > int rc; > > switch ( op ) > { > - case XENMEM_add_to_physmap: > - { > - struct xen_add_to_physmap xatp; > - struct domain *d; > - > - if ( copy_from_guest(&xatp, arg, 1) ) > - return -EFAULT; > - > - d = rcu_lock_domain_by_any_id(xatp.domid); > - if ( d == NULL ) > - return -ESRCH; > - > - if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) ) > - { > - rcu_unlock_domain(d); > - return -EPERM; > - } > - > - rc = xenmem_add_to_physmap(d, &xatp); > - > - rcu_unlock_domain(d); > - > - if ( xatp.space == XENMAPSPACE_gmfn_range ) > - { > - if ( rc && __copy_to_guest(arg, &xatp, 1) ) > - rc = -EFAULT; > - > - if ( rc == -EAGAIN ) > - rc = hypercall_create_continuation( > - __HYPERVISOR_memory_op, "ih", op, arg); > - } > - > - return rc; > - } > - > case XENMEM_set_memory_map: > { > struct xen_foreign_memory_map fmap; > --- a/xen/arch/x86/x86_64/compat/mm.c > +++ b/xen/arch/x86/x86_64/compat/mm.c > @@ -55,34 +55,6 @@ int compat_arch_memory_op(int op, XEN_GU > > switch ( op ) > { > - case XENMEM_add_to_physmap: > - { > - struct compat_add_to_physmap cmp; > - struct xen_add_to_physmap *nat = COMPAT_ARG_XLAT_VIRT_BASE; > - > - if ( copy_from_guest(&cmp, arg, 1) ) > - return -EFAULT; > - > - XLAT_add_to_physmap(nat, &cmp); > - rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); > - > - if ( !rc || cmp.space != XENMAPSPACE_gmfn_range ) > - break; > - > - XLAT_add_to_physmap(&cmp, nat); > - if ( __copy_to_guest(arg, &cmp, 1) ) > - { > - if ( rc == __HYPERVISOR_memory_op ) > - hypercall_cancel_continuation(); > - return -EFAULT; > - } > - > - if ( rc == __HYPERVISOR_memory_op ) > - hypercall_xlat_continuation(NULL, 0x2, nat, arg); > - > - break; > - } > - > case XENMEM_set_memory_map: > { > struct compat_foreign_memory_map cmp; > --- a/xen/common/compat/memory.c > +++ b/xen/common/compat/memory.c > @@ -26,11 +26,13 @@ int compat_memory_op(unsigned int cmd, X > XEN_GUEST_HANDLE_PARAM(void) hnd; > struct xen_memory_reservation *rsrv; > struct xen_memory_exchange *xchg; > + struct xen_add_to_physmap *atp; > struct xen_remove_from_physmap *xrfp; > } nat; > union { > struct compat_memory_reservation rsrv; > struct compat_memory_exchange xchg; > + struct compat_add_to_physmap atp; > } cmp; > > set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); > @@ -187,6 +189,14 @@ int compat_memory_op(unsigned int cmd, X > nat.hnd = compat; > break; > > + case XENMEM_add_to_physmap: > + if ( copy_from_guest(&cmp.atp, compat, 1) ) > + return -EFAULT; > + > + XLAT_add_to_physmap(nat.atp, &cmp.atp); > + > + break; > + > case XENMEM_remove_from_physmap: > { > struct compat_remove_from_physmap cmp; > @@ -315,6 +325,16 @@ int compat_memory_op(unsigned int cmd, X > case XENMEM_remove_from_physmap: > break; > > + case XENMEM_add_to_physmap: > + if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) > + break; Why is this check here in the compat layer? Surely the -ENOSYS from do_memory_op is sufficient to be included in the !rc check ? > + > + XLAT_add_to_physmap(&cmp.atp, nat.atp); > + if ( __copy_to_guest(compat, &cmp.atp, 1) ) > + rc = -EFAULT; > + > + break; > + > default: > domain_crash(current->domain); > split = 0; > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA > return rc; > } > > +static int xenmem_add_to_physmap(struct domain *d, > + struct xen_add_to_physmap *xatp) > +{ > + struct xen_add_to_physmap start_xatp; > + int rc = 0; > + > + if ( xatp->space != XENMAPSPACE_gmfn_range ) > + return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, > + xatp->idx, xatp->gpfn); > + > +#ifdef HAS_PASSTHROUGH > + if ( need_iommu(d) ) > + this_cpu(iommu_dont_flush_iotlb) = 1; > +#endif > + > + start_xatp = *xatp; > + while ( xatp->size > 0 ) > + { > + rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, > + xatp->idx, xatp->gpfn); > + if ( rc < 0 ) > + break; > + > + xatp->idx++; > + xatp->gpfn++; > + xatp->size--; > + > + /* Check for continuation if it's not the last iteration. */ > + if ( xatp->size > 0 && hypercall_preempt_check() ) > + { > + rc = -EAGAIN; > + break; > + } > + } > + > +#ifdef HAS_PASSTHROUGH > + if ( need_iommu(d) ) > + { > + this_cpu(iommu_dont_flush_iotlb) = 0; > + iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); > + iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); > + } > +#endif > + > + return rc; > +} > + > long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > { > struct domain *d; > @@ -673,6 +720,44 @@ long do_memory_op(unsigned long cmd, XEN > > break; > > + case XENMEM_add_to_physmap: > + { > + struct xen_add_to_physmap xatp; > + > + if ( copy_from_guest(&xatp, arg, 1) ) > + return -EFAULT; > + > + /* Foreign mapping is only possible via add_to_physmap_range. */ > + if ( xatp.space == XENMAPSPACE_gmfn_foreign ) > + return -ENOSYS; > + > + d = rcu_lock_domain_by_any_id(xatp.domid); > + if ( d == NULL ) > + return -ESRCH; > + > + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); > + if ( rc ) > + { > + rcu_unlock_domain(d); > + return rc; > + } > + > + rc = xenmem_add_to_physmap(d, &xatp); > + > + rcu_unlock_domain(d); > + > + if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) > + { > + if ( !__copy_to_guest(arg, &xatp, 1) ) > + rc = hypercall_create_continuation( > + __HYPERVISOR_memory_op, "ih", op, arg); > + else > + rc = -EFAULT; > + } > + > + return rc; > + } > + > case XENMEM_remove_from_physmap: > { > struct xen_remove_from_physmap xrfp; > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -356,6 +356,10 @@ static inline unsigned int get_order_fro > > void scrub_one_page(struct page_info *); > > +int xenmem_add_to_physmap_one(struct domain *d, uint16_t space, > + domid_t foreign_domid, > + unsigned long idx, xen_pfn_t gpfn); > + > /* Returns 1 on success, 0 on error, negative if the ring > * for event propagation is full in the presence of paging */ > int guest_remove_page(struct domain *d, unsigned long gmfn); > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel [-- Attachment #1.2: Type: text/html, Size: 15555 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code 2013-12-20 12:26 ` Andrew Cooper @ 2013-12-20 12:45 ` Jan Beulich 0 siblings, 0 replies; 47+ messages in thread From: Jan Beulich @ 2013-12-20 12:45 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel, Tim Deegan, KeirFraser, Ian Campbell, StefanoStabellini >>> On 20.12.13 at 13:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 20/12/13 10:41, Jan Beulich wrote: >> + case XENMEM_add_to_physmap: >> + if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) >> + break; > > Why is this check here in the compat layer? Surely the -ENOSYS from > do_memory_op is sufficient to be included in the !rc check ? > >> + >> + XLAT_add_to_physmap(&cmp.atp, nat.atp); >> + if ( __copy_to_guest(compat, &cmp.atp, 1) ) >> + rc = -EFAULT; >> + >> + break; You mis-read the meaning of the check: The reverse translation was necessary (it's gone with patch 2 anyway) only when wanting to copy back into the interface structure. Since that code is gone with us no longer doing these invalid modifications, any possible incorrectness here is moot now anyway. Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 2/5] fix XENMEM_add_to_physmap preemption handling 2013-12-20 10:35 ` [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich 2013-12-20 10:41 ` [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code Jan Beulich @ 2013-12-20 10:42 ` Jan Beulich 2013-12-20 10:47 ` Ian Campbell 2013-12-20 10:42 ` [PATCH v3 3/5] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich ` (2 subsequent siblings) 4 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 10:42 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 5386 bytes --] Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: drop bogus ASSERT() --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -190,6 +190,9 @@ int compat_memory_op(unsigned int cmd, X break; case XENMEM_add_to_physmap: + BUILD_BUG_ON((typeof(cmp.atp.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + if ( copy_from_guest(&cmp.atp, compat, 1) ) return -EFAULT; @@ -322,17 +325,8 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_current_reservation: case XENMEM_maximum_reservation: case XENMEM_maximum_gpfn: - case XENMEM_remove_from_physmap: - break; - case XENMEM_add_to_physmap: - if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp.atp, nat.atp); - if ( __copy_to_guest(compat, &cmp.atp, 1) ) - rc = -EFAULT; - + case XENMEM_remove_from_physmap: break; default: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -543,22 +543,29 @@ static long memory_exchange(XEN_GUEST_HA } static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) + struct xen_add_to_physmap *xatp, + unsigned int start) { - struct xen_add_to_physmap start_xatp; - int rc = 0; + unsigned int done = 0; + long rc = 0; if ( xatp->space != XENMAPSPACE_gmfn_range ) return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); + if ( xatp->size < start ) + return -EILSEQ; + + xatp->idx += start; + xatp->gpfn += start; + xatp->size -= start; + #ifdef HAS_PASSTHROUGH if ( need_iommu(d) ) this_cpu(iommu_dont_flush_iotlb) = 1; #endif - start_xatp = *xatp; - while ( xatp->size > 0 ) + while ( xatp->size > done ) { rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); @@ -567,12 +574,11 @@ static int xenmem_add_to_physmap(struct xatp->idx++; xatp->gpfn++; - xatp->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatp->size > 0 && hypercall_preempt_check() ) + if ( xatp->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; break; } } @@ -581,8 +587,8 @@ static int xenmem_add_to_physmap(struct if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, xatp->idx - done, done); + iommu_iotlb_flush(d, xatp->gpfn - done, done); } #endif @@ -594,10 +600,10 @@ long do_memory_op(unsigned long cmd, XEN struct domain *d; long rc; unsigned int address_bits; - unsigned long start_extent; struct xen_memory_reservation reservation; struct memop_args args; domid_t domid; + unsigned long start_extent = cmd >> MEMOP_EXTENT_SHIFT; int op = cmd & MEMOP_CMD_MASK; switch ( op ) @@ -605,8 +611,6 @@ long do_memory_op(unsigned long cmd, XEN case XENMEM_increase_reservation: case XENMEM_decrease_reservation: case XENMEM_populate_physmap: - start_extent = cmd >> MEMOP_EXTENT_SHIFT; - if ( copy_from_guest(&reservation, arg, 1) ) return start_extent; @@ -724,6 +728,12 @@ long do_memory_op(unsigned long cmd, XEN { struct xen_add_to_physmap xatp; + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for malicious or buggy input. */ + if ( start_extent != (typeof(xatp.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; @@ -742,18 +752,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap(d, &xatp); + rc = xenmem_add_to_physmap(d, &xatp, start_extent); rcu_unlock_domain(d); - if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatp, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( xatp.space == XENMAPSPACE_gmfn_range && rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #2: xatp-preemption.patch --] [-- Type: text/plain, Size: 5431 bytes --] fix XENMEM_add_to_physmap preemption handling Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: drop bogus ASSERT() --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -190,6 +190,9 @@ int compat_memory_op(unsigned int cmd, X break; case XENMEM_add_to_physmap: + BUILD_BUG_ON((typeof(cmp.atp.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + if ( copy_from_guest(&cmp.atp, compat, 1) ) return -EFAULT; @@ -322,17 +325,8 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_current_reservation: case XENMEM_maximum_reservation: case XENMEM_maximum_gpfn: - case XENMEM_remove_from_physmap: - break; - case XENMEM_add_to_physmap: - if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp.atp, nat.atp); - if ( __copy_to_guest(compat, &cmp.atp, 1) ) - rc = -EFAULT; - + case XENMEM_remove_from_physmap: break; default: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -543,22 +543,29 @@ static long memory_exchange(XEN_GUEST_HA } static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) + struct xen_add_to_physmap *xatp, + unsigned int start) { - struct xen_add_to_physmap start_xatp; - int rc = 0; + unsigned int done = 0; + long rc = 0; if ( xatp->space != XENMAPSPACE_gmfn_range ) return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); + if ( xatp->size < start ) + return -EILSEQ; + + xatp->idx += start; + xatp->gpfn += start; + xatp->size -= start; + #ifdef HAS_PASSTHROUGH if ( need_iommu(d) ) this_cpu(iommu_dont_flush_iotlb) = 1; #endif - start_xatp = *xatp; - while ( xatp->size > 0 ) + while ( xatp->size > done ) { rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); @@ -567,12 +574,11 @@ static int xenmem_add_to_physmap(struct xatp->idx++; xatp->gpfn++; - xatp->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatp->size > 0 && hypercall_preempt_check() ) + if ( xatp->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; break; } } @@ -581,8 +587,8 @@ static int xenmem_add_to_physmap(struct if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, xatp->idx - done, done); + iommu_iotlb_flush(d, xatp->gpfn - done, done); } #endif @@ -594,10 +600,10 @@ long do_memory_op(unsigned long cmd, XEN struct domain *d; long rc; unsigned int address_bits; - unsigned long start_extent; struct xen_memory_reservation reservation; struct memop_args args; domid_t domid; + unsigned long start_extent = cmd >> MEMOP_EXTENT_SHIFT; int op = cmd & MEMOP_CMD_MASK; switch ( op ) @@ -605,8 +611,6 @@ long do_memory_op(unsigned long cmd, XEN case XENMEM_increase_reservation: case XENMEM_decrease_reservation: case XENMEM_populate_physmap: - start_extent = cmd >> MEMOP_EXTENT_SHIFT; - if ( copy_from_guest(&reservation, arg, 1) ) return start_extent; @@ -724,6 +728,12 @@ long do_memory_op(unsigned long cmd, XEN { struct xen_add_to_physmap xatp; + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for malicious or buggy input. */ + if ( start_extent != (typeof(xatp.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; @@ -742,18 +752,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap(d, &xatp); + rc = xenmem_add_to_physmap(d, &xatp, start_extent); rcu_unlock_domain(d); - if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatp, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( xatp.space == XENMAPSPACE_gmfn_range && rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 2/5] fix XENMEM_add_to_physmap preemption handling 2013-12-20 10:42 ` [PATCH v3 2/5] fix XENMEM_add_to_physmap preemption handling Jan Beulich @ 2013-12-20 10:47 ` Ian Campbell 0 siblings, 0 replies; 47+ messages in thread From: Ian Campbell @ 2013-12-20 10:47 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 10:42 +0000, Jan Beulich wrote: > Just like for all other hypercalls we shouldn't be modifying the input > structure - all of the fields are, even if not explicitly documented, > just inputs. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Tim Deegan <tim@xen.org> > Acked-by: Keir Fraser <keir@xen.org> Acked-by: Ian Campbell <ian.campbell@citrix.com> ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 3/5] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-20 10:35 ` [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich 2013-12-20 10:41 ` [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code Jan Beulich 2013-12-20 10:42 ` [PATCH v3 2/5] fix XENMEM_add_to_physmap preemption handling Jan Beulich @ 2013-12-20 10:42 ` Jan Beulich 2013-12-20 10:51 ` Ian Campbell 2013-12-20 10:43 ` [PATCH v3 4/5] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich 2013-12-20 10:44 ` [PATCH v3 5/5] rename XENMEM_add_to_physmap_{range => batch} Jan Beulich 4 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 10:42 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 6303 bytes --] There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). This further eliminates the erroneous bailing from xenmem_add_to_physmap_range() if xenmem_add_to_physmap_one() fails. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: Restore restriction of XENMAPSPACE_gmfn_range only being supported through XENMEM_add_to_physmap. --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1057,103 +1057,10 @@ int xenmem_add_to_physmap_one( return rc; } -static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) -{ - int rc; - - while ( xatpr->size > 0 ) - { - xen_ulong_t idx; - xen_pfn_t gpfn; - - if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - rc = xenmem_add_to_physmap_one(d, xatpr->space, - xatpr->foreign_domid, - idx, gpfn); - - if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( rc < 0 ) - goto out; - - guest_handle_add_offset(xatpr->idxs, 1); - guest_handle_add_offset(xatpr->gpfns, 1); - guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - goto out; - } - } - - rc = 0; - -out: - return rc; - -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { - int rc; - switch ( op ) { - case XENMEM_add_to_physmap_range: - { - struct xen_add_to_physmap_range xatpr; - struct domain *d; - - if ( copy_from_guest(&xatpr, arg, 1) ) - return -EFAULT; - - /* This mapspace is redundant for this hypercall */ - if ( xatpr.space == XENMAPSPACE_gmfn_range ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatpr.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_range(d, &xatpr); - - rcu_unlock_domain(d); - - if ( rc && copy_to_guest(arg, &xatpr, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - - return rc; - } /* XXX: memsharing not working yet */ case XENMEM_get_sharing_shared_pages: case XENMEM_get_sharing_freed_pages: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -595,6 +595,57 @@ static int xenmem_add_to_physmap(struct return rc; } +static int xenmem_add_to_physmap_range(struct domain *d, + struct xen_add_to_physmap_range *xatpr) +{ + int rc; + + while ( xatpr->size > 0 ) + { + xen_ulong_t idx; + xen_pfn_t gpfn; + + if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + rc = xenmem_add_to_physmap_one(d, xatpr->space, + xatpr->foreign_domid, + idx, gpfn); + + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) + { + rc = -EFAULT; + goto out; + } + + guest_handle_add_offset(xatpr->idxs, 1); + guest_handle_add_offset(xatpr->gpfns, 1); + guest_handle_add_offset(xatpr->errs, 1); + xatpr->size--; + + /* Check for continuation if it's not the last iteration. */ + if ( xatpr->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + goto out; + } + } + + rc = 0; + +out: + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -764,6 +815,48 @@ long do_memory_op(unsigned long cmd, XEN return rc; } + case XENMEM_add_to_physmap_range: + { + struct xen_add_to_physmap_range xatpr; + struct domain *d; + + if ( copy_from_guest(&xatpr, arg, 1) || + !guest_handle_okay(xatpr.idxs, xatpr.size) || + !guest_handle_okay(xatpr.gpfns, xatpr.size) || + !guest_handle_okay(xatpr.errs, xatpr.size) ) + return -EFAULT; + + /* This mapspace is unsupported for this hypercall. */ + if ( xatpr.space == XENMAPSPACE_gmfn_range ) + return -EOPNOTSUPP; + + d = rcu_lock_domain_by_any_id(xatpr.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap_range(d, &xatpr); + + rcu_unlock_domain(d); + + if ( rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatpr, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; [-- Attachment #2: xatpr-common.patch --] [-- Type: text/plain, Size: 6369 bytes --] move XENMEM_add_to_physmap_range handling framework to common code There's really nothing really architecture specific here; the architecture specific handling is limited to xenmem_add_to_physmap_one(). This further eliminates the erroneous bailing from xenmem_add_to_physmap_range() if xenmem_add_to_physmap_one() fails. Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- v2: Restore restriction of XENMAPSPACE_gmfn_range only being supported through XENMEM_add_to_physmap. --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1057,103 +1057,10 @@ int xenmem_add_to_physmap_one( return rc; } -static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) -{ - int rc; - - while ( xatpr->size > 0 ) - { - xen_ulong_t idx; - xen_pfn_t gpfn; - - if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) - { - rc = -EFAULT; - goto out; - } - - rc = xenmem_add_to_physmap_one(d, xatpr->space, - xatpr->foreign_domid, - idx, gpfn); - - if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) - { - rc = -EFAULT; - goto out; - } - - if ( rc < 0 ) - goto out; - - guest_handle_add_offset(xatpr->idxs, 1); - guest_handle_add_offset(xatpr->gpfns, 1); - guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; - - /* Check for continuation if it's not the last interation */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) - { - rc = -EAGAIN; - goto out; - } - } - - rc = 0; - -out: - return rc; - -} - long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) { - int rc; - switch ( op ) { - case XENMEM_add_to_physmap_range: - { - struct xen_add_to_physmap_range xatpr; - struct domain *d; - - if ( copy_from_guest(&xatpr, arg, 1) ) - return -EFAULT; - - /* This mapspace is redundant for this hypercall */ - if ( xatpr.space == XENMAPSPACE_gmfn_range ) - return -EINVAL; - - d = rcu_lock_domain_by_any_id(xatpr.domid); - if ( d == NULL ) - return -ESRCH; - - rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); - if ( rc ) - { - rcu_unlock_domain(d); - return rc; - } - - rc = xenmem_add_to_physmap_range(d, &xatpr); - - rcu_unlock_domain(d); - - if ( rc && copy_to_guest(arg, &xatpr, 1) ) - rc = -EFAULT; - - if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - - return rc; - } /* XXX: memsharing not working yet */ case XENMEM_get_sharing_shared_pages: case XENMEM_get_sharing_freed_pages: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -595,6 +595,57 @@ static int xenmem_add_to_physmap(struct return rc; } +static int xenmem_add_to_physmap_range(struct domain *d, + struct xen_add_to_physmap_range *xatpr) +{ + int rc; + + while ( xatpr->size > 0 ) + { + xen_ulong_t idx; + xen_pfn_t gpfn; + + if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) + { + rc = -EFAULT; + goto out; + } + + rc = xenmem_add_to_physmap_one(d, xatpr->space, + xatpr->foreign_domid, + idx, gpfn); + + if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) + { + rc = -EFAULT; + goto out; + } + + guest_handle_add_offset(xatpr->idxs, 1); + guest_handle_add_offset(xatpr->gpfns, 1); + guest_handle_add_offset(xatpr->errs, 1); + xatpr->size--; + + /* Check for continuation if it's not the last iteration. */ + if ( xatpr->size > 0 && hypercall_preempt_check() ) + { + rc = -EAGAIN; + goto out; + } + } + + rc = 0; + +out: + return rc; +} + long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d; @@ -764,6 +815,48 @@ long do_memory_op(unsigned long cmd, XEN return rc; } + case XENMEM_add_to_physmap_range: + { + struct xen_add_to_physmap_range xatpr; + struct domain *d; + + if ( copy_from_guest(&xatpr, arg, 1) || + !guest_handle_okay(xatpr.idxs, xatpr.size) || + !guest_handle_okay(xatpr.gpfns, xatpr.size) || + !guest_handle_okay(xatpr.errs, xatpr.size) ) + return -EFAULT; + + /* This mapspace is unsupported for this hypercall. */ + if ( xatpr.space == XENMAPSPACE_gmfn_range ) + return -EOPNOTSUPP; + + d = rcu_lock_domain_by_any_id(xatpr.domid); + if ( d == NULL ) + return -ESRCH; + + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); + if ( rc ) + { + rcu_unlock_domain(d); + return rc; + } + + rc = xenmem_add_to_physmap_range(d, &xatpr); + + rcu_unlock_domain(d); + + if ( rc == -EAGAIN ) + { + if ( !__copy_to_guest(arg, &xatpr, 1) ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "ih", op, arg); + else + rc = -EFAULT; + } + + return rc; + } + case XENMEM_remove_from_physmap: { struct xen_remove_from_physmap xrfp; [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 3/5] move XENMEM_add_to_physmap_range handling framework to common code 2013-12-20 10:42 ` [PATCH v3 3/5] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich @ 2013-12-20 10:51 ` Ian Campbell 0 siblings, 0 replies; 47+ messages in thread From: Ian Campbell @ 2013-12-20 10:51 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 10:42 +0000, Jan Beulich wrote: > There's really nothing really architecture specific here; the > architecture specific handling is limited to > xenmem_add_to_physmap_one(). > > This further eliminates the erroneous bailing from > xenmem_add_to_physmap_range() if xenmem_add_to_physmap_one() fails. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Tim Deegan <tim@xen.org> > Acked-by: Keir Fraser <keir@xen.org> Acked-by: Ian Campbell <ian.campbell@citrix.com> ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 4/5] fix XENMEM_add_to_physmap_range preemption handling 2013-12-20 10:35 ` [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich ` (2 preceding siblings ...) 2013-12-20 10:42 ` [PATCH v3 3/5] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich @ 2013-12-20 10:43 ` Jan Beulich 2013-12-20 10:53 ` Ian Campbell 2013-12-20 10:44 ` [PATCH v3 5/5] rename XENMEM_add_to_physmap_{range => batch} Jan Beulich 4 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 10:43 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 3125 bytes --] Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs (the one OUT one really refers to the memory pointed to by that handle rather than the handle itself). Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -596,11 +596,21 @@ static int xenmem_add_to_physmap(struct } static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) + struct xen_add_to_physmap_range *xatpr, + unsigned int start) { + unsigned int done = 0; int rc; - while ( xatpr->size > 0 ) + if ( xatpr->size < start ) + return -EILSEQ; + + guest_handle_add_offset(xatpr->idxs, start); + guest_handle_add_offset(xatpr->gpfns, start); + guest_handle_add_offset(xatpr->errs, start); + xatpr->size -= start; + + while ( xatpr->size > done ) { xen_ulong_t idx; xen_pfn_t gpfn; @@ -630,12 +640,11 @@ static int xenmem_add_to_physmap_range(s guest_handle_add_offset(xatpr->idxs, 1); guest_handle_add_offset(xatpr->gpfns, 1); guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) + if ( xatpr->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; goto out; } } @@ -820,6 +829,13 @@ long do_memory_op(unsigned long cmd, XEN struct xen_add_to_physmap_range xatpr; struct domain *d; + BUILD_BUG_ON((typeof(xatpr.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for malicious or buggy input. */ + if ( start_extent != (typeof(xatpr.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatpr, arg, 1) || !guest_handle_okay(xatpr.idxs, xatpr.size) || !guest_handle_okay(xatpr.gpfns, xatpr.size) || @@ -841,18 +857,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap_range(d, &xatpr); + rc = xenmem_add_to_physmap_range(d, &xatpr, start_extent); rcu_unlock_domain(d); - if ( rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatpr, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #2: xatpr-preemption.patch --] [-- Type: text/plain, Size: 3174 bytes --] fix XENMEM_add_to_physmap_range preemption handling Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs (the one OUT one really refers to the memory pointed to by that handle rather than the handle itself). Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -596,11 +596,21 @@ static int xenmem_add_to_physmap(struct } static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr) + struct xen_add_to_physmap_range *xatpr, + unsigned int start) { + unsigned int done = 0; int rc; - while ( xatpr->size > 0 ) + if ( xatpr->size < start ) + return -EILSEQ; + + guest_handle_add_offset(xatpr->idxs, start); + guest_handle_add_offset(xatpr->gpfns, start); + guest_handle_add_offset(xatpr->errs, start); + xatpr->size -= start; + + while ( xatpr->size > done ) { xen_ulong_t idx; xen_pfn_t gpfn; @@ -630,12 +640,11 @@ static int xenmem_add_to_physmap_range(s guest_handle_add_offset(xatpr->idxs, 1); guest_handle_add_offset(xatpr->gpfns, 1); guest_handle_add_offset(xatpr->errs, 1); - xatpr->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatpr->size > 0 && hypercall_preempt_check() ) + if ( xatpr->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; goto out; } } @@ -820,6 +829,13 @@ long do_memory_op(unsigned long cmd, XEN struct xen_add_to_physmap_range xatpr; struct domain *d; + BUILD_BUG_ON((typeof(xatpr.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for malicious or buggy input. */ + if ( start_extent != (typeof(xatpr.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatpr, arg, 1) || !guest_handle_okay(xatpr.idxs, xatpr.size) || !guest_handle_okay(xatpr.gpfns, xatpr.size) || @@ -841,18 +857,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap_range(d, &xatpr); + rc = xenmem_add_to_physmap_range(d, &xatpr, start_extent); rcu_unlock_domain(d); - if ( rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatpr, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; } [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 4/5] fix XENMEM_add_to_physmap_range preemption handling 2013-12-20 10:43 ` [PATCH v3 4/5] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich @ 2013-12-20 10:53 ` Ian Campbell 0 siblings, 0 replies; 47+ messages in thread From: Ian Campbell @ 2013-12-20 10:53 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 10:43 +0000, Jan Beulich wrote: > Just like for all other hypercalls we shouldn't be modifying the input > structure - all of the fields are, even if not explicitly documented, > just inputs (the one OUT one really refers to the memory pointed to by > that handle rather than the handle itself). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Tim Deegan <tim@xen.org> > Acked-by: Keir Fraser <keir@xen.org> Acked-by: Ian Campbell <ian.campbell@citrix.com> ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 5/5] rename XENMEM_add_to_physmap_{range => batch} 2013-12-20 10:35 ` [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich ` (3 preceding siblings ...) 2013-12-20 10:43 ` [PATCH v3 4/5] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich @ 2013-12-20 10:44 ` Jan Beulich 2013-12-20 10:55 ` Ian Campbell 4 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 10:44 UTC (permalink / raw) To: xen-devel; +Cc: Tim Deegan, Keir Fraser, Ian Campbell, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 7402 bytes --] The use of "range" here wasn't really correct - there are no ranges involved. As the comment in the public header already correctly said, all this is about is batching of XENMEM_add_to_physmap calls (with the addition of having a way to specify a foreign domain for XENMAPSPACE_gmfn_foreign). Suggested-by: Ian Campbell <Ian.Campbell@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -595,54 +595,54 @@ static int xenmem_add_to_physmap(struct return rc; } -static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr, +static int xenmem_add_to_physmap_batch(struct domain *d, + struct xen_add_to_physmap_batch *xatpb, unsigned int start) { unsigned int done = 0; int rc; - if ( xatpr->size < start ) + if ( xatpb->size < start ) return -EILSEQ; - guest_handle_add_offset(xatpr->idxs, start); - guest_handle_add_offset(xatpr->gpfns, start); - guest_handle_add_offset(xatpr->errs, start); - xatpr->size -= start; + guest_handle_add_offset(xatpb->idxs, start); + guest_handle_add_offset(xatpb->gpfns, start); + guest_handle_add_offset(xatpb->errs, start); + xatpb->size -= start; - while ( xatpr->size > done ) + while ( xatpb->size > done ) { xen_ulong_t idx; xen_pfn_t gpfn; - if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) + if ( unlikely(__copy_from_guest_offset(&idx, xatpb->idxs, 0, 1)) ) { rc = -EFAULT; goto out; } - if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpb->gpfns, 0, 1)) ) { rc = -EFAULT; goto out; } - rc = xenmem_add_to_physmap_one(d, xatpr->space, - xatpr->foreign_domid, + rc = xenmem_add_to_physmap_one(d, xatpb->space, + xatpb->foreign_domid, idx, gpfn); - if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) + if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) ) { rc = -EFAULT; goto out; } - guest_handle_add_offset(xatpr->idxs, 1); - guest_handle_add_offset(xatpr->gpfns, 1); - guest_handle_add_offset(xatpr->errs, 1); + guest_handle_add_offset(xatpb->idxs, 1); + guest_handle_add_offset(xatpb->gpfns, 1); + guest_handle_add_offset(xatpb->errs, 1); /* Check for continuation if it's not the last iteration. */ - if ( xatpr->size > ++done && hypercall_preempt_check() ) + if ( xatpb->size > ++done && hypercall_preempt_check() ) { rc = start + done; goto out; @@ -797,7 +797,7 @@ long do_memory_op(unsigned long cmd, XEN if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; - /* Foreign mapping is only possible via add_to_physmap_range. */ + /* Foreign mapping is only possible via add_to_physmap_batch. */ if ( xatp.space == XENMAPSPACE_gmfn_foreign ) return -ENOSYS; @@ -824,29 +824,29 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - case XENMEM_add_to_physmap_range: + case XENMEM_add_to_physmap_batch: { - struct xen_add_to_physmap_range xatpr; + struct xen_add_to_physmap_batch xatpb; struct domain *d; - BUILD_BUG_ON((typeof(xatpr.size))-1 > + BUILD_BUG_ON((typeof(xatpb.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); /* Check for malicious or buggy input. */ - if ( start_extent != (typeof(xatpr.size))start_extent ) + if ( start_extent != (typeof(xatpb.size))start_extent ) return -EDOM; - if ( copy_from_guest(&xatpr, arg, 1) || - !guest_handle_okay(xatpr.idxs, xatpr.size) || - !guest_handle_okay(xatpr.gpfns, xatpr.size) || - !guest_handle_okay(xatpr.errs, xatpr.size) ) + if ( copy_from_guest(&xatpb, arg, 1) || + !guest_handle_okay(xatpb.idxs, xatpb.size) || + !guest_handle_okay(xatpb.gpfns, xatpb.size) || + !guest_handle_okay(xatpb.errs, xatpb.size) ) return -EFAULT; /* This mapspace is unsupported for this hypercall. */ - if ( xatpr.space == XENMAPSPACE_gmfn_range ) + if ( xatpb.space == XENMAPSPACE_gmfn_range ) return -EOPNOTSUPP; - d = rcu_lock_domain_by_any_id(xatpr.domid); + d = rcu_lock_domain_by_any_id(xatpb.domid); if ( d == NULL ) return -ESRCH; @@ -857,7 +857,7 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap_range(d, &xatpr, start_extent); + rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent); rcu_unlock_domain(d); --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -79,7 +79,7 @@ * * In addition the following arch specific sub-ops: * * XENMEM_add_to_physmap - * * XENMEM_add_to_physmap_range + * * XENMEM_add_to_physmap_batch * * HYPERVISOR_domctl * All generic sub-operations, with the exception of: --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -207,8 +207,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_map #define XENMAPSPACE_gmfn 2 /* GMFN */ #define XENMAPSPACE_gmfn_range 3 /* GMFN range, XENMEM_add_to_physmap only. */ #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom, - * XENMEM_add_to_physmap_range only. - */ + * XENMEM_add_to_physmap_batch only. */ /* ` } */ /* @@ -238,8 +237,8 @@ typedef struct xen_add_to_physmap xen_ad DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_t); /* A batched version of add_to_physmap. */ -#define XENMEM_add_to_physmap_range 23 -struct xen_add_to_physmap_range { +#define XENMEM_add_to_physmap_batch 23 +struct xen_add_to_physmap_batch { /* IN */ /* Which domain to change the mapping for. */ domid_t domid; @@ -260,8 +259,15 @@ struct xen_add_to_physmap_range { /* Per index error code. */ XEN_GUEST_HANDLE(int) errs; }; -typedef struct xen_add_to_physmap_range xen_add_to_physmap_range_t; -DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_range_t); +typedef struct xen_add_to_physmap_batch xen_add_to_physmap_batch_t; +DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_batch_t); + +#if __XEN_INTERFACE_VERSION__ < 0x00040400 +#define XENMEM_add_to_physmap_range XENMEM_add_to_physmap_batch +#define xen_add_to_physmap_range xen_add_to_physmap_batch +typedef struct xen_add_to_physmap_batch xen_add_to_physmap_range_t; +DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_batch_t); +#endif /* * Unmaps the page appearing at a particular GPFN from the specified guest's [-- Attachment #2: xatpr-naming.patch --] [-- Type: text/plain, Size: 7447 bytes --] rename XENMEM_add_to_physmap_{range => batch} The use of "range" here wasn't really correct - there are no ranges involved. As the comment in the public header already correctly said, all this is about is batching of XENMEM_add_to_physmap calls (with the addition of having a way to specify a foreign domain for XENMAPSPACE_gmfn_foreign). Suggested-by: Ian Campbell <Ian.Campbell@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -595,54 +595,54 @@ static int xenmem_add_to_physmap(struct return rc; } -static int xenmem_add_to_physmap_range(struct domain *d, - struct xen_add_to_physmap_range *xatpr, +static int xenmem_add_to_physmap_batch(struct domain *d, + struct xen_add_to_physmap_batch *xatpb, unsigned int start) { unsigned int done = 0; int rc; - if ( xatpr->size < start ) + if ( xatpb->size < start ) return -EILSEQ; - guest_handle_add_offset(xatpr->idxs, start); - guest_handle_add_offset(xatpr->gpfns, start); - guest_handle_add_offset(xatpr->errs, start); - xatpr->size -= start; + guest_handle_add_offset(xatpb->idxs, start); + guest_handle_add_offset(xatpb->gpfns, start); + guest_handle_add_offset(xatpb->errs, start); + xatpb->size -= start; - while ( xatpr->size > done ) + while ( xatpb->size > done ) { xen_ulong_t idx; xen_pfn_t gpfn; - if ( unlikely(__copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) ) + if ( unlikely(__copy_from_guest_offset(&idx, xatpb->idxs, 0, 1)) ) { rc = -EFAULT; goto out; } - if ( unlikely(__copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) ) + if ( unlikely(__copy_from_guest_offset(&gpfn, xatpb->gpfns, 0, 1)) ) { rc = -EFAULT; goto out; } - rc = xenmem_add_to_physmap_one(d, xatpr->space, - xatpr->foreign_domid, + rc = xenmem_add_to_physmap_one(d, xatpb->space, + xatpb->foreign_domid, idx, gpfn); - if ( unlikely(__copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) ) + if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) ) { rc = -EFAULT; goto out; } - guest_handle_add_offset(xatpr->idxs, 1); - guest_handle_add_offset(xatpr->gpfns, 1); - guest_handle_add_offset(xatpr->errs, 1); + guest_handle_add_offset(xatpb->idxs, 1); + guest_handle_add_offset(xatpb->gpfns, 1); + guest_handle_add_offset(xatpb->errs, 1); /* Check for continuation if it's not the last iteration. */ - if ( xatpr->size > ++done && hypercall_preempt_check() ) + if ( xatpb->size > ++done && hypercall_preempt_check() ) { rc = start + done; goto out; @@ -797,7 +797,7 @@ long do_memory_op(unsigned long cmd, XEN if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; - /* Foreign mapping is only possible via add_to_physmap_range. */ + /* Foreign mapping is only possible via add_to_physmap_batch. */ if ( xatp.space == XENMAPSPACE_gmfn_foreign ) return -ENOSYS; @@ -824,29 +824,29 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - case XENMEM_add_to_physmap_range: + case XENMEM_add_to_physmap_batch: { - struct xen_add_to_physmap_range xatpr; + struct xen_add_to_physmap_batch xatpb; struct domain *d; - BUILD_BUG_ON((typeof(xatpr.size))-1 > + BUILD_BUG_ON((typeof(xatpb.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); /* Check for malicious or buggy input. */ - if ( start_extent != (typeof(xatpr.size))start_extent ) + if ( start_extent != (typeof(xatpb.size))start_extent ) return -EDOM; - if ( copy_from_guest(&xatpr, arg, 1) || - !guest_handle_okay(xatpr.idxs, xatpr.size) || - !guest_handle_okay(xatpr.gpfns, xatpr.size) || - !guest_handle_okay(xatpr.errs, xatpr.size) ) + if ( copy_from_guest(&xatpb, arg, 1) || + !guest_handle_okay(xatpb.idxs, xatpb.size) || + !guest_handle_okay(xatpb.gpfns, xatpb.size) || + !guest_handle_okay(xatpb.errs, xatpb.size) ) return -EFAULT; /* This mapspace is unsupported for this hypercall. */ - if ( xatpr.space == XENMAPSPACE_gmfn_range ) + if ( xatpb.space == XENMAPSPACE_gmfn_range ) return -EOPNOTSUPP; - d = rcu_lock_domain_by_any_id(xatpr.domid); + d = rcu_lock_domain_by_any_id(xatpb.domid); if ( d == NULL ) return -ESRCH; @@ -857,7 +857,7 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap_range(d, &xatpr, start_extent); + rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent); rcu_unlock_domain(d); --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -79,7 +79,7 @@ * * In addition the following arch specific sub-ops: * * XENMEM_add_to_physmap - * * XENMEM_add_to_physmap_range + * * XENMEM_add_to_physmap_batch * * HYPERVISOR_domctl * All generic sub-operations, with the exception of: --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -207,8 +207,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_map #define XENMAPSPACE_gmfn 2 /* GMFN */ #define XENMAPSPACE_gmfn_range 3 /* GMFN range, XENMEM_add_to_physmap only. */ #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom, - * XENMEM_add_to_physmap_range only. - */ + * XENMEM_add_to_physmap_batch only. */ /* ` } */ /* @@ -238,8 +237,8 @@ typedef struct xen_add_to_physmap xen_ad DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_t); /* A batched version of add_to_physmap. */ -#define XENMEM_add_to_physmap_range 23 -struct xen_add_to_physmap_range { +#define XENMEM_add_to_physmap_batch 23 +struct xen_add_to_physmap_batch { /* IN */ /* Which domain to change the mapping for. */ domid_t domid; @@ -260,8 +259,15 @@ struct xen_add_to_physmap_range { /* Per index error code. */ XEN_GUEST_HANDLE(int) errs; }; -typedef struct xen_add_to_physmap_range xen_add_to_physmap_range_t; -DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_range_t); +typedef struct xen_add_to_physmap_batch xen_add_to_physmap_batch_t; +DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_batch_t); + +#if __XEN_INTERFACE_VERSION__ < 0x00040400 +#define XENMEM_add_to_physmap_range XENMEM_add_to_physmap_batch +#define xen_add_to_physmap_range xen_add_to_physmap_batch +typedef struct xen_add_to_physmap_batch xen_add_to_physmap_range_t; +DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_batch_t); +#endif /* * Unmaps the page appearing at a particular GPFN from the specified guest's [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 5/5] rename XENMEM_add_to_physmap_{range => batch} 2013-12-20 10:44 ` [PATCH v3 5/5] rename XENMEM_add_to_physmap_{range => batch} Jan Beulich @ 2013-12-20 10:55 ` Ian Campbell 2013-12-20 12:05 ` Jan Beulich 0 siblings, 1 reply; 47+ messages in thread From: Ian Campbell @ 2013-12-20 10:55 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 10:44 +0000, Jan Beulich wrote: > +#if __XEN_INTERFACE_VERSION__ < 0x00040400 > +#define XENMEM_add_to_physmap_range XENMEM_add_to_physmap_batch > +#define xen_add_to_physmap_range xen_add_to_physmap_batch > +typedef struct xen_add_to_physmap_batch xen_add_to_physmap_range_t; > +DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_batch_t); This last one should be a x_a_t_p_range_t I think? > +#endif > > /* > * Unmaps the page appearing at a particular GPFN from the specified guest's > > ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 5/5] rename XENMEM_add_to_physmap_{range => batch} 2013-12-20 10:55 ` Ian Campbell @ 2013-12-20 12:05 ` Jan Beulich 2013-12-20 12:09 ` Ian Campbell 0 siblings, 1 reply; 47+ messages in thread From: Jan Beulich @ 2013-12-20 12:05 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini >>> On 20.12.13 at 11:55, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Fri, 2013-12-20 at 10:44 +0000, Jan Beulich wrote: >> +#if __XEN_INTERFACE_VERSION__ < 0x00040400 >> +#define XENMEM_add_to_physmap_range XENMEM_add_to_physmap_batch >> +#define xen_add_to_physmap_range xen_add_to_physmap_batch >> +typedef struct xen_add_to_physmap_batch xen_add_to_physmap_range_t; >> +DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_batch_t); > > This last one should be a x_a_t_p_range_t I think? Oh, yes, of course. Jan ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 5/5] rename XENMEM_add_to_physmap_{range => batch} 2013-12-20 12:05 ` Jan Beulich @ 2013-12-20 12:09 ` Ian Campbell 0 siblings, 0 replies; 47+ messages in thread From: Ian Campbell @ 2013-12-20 12:09 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Stefano Stabellini On Fri, 2013-12-20 at 12:05 +0000, Jan Beulich wrote: > >>> On 20.12.13 at 11:55, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Fri, 2013-12-20 at 10:44 +0000, Jan Beulich wrote: > >> +#if __XEN_INTERFACE_VERSION__ < 0x00040400 > >> +#define XENMEM_add_to_physmap_range XENMEM_add_to_physmap_batch > >> +#define xen_add_to_physmap_range xen_add_to_physmap_batch > >> +typedef struct xen_add_to_physmap_batch xen_add_to_physmap_range_t; > >> +DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_batch_t); > > > > This last one should be a x_a_t_p_range_t I think? > > Oh, yes, of course. I had also meant to say: With that change: Acked-by: Ian Campbell <ian.campbell@citrix.com> Ian. ^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2013-12-20 12:45 UTC | newest]
Thread overview: 47+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-18 14:29 [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich
2013-12-18 14:34 ` [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich
2013-12-18 17:52 ` Ian Campbell
2013-12-20 7:48 ` Jan Beulich
2013-12-20 8:58 ` Ian Campbell
2013-12-20 9:12 ` Jan Beulich
2013-12-20 9:55 ` Ian Campbell
2013-12-18 14:35 ` [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling Jan Beulich
2013-12-18 15:48 ` Tim Deegan
2013-12-18 16:03 ` Jan Beulich
2013-12-18 16:04 ` Tim Deegan
2013-12-18 17:59 ` Ian Campbell
2013-12-20 7:52 ` Jan Beulich
2013-12-20 9:02 ` Ian Campbell
2013-12-20 9:16 ` Jan Beulich
2013-12-18 14:35 ` [PATCH 3/4] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich
2013-12-18 15:59 ` Tim Deegan
2013-12-18 16:08 ` Jan Beulich
2013-12-18 16:14 ` Tim Deegan
2013-12-19 10:48 ` Ian Campbell
2013-12-20 7:58 ` Jan Beulich
2013-12-20 9:09 ` Ian Campbell
2013-12-20 9:27 ` Jan Beulich
2013-12-18 14:36 ` [PATCH 4/4] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich
2013-12-18 14:43 ` [PATCH 0/4] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Ian Campbell
2013-12-18 15:04 ` Jan Beulich
2013-12-18 17:15 ` Keir Fraser
2013-12-20 8:22 ` [PATCH v2 " Jan Beulich
2013-12-20 8:28 ` [PATCH v2 1/4] move XENMEM_add_to_physmap handling framework to common code Jan Beulich
2013-12-20 8:29 ` [PATCH v2 2/4] fix XENMEM_add_to_physmap preemption handling Jan Beulich
2013-12-20 8:29 ` [PATCH v2 3/4] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich
2013-12-20 8:30 ` [PATCH v2 4/4] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich
2013-12-20 10:35 ` [PATCH v3 0/5] fix preemption handling for XENMEM_add_to_physmap_range{, _range} Jan Beulich
2013-12-20 10:41 ` [PATCH v3 1/5] move XENMEM_add_to_physmap handling framework to common code Jan Beulich
2013-12-20 10:45 ` Ian Campbell
2013-12-20 12:26 ` Andrew Cooper
2013-12-20 12:45 ` Jan Beulich
2013-12-20 10:42 ` [PATCH v3 2/5] fix XENMEM_add_to_physmap preemption handling Jan Beulich
2013-12-20 10:47 ` Ian Campbell
2013-12-20 10:42 ` [PATCH v3 3/5] move XENMEM_add_to_physmap_range handling framework to common code Jan Beulich
2013-12-20 10:51 ` Ian Campbell
2013-12-20 10:43 ` [PATCH v3 4/5] fix XENMEM_add_to_physmap_range preemption handling Jan Beulich
2013-12-20 10:53 ` Ian Campbell
2013-12-20 10:44 ` [PATCH v3 5/5] rename XENMEM_add_to_physmap_{range => batch} Jan Beulich
2013-12-20 10:55 ` Ian Campbell
2013-12-20 12:05 ` Jan Beulich
2013-12-20 12:09 ` Ian Campbell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.