[Xen-devel] Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Apr 29, 2016 at 04:30:16PM +0100, Wei Liu wrote:
> On Fri, Apr 29, 2016 at 05:12:51PM +0200, Roger Pau Monne wrote:
> > On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote:
> > > I have a gut feeling that returning XEN_ errno to userspace program is
> > > layering violation. They should always be translated to OS level errno
> > > by privcmd driver.
> > 
> > Yes, the error value returned from the hypercall executed is indeed 
> > translated into the native OS error space. The problem here is that those 
> > error codes are returned _inside_ of the specific hypercall struct, which 
> > sadly privcmd doesn't know anything about.
> > 
> > And of course teaching privcmd about every possible hypercall struct is 
> > simply impossible, since some of them are not stable (eg: domctls)
> >  
> > > Aren't FreeBSD and NetBSD already doing that?
> > 
> > As said above, this is only done for direct return codes, everything inside 
> > of the struct passed to the hypercall is returned as-is.
> > 
> > This is a complete mess, and TBH, I don't have a clever idea about how to 
> > solve it.
> > 
> 
> Me neither. Maybe a new thread should be started to discuss this.

So here we are.

In order to put everyone into context: the issue here is that some 
hypercalls (those that batch several operations) return an array of error 
codes inside of the hypercall structure. This array of error codes is not 
standardized, so the privcmd driver doesn't know anything about it, and thus 
cannot translate it into the native OS error space.

It has also been suggested that the privcmd driver simply doesn't translate 
error codes at all, and then let the applications figure out if the error 
code comes from Xen or from the OS. IMHO, this is impossible to achieve, 
because the ioctl syscall can return an error code that's been forwarded 
by Xen or a native one, and the application has no way of knowing where is 
it coming from.

I've identified at least the following hypercall structs that store XEN_* 
error codes inside:

 - xen_add_to_physmap_batch
 - xen_xsplice_status

TBH, it's quite hard to spot them, so I might have missed some. 

xen_add_to_physmap_batch is part of the public ABI, and cannot be changed. 
On the bright side, xen_add_to_physmap_batch is implemented as a different 
ioctl in privcmd usually (in order to map memory from other domains), so the 
error translation should be handled correctly.

Then the xsplice struct that uses XEN_* values is:

struct xen_xsplice_status {
#define XSPLICE_STATE_CHECKED      1
#define XSPLICE_STATE_APPLIED      2
    uint32_t state;                /* OUT: XSPLICE_STATE_*. */
    int32_t rc;                    /* OUT: 0 if no error, otherwise -XEN_EXX. */
};

Which is in turn used by:

struct xen_sysctl_xsplice_list {
    uint32_t version;                       /* OUT: Hypervisor stamps value.
                                               If varies between calls, we are
                                             * getting stale data. */
    uint32_t idx;                           /* IN: Index into hypervisor list. 
*/
    uint32_t nr;                            /* IN: How many status, name, and 
len
                                               should fill out. Can be zero to 
get
                                               amount of payloads and version.
                                               OUT: How many payloads left. */
    uint32_t pad;                           /* IN: Must be zero. */
    XEN_GUEST_HANDLE_64(xen_xsplice_status_t) status;  /* OUT. Must have enough
                                               space allocate for nr of them. */
    XEN_GUEST_HANDLE_64(char) name;         /* OUT: Array of names. Each member
                                               MUST XEN_XSPLICE_NAME_SIZE in 
size.
                                               Must have nr of them. */
    XEN_GUEST_HANDLE_64(uint32) len;        /* OUT: Array of lengths of name's.
                                               Must have nr of them. */
};

IMHO, the best way to solve this is to define a set of XSPLICE_ERROR_* that 
covers the error codes returned by xsplice, and use that instead of XEN_* 
errno values. This would make it much more easier to avoid mistakes when 
coding the toolstack part of xsplice.

Roger.

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