| Message ID | 20190315085847.31245-1-roger.pau@citrix.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen-mapcache: use MAP_FIXED flag so the mmap address hint is always honored | expand |
> -----Original Message----- > From: Roger Pau Monne [mailto:roger.pau@citrix.com] > Sent: 15 March 2019 08:59 > To: qemu-devel@nongnu.org > Cc: Roger Pau Monne <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony > Perard <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Igor Druzhinin > <igor.druzhinin@citrix.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; > Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum > <marcel.apfelbaum@gmail.com>; xen-devel@lists.xenproject.org > 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@citrix.com> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Anthony Perard <anthony.perard@citrix.com> > Cc: Paul Durrant <paul.durrant@citrix.com> > Cc: Igor Druzhinin <igor.druzhinin@citrix.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Cc: xen-devel@lists.xenproject.org > --- > 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. Paul > if (vaddr_base == NULL) { > perror("xenforeignmemory_map2"); > -- > 2.17.2 (Apple Git-113)
On Fri, Mar 15, 2019 at 10:54:42AM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monne [mailto:roger.pau@citrix.com] > > Sent: 15 March 2019 08:59 > > To: qemu-devel@nongnu.org > > Cc: Roger Pau Monne <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony > > Perard <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Igor Druzhinin > > <igor.druzhinin@citrix.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; > > Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum > > <marcel.apfelbaum@gmail.com>; xen-devel@lists.xenproject.org > > 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@citrix.com> > > --- > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: Anthony Perard <anthony.perard@citrix.com> > > Cc: Paul Durrant <paul.durrant@citrix.com> > > Cc: Igor Druzhinin <igor.druzhinin@citrix.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Richard Henderson <rth@twiddle.net> > > Cc: Eduardo Habkost <ehabkost@redhat.com> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > > Cc: xen-devel@lists.xenproject.org > > --- > > 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.
> -----Original Message----- > From: Roger Pau Monne > Sent: 15 March 2019 10:10 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: qemu-devel@nongnu.org; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard > <anthony.perard@citrix.com>; Igor Druzhinin <igor.druzhinin@citrix.com>; Paolo Bonzini > <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>; > Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>; xen- > devel@lists.xenproject.org > 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@citrix.com] > > > Sent: 15 March 2019 08:59 > > > To: qemu-devel@nongnu.org > > > Cc: Roger Pau Monne <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony > > > Perard <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Igor Druzhinin > > > <igor.druzhinin@citrix.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson > <rth@twiddle.net>; > > > Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum > > > <marcel.apfelbaum@gmail.com>; xen-devel@lists.xenproject.org > > > 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@citrix.com> > > > --- > > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > > Cc: Anthony Perard <anthony.perard@citrix.com> > > > Cc: Paul Durrant <paul.durrant@citrix.com> > > > Cc: Igor Druzhinin <igor.druzhinin@citrix.com> > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Cc: Richard Henderson <rth@twiddle.net> > > > Cc: Eduardo Habkost <ehabkost@redhat.com> > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > > > Cc: xen-devel@lists.xenproject.org > > > --- > > > 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.
On Fri, Mar 15, 2019 at 09:58:47AM +0100, Roger Pau Monne wrote: > 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@citrix.com> The patch looks fine, and MAP_FIXED seems to be the expected behavior even on Linux. Acked-by: Anthony PERARD <anthony.perard@citrix.com> > diff --git 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. > + */ I wonder if the note about FreeBSD is actually useful here. Even Linux may map at a different address without MAP_FIXED, if I read the man page correctly. Do you mind if it is removed? > 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); Thanks,
On Fri, Mar 15, 2019 at 09:54:42AM +0000, Paul Durrant wrote: > 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. I don't think that's true. The call at line 312 [1] may be called with vaddr_base != NULL. Then, xen_remap_bucket will unmap that entry before replacing it. We could maybe figure out if vaddr_base needs to be replaced in-place based on the flags XEN_MAPCACHE_ENTRY_DUMMY, but that seems more convoluted than the current approche. [1] https://git.qemu.org/?p=qemu.git;a=blob;f=hw/i386/xen/xen-mapcache.c;h=349f72d00cc2c9fc134df8cff7dd051b1fc2fa41;hb=HEAD#l312
On Fri, Mar 15, 2019 at 06:14:09PM +0000, Anthony PERARD wrote: > On Fri, Mar 15, 2019 at 09:58:47AM +0100, Roger Pau Monne wrote: > > 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@citrix.com> > > The patch looks fine, and MAP_FIXED seems to be the expected behavior > even on Linux. > > Acked-by: Anthony PERARD <anthony.perard@citrix.com> > > > diff --git 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); I've noticed that there's a mmap call just after which has also vaddr as hint, should that second call also have MAP_FIXED as flags? On the other hand, it seems that when vaddr!=NULL, dummy==false, so that second mmap call will never be called with vaddr!=NULL. But it might be worse fixing this in the same patch. > } else { > /* > * We create dummy mappings where we are unable to create a foreign > * mapping immediately due to certain circumstances (i.e. on resume now) > */ > vaddr_base = mmap(vaddr, size, PROT_READ | PROT_WRITE, > MAP_ANON | MAP_SHARED, -1, 0);
On 15/03/2019 18:38, Anthony PERARD wrote: > On Fri, Mar 15, 2019 at 06:14:09PM +0000, Anthony PERARD wrote: >> On Fri, Mar 15, 2019 at 09:58:47AM +0100, Roger Pau Monne wrote: >>> 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@citrix.com> >> >> The patch looks fine, and MAP_FIXED seems to be the expected behavior >> even on Linux. >> >> Acked-by: Anthony PERARD <anthony.perard@citrix.com> >> >>> diff --git 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); > > I've noticed that there's a mmap call just after which has also vaddr as > hint, should that second call also have MAP_FIXED as flags? I think so, yes. The intended usage of xen_remap_bucket() is to create a mapcache entry - either dummy or real. So this patch fixes the present problem for real entry now but not for dummy. In future, there might be xen_remap_bucket() calls to create dummy entries at a predefined location so, I think, MAP_FIXED should be passed to mmap() call as well. Igor
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); if (vaddr_base == NULL) { perror("xenforeignmemory_map2");
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@citrix.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Anthony Perard <anthony.perard@citrix.com> Cc: Paul Durrant <paul.durrant@citrix.com> Cc: Igor Druzhinin <igor.druzhinin@citrix.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Cc: xen-devel@lists.xenproject.org --- hw/i386/xen/xen-mapcache.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)