x86/HVM: fix preemption handling in do_hvm_op() (try 2) Just like previously done for some mem-op hypercalls, undo preemption using the interface structures (altering it in ways the caller may not expect) and replace it by storing the continuation point in the high bits of sub-operation argument. This also changes the "nr" fields of struct xen_hvm_track_dirty_vram (operation already limited to 1Gb worth of pages) and struct xen_hvm_modified_memory to be only 32 bits wide, consistent with those of struct xen_set_mem{type,access}. If that's not acceptable for some reason, we'd need to shrink the HVMOP_op_bits (while still enforcing the [then higher] limit resulting from the need to be able to encode the continuation). Whether (and if so how) to adjust xc_hvm_track_dirty_vram(), xc_hvm_modified_memory(), xc_hvm_set_mem_type(), and xc_hvm_set_mem_access() to reflect the 32-bit restriction on "nr" is unclear: If the APIs need to remain stable, all four functions should probably check that there was no truncation. Preferably their parameters would be changed to uint32_t or unsigned int, though. Signed-off-by: Jan Beulich --- v2: Avoid double increment of pfn in HVMOP_modified_memory and HVMOP_set_mem_type handling. Change preemption logic to also work for 32-bit callers. Drop unrelated cleanup. --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4047,13 +4047,16 @@ static int hvm_replace_event_channel(str return 0; } +#define HVMOP_op_mask 0xff + long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *curr_d = current->domain; + unsigned long start_iter = op & ~HVMOP_op_mask; long rc = 0; - switch ( op ) + switch ( op &= HVMOP_op_mask ) { case HVMOP_set_param: case HVMOP_get_param: @@ -4383,7 +4386,8 @@ long do_hvm_op(unsigned long op, XEN_GUE goto param_fail3; rc = -EINVAL; - if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) || + if ( a.nr < start_iter || + ((a.first_pfn + a.nr - 1) < a.first_pfn) || ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) ) goto param_fail3; @@ -4391,9 +4395,9 @@ long do_hvm_op(unsigned long op, XEN_GUE if ( !paging_mode_log_dirty(d) ) goto param_fail3; - while ( a.nr > 0 ) + while ( a.nr > start_iter ) { - unsigned long pfn = a.first_pfn; + unsigned long pfn = a.first_pfn + start_iter; struct page_info *page; page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE); @@ -4406,16 +4410,11 @@ long do_hvm_op(unsigned long op, XEN_GUE put_page(page); } - a.first_pfn++; - a.nr--; - /* Check for continuation if it's not the last interation */ - if ( a.nr > 0 && hypercall_preempt_check() ) + if ( a.nr > ++start_iter && !(start_iter & HVMOP_op_mask) && + hypercall_preempt_check() ) { - if ( __copy_to_guest(arg, &a, 1) ) - rc = -EFAULT; - else - rc = -EAGAIN; + rc = -EAGAIN; break; } } @@ -4497,16 +4496,17 @@ long do_hvm_op(unsigned long op, XEN_GUE goto param_fail4; rc = -EINVAL; - if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) || + if ( a.nr < start_iter || + ((a.first_pfn + a.nr - 1) < a.first_pfn) || ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) ) goto param_fail4; if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ) goto param_fail4; - while ( a.nr ) + while ( a.nr > start_iter ) { - unsigned long pfn = a.first_pfn; + unsigned long pfn = a.first_pfn + start_iter; p2m_type_t t; p2m_type_t nt; mfn_t mfn; @@ -4547,16 +4547,11 @@ long do_hvm_op(unsigned long op, XEN_GUE } put_gfn(d, pfn); - a.first_pfn++; - a.nr--; - /* Check for continuation if it's not the last interation */ - if ( a.nr > 0 && hypercall_preempt_check() ) + if ( a.nr > ++start_iter && !(start_iter & HVMOP_op_mask) && + hypercall_preempt_check() ) { - if ( __copy_to_guest(arg, &a, 1) ) - rc = -EFAULT; - else - rc = -EAGAIN; + rc = -EAGAIN; goto param_fail4; } } @@ -4590,19 +4585,17 @@ long do_hvm_op(unsigned long op, XEN_GUE rc = -EINVAL; if ( (a.first_pfn != ~0ull) && - (((a.first_pfn + a.nr - 1) < a.first_pfn) || + (a.nr < start_iter || + ((a.first_pfn + a.nr - 1) < a.first_pfn) || ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) ) goto param_fail5; - rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access); + rc = p2m_set_mem_access(d, a.first_pfn, a.nr, start_iter, + HVMOP_op_mask, a.hvmmem_access); if ( rc > 0 ) { - a.first_pfn += a.nr - rc; - a.nr = rc; - if ( __copy_to_guest(arg, &a, 1) ) - rc = -EFAULT; - else - rc = -EAGAIN; + start_iter = rc; + rc = -EAGAIN; } param_fail5: @@ -4751,8 +4744,11 @@ long do_hvm_op(unsigned long op, XEN_GUE } if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_hvm_op, "lh", op, arg); + { + ASSERT(!(start_iter & HVMOP_op_mask)); + rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh", + op | start_iter, arg); + } return rc; } --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1334,13 +1334,13 @@ void p2m_mem_access_resume(struct domain /* Set access type for a region of pfns. * If start_pfn == -1ul, sets the default access type */ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr, - hvmmem_access_t access) + uint32_t start, uint32_t mask, hvmmem_access_t access) { struct p2m_domain *p2m = p2m_get_hostp2m(d); p2m_access_t a, _a; p2m_type_t t; mfn_t mfn; - long rc; + long rc = 0; /* N.B. _not_ static: initializer depends on p2m->default_access */ p2m_access_t memaccess[] = { @@ -1369,11 +1369,8 @@ long p2m_set_mem_access(struct domain *d return 0; } - if ( !nr ) - return 0; - p2m_lock(p2m); - for ( ; ; ++pfn ) + for ( pfn += start; nr > start; ++pfn ) { mfn = p2m->get_entry(p2m, pfn, &t, &_a, 0, NULL); if ( p2m->set_entry(p2m, pfn, mfn, PAGE_ORDER_4K, t, a) == 0 ) @@ -1383,9 +1380,9 @@ long p2m_set_mem_access(struct domain *d } /* Check for continuation if it's not the last interation. */ - if ( !--nr || hypercall_preempt_check() ) + if ( nr > ++start && !(start & mask) && hypercall_preempt_check() ) { - rc = nr; + rc = start; break; } } --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -576,8 +576,8 @@ void p2m_mem_access_resume(struct domain /* Set access type for a region of pfns. * If start_pfn == -1ul, sets the default access type */ -long p2m_set_mem_access(struct domain *d, unsigned long start_pfn, - uint32_t nr, hvmmem_access_t access); +long p2m_set_mem_access(struct domain *d, unsigned long start_pfn, uint32_t nr, + uint32_t start, uint32_t mask, hvmmem_access_t access); /* Get access type for a pfn * If pfn == -1ul, gets the default access type */ --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -90,10 +90,10 @@ typedef enum { struct xen_hvm_track_dirty_vram { /* Domain to be tracked. */ domid_t domid; + /* Number of pages to track. */ + uint32_t nr; /* First pfn to track. */ uint64_aligned_t first_pfn; - /* Number of pages to track. */ - uint64_aligned_t nr; /* OUT variable. */ /* Dirty bitmap buffer. */ XEN_GUEST_HANDLE_64(uint8) dirty_bitmap; @@ -106,10 +106,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_track_di struct xen_hvm_modified_memory { /* Domain to be updated. */ domid_t domid; + /* Number of pages. */ + uint32_t nr; /* First pfn. */ uint64_aligned_t first_pfn; - /* Number of pages. */ - uint64_aligned_t nr; }; typedef struct xen_hvm_modified_memory xen_hvm_modified_memory_t; DEFINE_XEN_GUEST_HANDLE(xen_hvm_modified_memory_t);