Re: [PATCH v4] tools/foreignmem: Support querying the size of a resource

[Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>]

On Fri, Jan 29, 2021 at 03:09:30PM +0000, Andrew Cooper wrote:
> On 29/01/2021 14:59, Roger Pau Monné wrote:
> > On Fri, Jan 29, 2021 at 11:38:43AM +0100, Manuel Bouyer wrote:
> >> On Thu, Jan 28, 2021 at 12:01:52PM +0000, Andrew Cooper wrote:
> >>> With the Xen side of this interface (soon to be) fixed to return real 
> >>> sizes,
> >>> userspace needs to be able to make the query.
> >>>
> >>> Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> >>> library minor version.
> >>>
> >>> Update both all osdep_xenforeignmemory_map_resource() implementations to
> >>> understand size requests, skip the mmap() operation, and copy back the
> >>> nr_frames field.
> >>>
> >>> For NetBSD, also fix up the ioctl() error path to issue an unmap(), which 
> >>> was
> >>> overlooked by c/s 4a64e2bb39 "libs/foreignmemory: Implement on NetBSD".
> >>> [....]
> >>> diff --git a/tools/libs/foreignmemory/netbsd.c 
> >>> b/tools/libs/foreignmemory/netbsd.c
> >>> index d26566f601..4ae60aafdd 100644
> >>> --- a/tools/libs/foreignmemory/netbsd.c
> >>> +++ b/tools/libs/foreignmemory/netbsd.c
> >>> @@ -132,6 +132,10 @@ int osdep_xenforeignmemory_map_resource(
> >>>      };
> >>>      int rc;
> >>>  
> >>> +    if ( !fres->addr && !fres->nr_frames )
> >>> +        /* Request for resource size.  Skip mmap(). */
> >>> +        goto skip_mmap;
> >>> +
> >>>      fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
> >>>                        fres->prot, fres->flags | MAP_ANON | MAP_SHARED, 
> >>> -1, 0);
> >> What happens if fres->addr is not NULL and nr_frames is 0 ?
> > mmap would return MAP_FAILED and errno == EINVAL in that case AFAICT
> > on Linux and FreeBSD. NetBSD mmap man page doesn't seem to mention
> > what happens in that case, so the comments below apply to Linux and
> > FreeBSD. Maybe we need to handle this differently for NetBSD?
> >
> >> Is it supposed to happen ?
> > I think that's fine. Calling osdep_xenforeignmemory_map_resource with
> > nr_frames == 0 is pointing to a bug in the caller, so returning error
> > should be fine.
> >
> >> Should we assert that fres->addr is NULL when
> >> nr_frames is 0 ? Or force fres->addr to NULL when nr_frames is 0 ?
> > Doesn't really matter, mmap will return EINVAL if nr_frames == 0
> > regardless of the value of addr.
> 
> mmap() of 0 is an unconditional failure.  So sayeth POSIX.
> 
> For the size request, we don't mmap(), and a pointer of 0 is the signal
> to Xen.
> 
> For the regular mapping, we support both NULL (let the kernel choose),
> and non-NULL (I want my mapping here please), just like regular mmap().

OK, so that's no a bug. Thanks

-- 
Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
     NetBSD: 26 ans d'experience feront toujours la difference
--