Re: Device model operation hypercall (DMOP, re qemu depriv)

[George Dunlap <george.dunlap@citrix.com>]

On 21/09/16 12:21, Ian Jackson wrote:
> Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv)"):
>> On 13.09.16 at 18:07, <david.vrabel@citrix.com> wrote:
>>> This is an direct result of the requirement that the privcmd driver does
>>> not know the types of the sub-ops themselves.  We can't have this
>>> requirement and type safety.  Which do we want?
>>
>> Both, which the proposal utilizing side band information on VA
>> ranges allows for. (And in any event this to me clearly is an
>> aspect that would need to be mentioned in the disadvantages
>> section of the document.)
> 
> The side band information approach has the problem that it is easy to
> accidentally write hypervisor code which misses the
> additionally-required range check.
> 
> It might be possible to provide both strict type safety of the kind
> you're concerned about here, _and_ protection against missing access
> checks, but it's not very straightforward.
> 
> One approach to do that would be to augment the guest handle array
> proposal with a typesafe macro system for accessing the guest handle
> slots.
> 
> But I think George has a good argument as to why this is not needed:
> 
>   If most of the time the hypercalls are made by calling libxc functions,
>   and the libxc functions have types as arguments, then the end caller has
>   the same type safety.  We'll have to be careful inside the individual
>   libxc functions, but that should be fairly straightforward to do.  So
>   the places where we need to take extra care should be very localized.
> 
> We do not expect DMOP callers to make the hypercalls directly.  (They
> can't sensibly do so because the DMOP ABI is not stable - they need
> the assistance of the stability layer which we intend to provide in
> libxc.)
> 
> So the actual hypercall call sites are all in-tree, in libxc.  If the
> format of the struct used for one of these guest handle slots changes,
> the same struct definition from the Xen headers is used both in the
> hypervisor and in libxc, and any incompatibility will be detected.
> 
> It's true that changes to the semantics of these slots (for example, a
> change of a slot from "array of struct X" to "one struct Y") will not
> be detected by the compiler.
> 
> But *all* ABI changes to the DMOP interface need to be accompanied by
> changes to libxc to add compatibility code.  I think it will be fairly
> straightforward to check, during review, that each DMOP change comes
> with a corresponding libxc change.
> 
> 
> But if you do not agree, then how about hiding the slots with a macro
> setup ?  Something like:
> 
>     struct device_model_op {
>          uint32_t op;
>          union {
>               struct op_foo foo;
>               struct op_bar bar;
>               /* etc... */
>          } u;
>     };
> 
>     struct op_foo {
>        some stuff;
>     #define DMOP_GUESTHANDLES_foo(O,ONE,MANY) \
>        ONE(O,  struct op_foo_big_block,           big_block) \
>        MANY(O, struct op_foo_data_array_element,  data_array)
>     };
>     DMOP_DEFINE(foo)
> 
> 
> Supported (preceded by) something like this (many toothpicks elided
> for clarity and ease of editing):
> 
>     /* dmop typesafe slot machinery: */
> 
>     #define DMOP_DEFINE(opname) \
>         enum {
>             DMOP_GUESTHANDLES_##opname(DMOP__SLOT_INDEX,DMOP__SLOT_INDEX)
>         };
>         DMOP_GUESTHANDLES_##opname(DMOP__ACCESSORS_ONE,DMOP__ACCESSORS_MANY)
> 
>     #define DMOP__SLOT_INDEX(O,T,N) DMOP__SLOT_INDEX__##O##_##N,
> 
>     #define DMOP__ACCESSORS_ONE(O,T,N) \
>         static inline int copy_dm_buffer_from_guest_##O##_##N(
>             T *dst,
>             const struct device_model_op *dmop /* accesses [nr_]buffers */)
>         {
>             return copy_dm_buffer_from_guest__raw
>                 ((void*)dst, sizeof(*dst), dmop,
>                  DMOP__SLOT_INDEX__##O##_##N);
>         }
>         likewise copy_...to
> 
>     #define DMOP__ACCESSORS_MANY(O,T,N) \
>         static inline size_t get_dm_buffer_array_size_##O##_##N ... {
>             calculation divides buffer size by type size;
>         }
>         static inline long copy_dm_buffer_from_guest_##O##_##N(
>             T *dst, size_t nr_dst,
>             const struct device_model_op *dmop /* accesses [nr_]buffers */)
>         {
> 
> 
> Personally I don't think this is worth the effort.  But if you do I
> would be happy to turn this into something which actually compiles
> :-).
> 
> Thanks for your attention.

I think the other slight bit of information missing from Jan at least
(and perhaps also David) is the difference between their preference /
recommendation and their requirement.

That is, if David and Jan each strongly recommend their own preferred
method, but are willing to (perhaps grudingly) give Ack's to an
implementaton of the other's method, then it's really down to the one
doing the implementation to read the advice and make their own decision
as best they can.

If someone feels strongly enough to Nack the other version, please say
so now; otherwise, I think Ian (since it seems like he'll be the one
implementing it) should make his best judgement based on the arguments
available and begin implementation.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel