[Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range

[Jean Guyader <jean.guyader@xxxxxxxxxxxxx>]

On 10/11 12:47, Jan Beulich wrote:
> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@xxxxxxxxxxxxx> wrote:
> >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) 
> >arg)
> >         }
> > 
> >         rc = xenmem_add_to_physmap(d, &xatp);
> >+        if ( rc == -EAGAIN )
> 
>         if ( rc )
> 
> >+        {
> >+            if ( copy_to_guest(arg, &xatp, 1) )
> >+            {
> >+                rcu_unlock_domain(d);
> >+                return -EFAULT;
> >+            }
> 
>         }
>         if ( rc == -EAGAIN )
> 
> (with some room for further simplification). Without that (or the minimal
> alternative of copying back just .size or yet some other mechanism), as
> pointed out before, the caller won't have a way to know how far into
> the batch things succeeded.
> 

In xenmem_add_to_physmap I modify xatp in place so when I exit this
function xatp will contain the updated value (new start value in
.gpfn and .idx, how far do I need to go in .size).

The idea here was to call the copy_to_guest only when we got preempted.
If I copy xatp back to the guest I should get the updated value
in xatp from the copy_from_guest when I'll be called by the continuation.

> >+
> >+            rc = hypercall_create_continuation(
> >+                    __HYPERVISOR_memory_op, "ih", op, arg);
> >+        }
> > 
> >         rcu_unlock_domain(d);
> 
> Also, the whole block above can be move past this rcu_unlock_domain(),
> eliminating the need to do it separately in the above error path(s).
> 

Ok.

Jean

> > 
> >--- a/xen/arch/x86/x86_64/compat/mm.c
> >+++ b/xen/arch/x86/x86_64/compat/mm.c
> >@@ -63,6 +63,18 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE(void) 
> >arg)
> > 
> >         XLAT_add_to_physmap(nat, &cmp);
> >         rc = arch_memory_op(op, guest_handle_from_ptr(nat, void));
> >+        if ( rc < 0 )
> >+            break;
> >+
> 
> With the way the code below is currently this is superfluous. But just
> as above you will need to provide some indication to the caller
> *where* the failure occurred.
> 
> >+        if ( rc == __HYPERVISOR_memory_op )
> >+        {
> >+            hypercall_xlat_continuation(NULL, 0x2, nat, arg);
> >+
> >+            XLAT_add_to_physmap(&cmp, nat);
> >+
> >+            if ( copy_to_guest(arg, &cmp, 1) )
> >+                return -EFAULT;
> 
> I realize that this is the same way in the code handling
> XENMEM_[gs]et_pod_target, but unfortunately that's wrong (that's
> why I'm copying you, George): Once a continuation was set up, you
> mustn't change the return value anymore, since the continuation was
> established by adjusting the guest's rIP.
> 
> As for other memory ops, the continuation can be encoded in "op" (see
> xen/common/memory.c and xen/common/compat/memory.c). However,
> while suitable here I don't think that's usable for the PoD variant. The
> alternative is to cancel the continuation (would require quite a bit of
> new code I think) or to adjust the low level hypercall handler code (at
> least the compat mode one) to special case rAX values in the negative
> errno range, leaving rAX unchanged instead of returning -ENOSYS.
> Keir?
> 
> Jan
> 
> >+        }
> > 
> >         break;
> >     }
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel