Re: [PATCH XEN v3 14/22] tools: foreignmemory: provide xenforeignmemory_unmap.

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

On Wed, 2015-10-07 at 16:03 +0100, Andrew Cooper wrote:
> On 07/10/15 15:15, Ian Campbell wrote:
> 
> What is the plan WRT fixing the existing APIs?

Which ones do you mean? The xc_map_foreign_* which aren't moved here?

My intention is to deprecate them and switch everything over to the API
provided by this library. IOW xenforeignmemory_map() will be the sole (at
least ABI stable way) way to map foreign memory.

I don't want to go replicate the "there's ten ways to do it" aspect of the
current interfaces in this new library.

I think the selected interface (which matches the old _bulk one) provides
sufficient flexibility to be usable going forward. Evidence of this is that
all the old libxc interfaces are now implemented in terms of it.

> The two options are 1) Move to new libs then fix, or 2) Fix in tree then
> move to pristine new libs.
> 
> Option 1) runs the risk of digging ourself a hole in the form of a
> stable API.  Option 2) seems like it would be clearer to review.

I suppose I'm proposing option 3) port everything to this new API when we
like and eventually remove these functions.

diff --git a/tools/libs/foreignmemory/linux.c
> > b/tools/libs/foreignmemory/linux.c
> > index 01cd42e..86a5a97 100644
> > --- a/tools/libs/foreignmemory/linux.c
> > +++ b/tools/libs/foreignmemory/linux.c
> > @@ -276,6 +276,12 @@ void *xenforeignmemory_map(xenforeignmemory_handle
> > *fmem,
> >      return addr;
> >  }
> >  
> > +int xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
> > +                           void *addr, unsigned int num)
> > +{
> > +    return munmap(addr, (unsigned long)num << PAGE_SHIFT);
> 
> This cast indicates that unsigned int is not appropriate in the API.
> 
> It matches the map() side, but severely risks truncating the unmap()
> because of the internal multiplication by 4096.  Both the map and unmap
> side should use "size_t num" rather than unsigned int.

Agreed.

> > diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
> > index 2e41af8..1a0d76a 100644
> > --- a/tools/libxc/xc_sr_restore.c
> > +++ b/tools/libxc/xc_sr_restore.c
> > @@ -378,7 +378,7 @@ static int process_page_data(struct xc_sr_context
> > *ctx, unsigned count,
> >  
> >   err:
> >      if ( mapping )
> > -        munmap(mapping, nr_pages * PAGE_SIZE);
> > +        xenforeignmemory_unmap(xch->fmem, mapping, nr_pages *
> > PAGE_SIZE);
> 
> The semantics are different between munmap() and
> xenforeignmemory_unmap(), which means you need to drop the * PAGE_SIZE 
> in each of the replacements.

Oops, yes indeed.

Ian.