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