Re: [Xen-devel] hypercall_xlat_continuation()

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> Ian Campbell <Ian.Campbell@xxxxxxxxxx> 24.05.09 00:36 >>>
>The mask argument to hypercall_xlat_continuation indicates which of the
>6 potential continuation arguments (corresponding to the up to 6
>arguments to a hypercall) need to be translated from a native value to a
>compat value. The least significant bit == the first argument, the
>second least is the second argument etc. For each bit that is set the
>varargs should contain a pair of additional arguments, the native value
>and the replacement compat value. The native value is compared to the
>value in the continuation before replacing it with the compat value. I
>would have thought the native value must always match by design so this
>might just be a sanity check.

It indeed is a sanity check, to avoid silently doing the wrong thing.

>For example do_memory_op takes arguments (cmd, nat.hnd) and therefore we
>pass mask==0x2 followed by varargs (nat.hnd, compat). So if the second
>continuation argument matches nat.hnd it will be replaced with compat.
>
>Similarly do_mmuext_op takes (nat_ops, otherstuff, etc...) and we pass a
>mask==0x01 with varargs (nat_ops, cmp_uops so if the first continuation
>argument matches nat_ops we replace it with cmp_uops.
>
>In both cases if the native and compat things are the same we ignore the
>bit set in the mask.

... the primary purpose of which is to deal with NULL arguments.

>I don't recall what the "BUG_ON(nval == (unsigned int)nval)" is all
>about. I guess the assumption is that if an argument requires
>translation it must be too large to fit in a compat sized variable. This
>seems to be true for the existing cases (which are both
>XEN_GUEST_HANDLEs), I don't see why it would be true in general. Maybe
>the assumption is that only XEN_GUEST_HANDLES ever need translation?

The intention here is to avoid someone trying to put translated arguments
in guest visible space (i.e. va < 4G).

>...
>
>The first argument to hypercall_xlat_continuation (unsigned int *id) is
>a pointer to an index which, if non-NULL, is replaced with the value of
>the argument at that index in the continuation, I think it's just used
>for sanity checking, I'm not all that convinced it is necessary (maybe
>it was useful when initially debugging this stuff)

No, this is not just for sanity checking. For an example, look at
compat_memory_op()'s use of it: It has no other way of knowing how much
of the current batch do_memory_op() actually processed, since the return
value is either an error code or __HYPERVISOR_memory_op.

>It all seems rather complicated and fragile to me, but it does seem to
>work so I'm not inclined to go poking at it...

Indeed, I didn't try to make it artificially complicated, but the main goal
was to keep the users of it simple (which I still believe they are). I'd be
all for simplifying it *without* imposing more complexity on the callers.
Otoh I don't think it's *that* complicated...

Jan


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