[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/2] x86/HVM: fix preemption handling in do_hvm_op() (try 2)



>-----Original Message-----
>From: Jan Beulich <JBeulich@xxxxxxxx>
>Date: Wed, Apr 2, 2014 at 1:09 AM
>Subject: [Xen-devel] [PATCH 1/2] x86/HVM: fix preemption handling in
>do_hvm_op() (try 2)
>To: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
>Cc: Tim Deegan <tim@xxxxxxx>, Keir Fraser <keir@xxxxxxx>
>
>
>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.

As part of my PV mem_access work, I am moving HVMOP_set_mem_access in to 
mem_access_memop() as XENMEM_access_op_set_access, to make it more generic. I 
am not sure how to go about doing the preemption handling here. I am not able 
to do something similar as the mem_access ops is a subarch_memory_op, so I was 
thinking of using the interface structures but the above comment has me 
worried. Is it not safe to do this for _set_mem_access op? I will send out an 
RFC patch to make it clear as to what I am doing. Please take a look and advise 
as to how I should proceed.

Thanks,
Aravindh

>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 <jbeulich@xxxxxxxx>
>---
>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);
>
>
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@xxxxxxxxxxxxx
>http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.