x86/HVM: fix preemption handling in do_hvm_op() 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. As a minor cleanup, along with introducing the switch-wide "pfn" the redundant "d" is also being converted to a switch-wide one. Signed-off-by: Jan Beulich --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4050,20 +4050,25 @@ static int hvm_replace_event_channel(str return 0; } +#define HVMOP_op_bits 32 + 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_bits; long rc = 0; - switch ( op ) + switch ( op &= ((1UL << HVMOP_op_bits) - 1) ) { + struct domain *d; + unsigned long pfn; + case HVMOP_set_param: case HVMOP_get_param: { struct xen_hvm_param a; struct hvm_ioreq_page *iorp; - struct domain *d; struct vcpu *v; if ( copy_from_guest(&a, arg, 1) ) @@ -4327,7 +4332,6 @@ long do_hvm_op(unsigned long op, XEN_GUE case HVMOP_track_dirty_vram: { struct xen_hvm_track_dirty_vram a; - struct domain *d; if ( copy_from_guest(&a, arg, 1) ) return -EFAULT; @@ -4368,7 +4372,6 @@ long do_hvm_op(unsigned long op, XEN_GUE case HVMOP_modified_memory: { struct xen_hvm_modified_memory a; - struct domain *d; if ( copy_from_guest(&a, arg, 1) ) return -EFAULT; @@ -4386,7 +4389,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; @@ -4394,9 +4398,8 @@ long do_hvm_op(unsigned long op, XEN_GUE if ( !paging_mode_log_dirty(d) ) goto param_fail3; - while ( a.nr > 0 ) + for ( pfn = a.first_pfn + start_iter; a.nr > start_iter; ++pfn ) { - unsigned long pfn = a.first_pfn; struct page_info *page; page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE); @@ -4409,16 +4412,13 @@ long do_hvm_op(unsigned long op, XEN_GUE put_page(page); } - a.first_pfn++; - a.nr--; + ++pfn; + ++start_iter; /* Check for continuation if it's not the last interation */ - if ( a.nr > 0 && hypercall_preempt_check() ) + if ( a.nr > start_iter && hypercall_preempt_check() ) { - if ( __copy_to_guest(arg, &a, 1) ) - rc = -EFAULT; - else - rc = -EAGAIN; + rc = -EAGAIN; break; } } @@ -4431,7 +4431,6 @@ long do_hvm_op(unsigned long op, XEN_GUE case HVMOP_get_mem_type: { struct xen_hvm_get_mem_type a; - struct domain *d; p2m_type_t t; if ( copy_from_guest(&a, arg, 1) ) @@ -4475,7 +4474,6 @@ long do_hvm_op(unsigned long op, XEN_GUE case HVMOP_set_mem_type: { struct xen_hvm_set_mem_type a; - struct domain *d; /* Interface types to internal p2m types */ static const p2m_type_t memtype[] = { @@ -4500,20 +4498,19 @@ 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 ) + for ( pfn = a.first_pfn + start_iter; a.nr > start_iter; ++pfn ) { - unsigned long pfn = a.first_pfn; - p2m_type_t t; - p2m_type_t nt; - mfn_t mfn; - mfn = get_gfn_unshare(d, pfn, &t); + p2m_type_t t, nt; + + get_gfn_unshare(d, pfn, &t); if ( p2m_is_paging(t) ) { put_gfn(d, pfn); @@ -4550,16 +4547,13 @@ long do_hvm_op(unsigned long op, XEN_GUE } put_gfn(d, pfn); - a.first_pfn++; - a.nr--; + ++pfn; + ++start_iter; /* Check for continuation if it's not the last interation */ - if ( a.nr > 0 && hypercall_preempt_check() ) + if ( a.nr > start_iter && hypercall_preempt_check() ) { - if ( __copy_to_guest(arg, &a, 1) ) - rc = -EFAULT; - else - rc = -EAGAIN; + rc = -EAGAIN; goto param_fail4; } } @@ -4574,7 +4568,6 @@ long do_hvm_op(unsigned long op, XEN_GUE case HVMOP_set_mem_access: { struct xen_hvm_set_mem_access a; - struct domain *d; if ( copy_from_guest(&a, arg, 1) ) return -EFAULT; @@ -4593,19 +4586,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 + start_iter, a.nr - start_iter, + 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 = a.nr - rc; + rc = -EAGAIN; } param_fail5: @@ -4616,7 +4607,6 @@ long do_hvm_op(unsigned long op, XEN_GUE case HVMOP_get_mem_access: { struct xen_hvm_get_mem_access a; - struct domain *d; hvmmem_access_t access; if ( copy_from_guest(&a, arg, 1) ) @@ -4653,7 +4643,6 @@ long do_hvm_op(unsigned long op, XEN_GUE case HVMOP_pagetable_dying: { struct xen_hvm_pagetable_dying a; - struct domain *d; if ( copy_from_guest(&a, arg, 1) ) return -EFAULT; @@ -4706,7 +4695,6 @@ long do_hvm_op(unsigned long op, XEN_GUE case HVMOP_inject_trap: { xen_hvm_inject_trap_t tr; - struct domain *d; struct vcpu *v; if ( copy_from_guest(&tr, arg, 1 ) ) @@ -4754,8 +4742,9 @@ long do_hvm_op(unsigned long op, XEN_GUE } if ( rc == -EAGAIN ) - rc = hypercall_create_continuation( - __HYPERVISOR_hvm_op, "lh", op, arg); + rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh", + op | (start_iter << HVMOP_op_bits), + arg); return rc; } --- 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);