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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 15 March 2019 10:10
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: qemu-devel@xxxxxxxxxx; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
> Anthony Perard
> <anthony.perard@xxxxxxxxxx>; Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>; 
> Paolo Bonzini
> <pbonzini@xxxxxxxxxx>; Richard Henderson <rth@xxxxxxxxxxx>; Eduardo Habkost 
> <ehabkost@xxxxxxxxxx>;
> Michael S. Tsirkin <mst@xxxxxxxxxx>; Marcel Apfelbaum 
> <marcel.apfelbaum@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] xen-mapcache: use MAP_FIXED flag so the mmap address 
> hint is always honored
> 
> On Fri, Mar 15, 2019 at 10:54:42AM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> > > Sent: 15 March 2019 08:59
> > > To: qemu-devel@xxxxxxxxxx
> > > Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>; Stefano Stabellini 
> > > <sstabellini@xxxxxxxxxx>; Anthony
> > > Perard <anthony.perard@xxxxxxxxxx>; Paul Durrant 
> > > <Paul.Durrant@xxxxxxxxxx>; Igor Druzhinin
> > > <igor.druzhinin@xxxxxxxxxx>; Paolo Bonzini <pbonzini@xxxxxxxxxx>; Richard 
> > > Henderson
> <rth@xxxxxxxxxxx>;
> > > Eduardo Habkost <ehabkost@xxxxxxxxxx>; Michael S. Tsirkin 
> > > <mst@xxxxxxxxxx>; Marcel Apfelbaum
> > > <marcel.apfelbaum@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> > > 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é <roger.pau@xxxxxxxxxx>
> > > ---
> > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>
> > > Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > Cc: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> > > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > Cc: Richard Henderson <rth@xxxxxxxxxxx>
> > > Cc: Eduardo Habkost <ehabkost@xxxxxxxxxx>
> > > Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> > > Cc: Marcel Apfelbaum <marcel.apfelbaum@xxxxxxxxx>
> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > > ---
> > >  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.

I think it does indeed DTRT as-is. Let's see what Anthony says.

  Paul

> 
> Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel