diff mbox series

xen-mapcache: use MAP_FIXED flag so the mmap address hint is always honored

Message ID 20190315085847.31245-1-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series xen-mapcache: use MAP_FIXED flag so the mmap address hint is always honored | expand

Commit Message

Roger Pau Monné March 15, 2019, 8:58 a.m. UTC
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(-)

Comments

Paul Durrant March 15, 2019, 9:54 a.m. UTC | #1
> -----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)
Roger Pau Monné March 15, 2019, 10:09 a.m. UTC | #2
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.
Paul Durrant March 15, 2019, 10:12 a.m. UTC | #3
> -----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.
Anthony PERARD March 15, 2019, 6:14 p.m. UTC | #4
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,
Anthony PERARD March 15, 2019, 6:27 p.m. UTC | #5
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
Anthony PERARD March 15, 2019, 6:38 p.m. UTC | #6
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);
Igor Druzhinin March 15, 2019, 8:24 p.m. UTC | #7
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 mbox series

Patch

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");