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