[PATCH 0/2] x86/HVM: XSA-89 follow-ups

[PATCH 0/2] x86/HVM: XSA-89 follow-ups

[Jan Beulich]

1: simplify do_hvm_op()
2: fix preemption handling in do_hvm_op()

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/2] x86/HVM: simplify do_hvm_op()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]

- boundary checks in HVMOP_modified_memory, HVMOP_set_mem_type, and
  HVMOP_set_mem_access: all of these already check for overflow, so
  there's no need to range check the first _and_ last PFN (checking
  the last one suffices)
- copying back interface structures that were previously copied from
  guest memory can use __copy_to_...(), since copy_from_...() already
  did the address range validation

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4288,7 +4288,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 a.value = d->arch.hvm_domain.params[a.index];
                 break;
             }
-            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
 
         HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
@@ -4386,8 +4386,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail3;
 
         rc = -EINVAL;
-        if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+        if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto param_fail3;
 
@@ -4416,7 +4415,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             /* Check for continuation if it's not the last interation */
             if ( a.nr > 0 && hypercall_preempt_check() )
             {
-                if ( copy_to_guest(arg, &a, 1) )
+                if ( __copy_to_guest(arg, &a, 1) )
                     rc = -EFAULT;
                 else
                     rc = -EAGAIN;
@@ -4465,7 +4464,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 a.mem_type =  HVMMEM_ram_rw;
             else
                 a.mem_type =  HVMMEM_mmio_dm;
-            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
 
     param_fail_getmemtype:
@@ -4501,8 +4500,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail4;
 
         rc = -EINVAL;
-        if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+        if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto param_fail4;
             
@@ -4558,7 +4556,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             /* Check for continuation if it's not the last interation */
             if ( a.nr > 0 && hypercall_preempt_check() )
             {
-                if ( copy_to_guest(arg, &a, 1) )
+                if ( __copy_to_guest(arg, &a, 1) )
                     rc = -EFAULT;
                 else
                     rc = -EAGAIN;
@@ -4595,9 +4593,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
 
         rc = -EINVAL;
         if ( (a.first_pfn != ~0ull) &&
-             ((a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
-             ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
+             (((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);
@@ -4646,7 +4643,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail6;
 
         a.hvmmem_access = access;
-        rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
 
     param_fail6:
         rcu_unlock_domain(d);




[-- Attachment #2: x86-HVM-op-simplify.patch --]
[-- Type: text/plain, Size: 3813 bytes --]

x86/HVM: simplify do_hvm_op()

- boundary checks in HVMOP_modified_memory, HVMOP_set_mem_type, and
  HVMOP_set_mem_access: all of these already check for overflow, so
  there's no need to range check the first _and_ last PFN (checking
  the last one suffices)
- copying back interface structures that were previously copied from
  guest memory can use __copy_to_...(), since copy_from_...() already
  did the address range validation

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4288,7 +4288,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 a.value = d->arch.hvm_domain.params[a.index];
                 break;
             }
-            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
 
         HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
@@ -4386,8 +4386,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail3;
 
         rc = -EINVAL;
-        if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+        if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto param_fail3;
 
@@ -4416,7 +4415,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             /* Check for continuation if it's not the last interation */
             if ( a.nr > 0 && hypercall_preempt_check() )
             {
-                if ( copy_to_guest(arg, &a, 1) )
+                if ( __copy_to_guest(arg, &a, 1) )
                     rc = -EFAULT;
                 else
                     rc = -EAGAIN;
@@ -4465,7 +4464,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 a.mem_type =  HVMMEM_ram_rw;
             else
                 a.mem_type =  HVMMEM_mmio_dm;
-            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
 
     param_fail_getmemtype:
@@ -4501,8 +4500,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail4;
 
         rc = -EINVAL;
-        if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+        if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto param_fail4;
             
@@ -4558,7 +4556,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             /* Check for continuation if it's not the last interation */
             if ( a.nr > 0 && hypercall_preempt_check() )
             {
-                if ( copy_to_guest(arg, &a, 1) )
+                if ( __copy_to_guest(arg, &a, 1) )
                     rc = -EFAULT;
                 else
                     rc = -EAGAIN;
@@ -4595,9 +4593,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
 
         rc = -EINVAL;
         if ( (a.first_pfn != ~0ull) &&
-             ((a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
-             ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
+             (((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);
@@ -4646,7 +4643,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail6;
 
         a.hvmmem_access = access;
-        rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
 
     param_fail6:
         rcu_unlock_domain(d);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/2] x86/HVM: simplify do_hvm_op()

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 4107 bytes --]

On 25/03/14 16:03, Jan Beulich wrote:
> - boundary checks in HVMOP_modified_memory, HVMOP_set_mem_type, and
>   HVMOP_set_mem_access: all of these already check for overflow, so
>   there's no need to range check the first _and_ last PFN (checking
>   the last one suffices)
> - copying back interface structures that were previously copied from
>   guest memory can use __copy_to_...(), since copy_from_...() already
>   did the address range validation
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4288,7 +4288,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
>                  a.value = d->arch.hvm_domain.params[a.index];
>                  break;
>              }
> -            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>          }
>  
>          HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
> @@ -4386,8 +4386,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
>              goto param_fail3;
>  
>          rc = -EINVAL;
> -        if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
> -             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> +        if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>              goto param_fail3;
>  
> @@ -4416,7 +4415,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
>              /* Check for continuation if it's not the last interation */
>              if ( a.nr > 0 && hypercall_preempt_check() )
>              {
> -                if ( copy_to_guest(arg, &a, 1) )
> +                if ( __copy_to_guest(arg, &a, 1) )
>                      rc = -EFAULT;
>                  else
>                      rc = -EAGAIN;
> @@ -4465,7 +4464,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
>                  a.mem_type =  HVMMEM_ram_rw;
>              else
>                  a.mem_type =  HVMMEM_mmio_dm;
> -            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>          }
>  
>      param_fail_getmemtype:
> @@ -4501,8 +4500,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
>              goto param_fail4;
>  
>          rc = -EINVAL;
> -        if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
> -             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> +        if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>              goto param_fail4;
>              
> @@ -4558,7 +4556,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
>              /* Check for continuation if it's not the last interation */
>              if ( a.nr > 0 && hypercall_preempt_check() )
>              {
> -                if ( copy_to_guest(arg, &a, 1) )
> +                if ( __copy_to_guest(arg, &a, 1) )
>                      rc = -EFAULT;
>                  else
>                      rc = -EAGAIN;
> @@ -4595,9 +4593,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
>  
>          rc = -EINVAL;
>          if ( (a.first_pfn != ~0ull) &&
> -             ((a.first_pfn > domain_get_maximum_gpfn(d)) ||
> -             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> -             ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
> +             (((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);
> @@ -4646,7 +4643,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
>              goto param_fail6;
>  
>          a.hvmmem_access = access;
> -        rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>  
>      param_fail6:
>          rcu_unlock_domain(d);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 4965 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 9492 bytes --]

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 <jbeulich@suse.com>

--- 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);



[-- Attachment #2: x86-HVM-op-preemption.patch --]
[-- Type: text/plain, Size: 9539 bytes --]

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 <jbeulich@suse.com>

--- 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);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[Aravindh Puthiyaparambil (aravindp)]

>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.

I am adding mem_access support to PV domains. As part of that I am adding a more generic xc_set/get_mem_access() APIs and deprecating the xc_hvm_set/get_mem_access() APIs. At the moment I have the xc_hvm_set/get_mem_access() APIs calling the newer ones. I am also folding the mem_access ops under XENMEM_access_op and deprecating the HVMOPs. Maybe as part of this I can change the nr parameter to uint32_t. Please advice and I will make it part of the patch which should be ready in a couple of weeks.

Thanks,
Aravindh

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

Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[Jan Beulich]

>>> On 25.03.14 at 19:40, <aravindp@cisco.com> wrote:
>> 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.
> 
> I am adding mem_access support to PV domains. As part of that I am adding a 
> more generic xc_set/get_mem_access() APIs and deprecating the 
> xc_hvm_set/get_mem_access() APIs. At the moment I have the 
> xc_hvm_set/get_mem_access() APIs calling the newer ones. I am also folding 
> the mem_access ops under XENMEM_access_op and deprecating the HVMOPs. Maybe 
> as part of this I can change the nr parameter to uint32_t. Please advice and 
> I will make it part of the patch which should be ready in a couple of weeks.

You'll need to think about how to handle preemption anyway there,
and I would guess that you'd want to go with the same method used
here (due to the alternatives being more complicated). Hence you'll
need to introduce an upper bound on the number of pages anyway,
and one way that could be done would be by limiting the respective
interface structure field to 32 bits.

Jan

Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[George Dunlap]

On Tue, Mar 25, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> 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).

So these calls are made directly by qemu -- and I thought that we were
trying to have a stable ABI with qemu so that people could pop in any
(recent) version of qemu and have it Just Work.

Would this have an impact on that?  It seems like if qemu is
dynamically linked to libxc, and libxc is matched to an appropriate
hypervisor, that shouldn't be an issue.

However, at the moment libxc exposes "nr" as uint64_t; leaving it that
way would be very misleading.

Is there a real problem with "altering [interface structures] in a way
the caller may not expect"?  Can't we just document that Xen may
change some of the structures?

 -George

Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[Ian Campbell]

On Wed, 2014-03-26 at 14:42 +0000, George Dunlap wrote:
> On Tue, Mar 25, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > 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).
> 
> So these calls are made directly by qemu -- and I thought that we were
> trying to have a stable ABI with qemu so that people could pop in any
> (recent) version of qemu and have it Just Work.

I don't think we've said that, we've always been quite explicit that
libxc is an unstable API.

AFAIK qemu contains a compat shim which allows it to compile against
multiple libxc versions. If libxc is to change again then qemu might
need updating.

To some extent we've resisted gratuitous cleanups etc to libxc if they
would impact e.g. qemu, but if a change is required then we should go
ahead with it (NB: I've not looked into the actual proposal in this
thread to know which it is).

Maybe we should make stronger guarantees about interfaces used by device
models but that process would have to start with an enumeration of
exactly what those interfaces are. *If* we were to then go ahead with
that then I would advocate moving those interfaces into a library other
than libxc so it is clear what is what.

Ian.

Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[Jan Beulich]

>>> On 26.03.14 at 15:42, <George.Dunlap@eu.citrix.com> wrote:
> Is there a real problem with "altering [interface structures] in a way
> the caller may not expect"?  Can't we just document that Xen may
> change some of the structures?

The two main problems here, just like with the memory op we had to
change before 4.4, are
- we can't and shouldn't assume that qemu is the only consumer, and
  hence surprises to the caller must be avoided
- no interface anywhere in the tree should set bad precedents, or
  else keeping people from doing the same wrong thing elsewhere is
  much harder to achieve (i.e. just like in so many other places
  recently, I'm advocating for consistency throughout the source
  base here)

Jan

Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[George Dunlap]

On 03/26/2014 03:44 PM, Jan Beulich wrote:
>>>> On 26.03.14 at 15:42, <George.Dunlap@eu.citrix.com> wrote:
>> Is there a real problem with "altering [interface structures] in a way
>> the caller may not expect"?  Can't we just document that Xen may
>> change some of the structures?
> The two main problems here, just like with the memory op we had to
> change before 4.4, are
> - we can't and shouldn't assume that qemu is the only consumer, and
>    hence surprises to the caller must be avoided
> - no interface anywhere in the tree should set bad precedents, or
>    else keeping people from doing the same wrong thing elsewhere is
>    much harder to achieve (i.e. just like in so many other places
>    recently, I'm advocating for consistency throughout the source
>    base here)

"Surprises to the caller" can be avoided by adding comments in the 
public header.  The hypercall can't be used without someone looking at 
the structure definition. :-)

"Setting a bad precedent" is a reasonable argument *if* we're not 
constrained by ABI / API compatibility.  I agree with IanC, that libxc 
should be an internal-only, unstable interface; and that we should 
probably come up with some more reasonable way to support "external" 
APIs like this for device models.

  -George

Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[Jan Beulich]

>>> On 26.03.14 at 18:06, <george.dunlap@eu.citrix.com> wrote:
> On 03/26/2014 03:44 PM, Jan Beulich wrote:
>>>>> On 26.03.14 at 15:42, <George.Dunlap@eu.citrix.com> wrote:
>>> Is there a real problem with "altering [interface structures] in a way
>>> the caller may not expect"?  Can't we just document that Xen may
>>> change some of the structures?
>> The two main problems here, just like with the memory op we had to
>> change before 4.4, are
>> - we can't and shouldn't assume that qemu is the only consumer, and
>>    hence surprises to the caller must be avoided
>> - no interface anywhere in the tree should set bad precedents, or
>>    else keeping people from doing the same wrong thing elsewhere is
>>    much harder to achieve (i.e. just like in so many other places
>>    recently, I'm advocating for consistency throughout the source
>>    base here)
> 
> "Surprises to the caller" can be avoided by adding comments in the 
> public header.  The hypercall can't be used without someone looking at 
> the structure definition. :-)

But see - the public header doesn't have any such comments right
now, and hence I'm implying it to state that the structures don't
change except for fields designated for output.

> "Setting a bad precedent" is a reasonable argument *if* we're not 
> constrained by ABI / API compatibility.  I agree with IanC, that libxc 
> should be an internal-only, unstable interface; and that we should 
> probably come up with some more reasonable way to support "external" 
> APIs like this for device models.

And again - you don't mention this aspect at all - consistency is quite
significant an aspect. And the interface stability requirements at the
libxc layer are completely independent of the ones of the hypercalls.
Back when I joined the Xen project, it was already the case that tools-
only interfaces were allowed to change at any time, and to my
knowledge this didn't change. Hence if we fix the hypercalls here and
libxc would have to retain its current interface, we'd need to adjust
libxc accordingly (as said in the original submission, some change to
the library is going to be at least desirable anyway).

Jan

Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()

[Ian Campbell]

On Thu, 2014-03-27 at 08:05 +0000, Jan Beulich wrote:
> >>> On 26.03.14 at 18:06, <george.dunlap@eu.citrix.com> wrote:
> > On 03/26/2014 03:44 PM, Jan Beulich wrote:
> >>>>> On 26.03.14 at 15:42, <George.Dunlap@eu.citrix.com> wrote:
> >>> Is there a real problem with "altering [interface structures] in a way
> >>> the caller may not expect"?  Can't we just document that Xen may
> >>> change some of the structures?
> >> The two main problems here, just like with the memory op we had to
> >> change before 4.4, are
> >> - we can't and shouldn't assume that qemu is the only consumer, and
> >>    hence surprises to the caller must be avoided
> >> - no interface anywhere in the tree should set bad precedents, or
> >>    else keeping people from doing the same wrong thing elsewhere is
> >>    much harder to achieve (i.e. just like in so many other places
> >>    recently, I'm advocating for consistency throughout the source
> >>    base here)
> > 
> > "Surprises to the caller" can be avoided by adding comments in the 
> > public header.  The hypercall can't be used without someone looking at 
> > the structure definition. :-)
> 
> But see - the public header doesn't have any such comments right
> now, and hence I'm implying it to state that the structures don't
> change except for fields designated for output.
> 
> > "Setting a bad precedent" is a reasonable argument *if* we're not 
> > constrained by ABI / API compatibility.  I agree with IanC, that libxc 
> > should be an internal-only, unstable interface; and that we should 
> > probably come up with some more reasonable way to support "external" 
> > APIs like this for device models.
> 
> And again - you don't mention this aspect at all - consistency is quite
> significant an aspect. And the interface stability requirements at the
> libxc layer are completely independent of the ones of the hypercalls.
> Back when I joined the Xen project, it was already the case that tools-
> only interfaces were allowed to change at any time, and to my
> knowledge this didn't change.

Yes, to be clear I was only talking about the libxc A[BP]I.

I think for the purposes of supporting external device models and such
we would only need to declare a stable library wrapper interface, not
change any of the rules regarding the underlying hypercalls (i.e.
sysctl, domctl remain non-stable interfaces). If the hypercalls change
*and* we have declared a stable library interface over them then the
library would have to do impedance matching.

That's part of the reason I would insist on an enumeration and audit of
the library interfaces which we were to declare stable for the use of
qemu etc. As with the libxl stable API declaration we would want to make
sure that the interface was supportable as a frozen API going forward
before committing to it long term. I very much doubt that is the case of
many of the libxc interfaces right now.

Ian.

>  Hence if we fix the hypercalls here and
> libxc would have to retain its current interface, we'd need to adjust
> libxc accordingly (as said in the original submission, some change to
> the library is going to be at least desirable anyway).
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0/2] x86/HVM: XSA-89 follow-ups

[Tim Deegan]

At 15:51 +0000 on 25 Mar (1395759111), Jan Beulich wrote:
> 1: simplify do_hvm_op()
> 2: fix preemption handling in do_hvm_op()
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>