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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.11.11 at 16:44, Jean Guyader <jean.guyader@xxxxxxxxxxxxx> wrote:
> 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.
>> 
>> >+
>> >+            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).
>> 
>> > 
>> >--- 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, did you have something like the attached patch in mind?

Indeed, and it's fortunately much simpler than I would have thought.

> We will return EFAULT to the hypercall without touching the guest memory
> because the copy_to_guest failed.
> 
> For the example I ignored the multicalls.

Adding the support for that would seem to be strait forward too.

Jan


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