Re: [Xen-devel] [PATCH XEN v5 09/23] tools: Refactor hypercall calling wrappers into libxencall.

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-11-11 at 15:08 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH XEN v5 09/23] tools: Refactor hypercall
> calling wrappers into libxencall."):
> > libxencall will provide a stable API and ABI for calling hypercalls
> > (although those hypercalls themselves may not have a stable API). As
> > well as the hypercall buffer infrastructure needed in order to safely
> > provide pointer arguments to hypercalls.
> ...
> > +/*
> > + * This library allows you to make arbitrary hypercalls (subject to
> > + * sufficient permission for the process and the domain itself). Note
> > + * that while the library interface is stable the hypercalls are
> > + * subject to their own rules.
> 
> Something needs to say what the error handling is like.
> 
> Do these functions set errno ?

Yes, they should have the customary "return -1 and set errno" pattern. I
shall make this explicit.

> > +/*
> > + * Call hypercalls with varying numbers of arguments.
> > + */
> > +int xencall0(xencall_handle *xcall, unsigned int op);
> 
> Is the return value the raw hypercall return value, or is hypervisor
> do_foo returning -EFOOBAR turned into to -1/errno=EFOOBAR ?
> (Hopefully the answer to this doesn't depend on the hypercall ABI...)

Oh god, this looks to vary by platform :-(

Linux returns the hypercall return value directly from the ioctl as its
return value, which the usual libc stuff turns into -1/errno=EFOO on
failure.

FreeBSD has a separate hypervisor retval field in the ioctl argument struct
and does:
    return (ret == 0) ? hypercall->retval : ret;

MiniOS goes for -1/errno=EFOO.

What a mess.

Either Linux+MiniOS or FreeBSD are going to have to change (i.e. in the
wrappers in this library, not the underlying mechanisms) to match the other
for consistency.

On FreeBSD hypercall->retval is in the XEN_ERRNO_* space. But I think we
probably want to map onto the FreeBSD ERRNO space, IIRC we decided on this
approach, rather than converting all our callers to the XEN_ERRNO_* space,
in a previous discussion.

If we are going to standardise on one of those then given we've gone for
-1/errno in most other places and it's the "normal" way to do things I
think we should do the same here and the freebsd driver in libxencall
should set errno.

There are other alternatives, such as having this library return the
"privcmd ioctl" result in its return value + errno and the "hypercall
result" in a separate output pointer argument. This would require, possibly
substantial, changes in the callers, but right now they are all in libxc,
which could munge that back into the current somewhat braindead scheme
allowing us to update those call paths piecemeal and allowing new users to
use the sane interface.

Thoughts?

> > +/*
> > + * Allocate and free memory which is suitable for use as a pointer
> > + * argument to a hypercall.
> > + */
> > +void *xencall_alloc_buffer_pages(xencall_handle *xcall, int nr_pages);
> > +void xencall_free_buffer_pages(xencall_handle *xcall, void *p, int
> > nr_pages);
> > +
> > +void *xencall_alloc_buffer(xencall_handle *xcall, size_t size);
> > +void xencall_free_buffer(xencall_handle *xcall, void *p);
> 
> See above re error handling.
> 
> Can these functions be used without (a) knowing the page size
> (b) a rounding macro ?
> 
> It would be best to save callers the trouble of providing those
> themselves.

The library deals with this internally, size can be anything the caller
likes.

Ian.

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