Re: [PATCH XEN v5 13/23] tools: Refactor foreign memory mapping into libxenforeignmemory

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-11-11 at 15:13 +0000, Ian Jackson wrote:
> > +/*
> > + * Maps a range within one domain to a local address range.  Mappings
> > + * should be unmapped with munmap and should follow the same rules as
> > mmap
> > + * regarding page alignment.
> > + *
> > + * prot is as for mmap(2).
> > + *
> > + * Can partially succeed. When a page cannot be mapped, its respective
> > + * field in @err is set to the corresponding errno value.
> > + *
> > + * Returns NULL if no pages can be mapped.
> ...
> > +void *xenforeignmemory_map(xenforeignmemory_handle *fmem, uint32_t
> > dom,
> > +                           int prot, const xen_pfn_t *arr, int *err,
> > +                           unsigned int num);
> 
> If it returns NULL, will all of *err be filled with errno values ?
> Ie is all of err[] always written ?
> 
> Does this function ever set errno ?

From my reading of the Linux code it appears to be thus:

If it returns NULL it will have set errno. In that case it appears that err
is invalid.

If it returns non-NULL then all of err has been written to either 0 or a
negative errno val, indicating the state of that particular frame.

errno vals are in the OS space, not the Xen one, for both the global errno
and the err[] array vals.

I think the same is true of FreeBSD from my reading of that code.

In both cases the behaviour is a combined property of the behaviours of the
libxc functions, the privcmd driver and the hypercall on xen side, where
the first two differ a fair bit on different platforms (and call different
hypercalls under the hood, with different behaviour there too)

But the sum _seems_ to add up the same in both cases. Mini-OS is simpler
but looks to do about the same.

IOW I'm not 100% sure of the above but it seems a) like a
sensible/desirable enough behaviour and b) it does look, as far as I can
tell, like the code does this.

> This API invites callers to fail to notice partial failures.
> 
> How about permitting err==NULL to mean `on partial failure, unmap
> everything and return NULL setting errno' ?

AFAICT both the FreeBSD and Linux privcmd driver do not handle this case,
so we would be looking at adding functionality to the library to paper over
it.

Looking at the users it seems that those which map batches of pages get
things right, since they naturally tend to be looping over the arrays
anyway.

It's the callers who are mapping a single page which tend to get this
wrong, many of them are pre-existing bugs but a fair few are as a result of
conversions made in this set of series.

In particular xc_map_foreign_pages used to unmap and return NULL with errno
set to the value first non-zero errno from the err array, which I suppose
is what you were thinking of for setting errno in this case? 

I'm considering whether to add a second function in the API to map single
pages (since that seems to be the most common usage, and commonly exhibits
this error) vs doing as you suggest, which has the downside of throwing
away all the other errors (which may be more critical than the first).

I did briefly consider allow err == NULL iff nr == 1, but that seems worse
than both the other options.

Ian.

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