Re: [Qemu-devel] [PATCH] xen-mapcache: use MAP_FIXED flag so the mmap address hint is always honored

[Roger Pau Monné]

On Fri, Mar 15, 2019 at 10:54:42AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne [mailto:address@hidden
> > Sent: 15 March 2019 08:59
> > To: address@hidden
> > Cc: Roger Pau Monne <address@hidden>; Stefano Stabellini <address@hidden>; 
> > Anthony
> > Perard <address@hidden>; Paul Durrant <address@hidden>; Igor Druzhinin
> > <address@hidden>; Paolo Bonzini <address@hidden>; Richard Henderson 
> > <address@hidden>;
> > Eduardo Habkost <address@hidden>; Michael S. Tsirkin <address@hidden>; 
> > Marcel Apfelbaum
> > <address@hidden>; address@hidden
> > Subject: [PATCH] xen-mapcache: use MAP_FIXED flag so the mmap address hint 
> > is always honored
> > 
> > Or if it's not possible to honor the hinted address an error is returned
> > instead. This makes it easier to spot the actual failure, instead of
> > failing later on when the caller of xen_remap_bucket realizes the
> > mapping has not been created at the requested address.
> > 
> > Also note that at least on FreeBSD using MAP_FIXED will cause mmap to
> > try harder to honor the passed address.
> > 
> > Signed-off-by: Roger Pau Monné <address@hidden>
> > ---
> > Cc: Stefano Stabellini <address@hidden>
> > Cc: Anthony Perard <address@hidden>
> > Cc: Paul Durrant <address@hidden>
> > Cc: Igor Druzhinin <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: Richard Henderson <address@hidden>
> > Cc: Eduardo Habkost <address@hidden>
> > Cc: "Michael S. Tsirkin" <address@hidden>
> > Cc: Marcel Apfelbaum <address@hidden>
> > Cc: address@hidden
> > ---
> >  hw/i386/xen/xen-mapcache.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> > index 349f72d00c..0e2870b320 100644
> > --- a/hw/i386/xen/xen-mapcache.c
> > +++ b/hw/i386/xen/xen-mapcache.c
> > @@ -185,8 +185,14 @@ static void xen_remap_bucket(MapCacheEntry *entry,
> >      }
> > 
> >      if (!dummy) {
> > +        /*
> > +         * If the caller has requested the mapping at a specific address 
> > use
> > +         * MAP_FIXED to make sure it's honored. Note that with MAP_FIXED at
> > +         * least FreeBSD will try harder to honor the passed address.
> > +         */
> >          vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid, vaddr,
> > -                                           PROT_READ | PROT_WRITE, 0,
> > +                                           PROT_READ | PROT_WRITE,
> > +                                           vaddr ? MAP_FIXED : 0,
> >                                             nb_pfn, pfns, err);
> 
> AFAICT xen_remap_bucket() is always called with a NULL vaddr argument if 
> entry->vaddr_base == NULL, and called with vaddr == entry->vaddr_base in the 
> other case, so I'd say the vaddr argument is superfluous.
> So, I think it would be better to just use entry->vaddr_base in the call 
> above and I also wonder whether the mmap() below should only be allowed to 
> happen if entry->vaddr_base == NULL.

The suggested changes above look reasonable to me (note I'm not
familiar with qemu Xen mapcache), however they should be separate
patch(es) IMO, since they are not directly related to the MAP_FIXED
issue this patch attempts to fix.

I would prefer to keep this patch as-is if possible, so that it's
easier and safer for me to backport in order to fix qemu in Xen 4.11
for FreeBSD.

Thanks, Roger.