[Qemu-devel] [PATCH] possible mmap regression

[Qemu-devel] [PATCH] possible mmap regression

[Felipe Contreras]

[-- Attachment #1: Type: text/plain, Size: 240 bytes --]

Hi,

I don't know what I'm doing but this seems to fix the weird issue I was having.
http://article.gmane.org/gmane.comp.emulators.qemu/23314

I've found out that this happens on linux 2.6.23, but not 2.6.24.

Cheers.

-- 
Felipe Contreras

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: qemu-mmap-fix.diff --]
[-- Type: text/x-patch; name=qemu-mmap-fix.diff, Size: 608 bytes --]

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 6292826..3050ad9 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -251,7 +251,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
            especially important if qemu_host_page_size >
            qemu_real_host_page_size */
         p = mmap(g2h(mmap_start),
-                 host_len, prot, flags | MAP_FIXED, fd, host_offset);
+                 host_len, prot, flags, fd, host_offset);
         if (p == MAP_FAILED)
             return -1;
         /* update start so that it points to the file position at 'offset' */

Re: [Qemu-devel] [PATCH] possible mmap regression

[Edgar E. Iglesias]

On Tue, Feb 12, 2008 at 09:42:15PM +0200, Felipe Contreras wrote:
> Hi,
> 
> I don't know what I'm doing but this seems to fix the weird issue I was having.
> http://article.gmane.org/gmane.comp.emulators.qemu/23314
> 
> I've found out that this happens on linux 2.6.23, but not 2.6.24.
> 
> Cheers.
> 
> -- 
> Felipe Contreras
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 6292826..3050ad9 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -251,7 +251,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>             especially important if qemu_host_page_size >
>             qemu_real_host_page_size */
>          p = mmap(g2h(mmap_start),
> -                 host_len, prot, flags | MAP_FIXED, fd, host_offset);
> +                 host_len, prot, flags, fd, host_offset);
>          if (p == MAP_FAILED)
>              return -1;
>          /* update start so that it points to the file position at 'offset' */

Hello,

Sorry, but I beleive your patch will break simulations where the targets pagesize is larger than the hosts.

Would you mind trying the attach patched and let us know if it helps for you?
If not, it would be great if you could provide a small test case that trigs the bug you are seeing so we can debug the problem.

Best regards
-- 
Edgar E. Iglesias
Axis Communications AB

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 6292826..78a8162 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -385,6 +385,9 @@ int target_munmap(abi_ulong start, abi_ulong len)
             real_end -= qemu_host_page_size;
     }
 
+    if (start < mmap_next_start)
+    	mmap_next_start = start;
+
     /* unmap what we can */
     if (real_start < real_end) {
         ret = munmap(g2h(real_start), real_end - real_start);

Re: [Qemu-devel] [PATCH] possible mmap regression

[Felipe Contreras]

Hi Edgar,

On Feb 16, 2008 11:07 PM, Edgar E. Iglesias <edgar.iglesias@axis.com> wrote:
>
> On Tue, Feb 12, 2008 at 09:42:15PM +0200, Felipe Contreras wrote:
> > Hi,
> >
> > I don't know what I'm doing but this seems to fix the weird issue I was having.
> > http://article.gmane.org/gmane.comp.emulators.qemu/23314
> >
> > I've found out that this happens on linux 2.6.23, but not 2.6.24.
> >
> > Cheers.
> >
> > --
> > Felipe Contreras
> > diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> > index 6292826..3050ad9 100644
> > --- a/linux-user/mmap.c
> > +++ b/linux-user/mmap.c
> > @@ -251,7 +251,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
> >             especially important if qemu_host_page_size >
> >             qemu_real_host_page_size */
> >          p = mmap(g2h(mmap_start),
> > -                 host_len, prot, flags | MAP_FIXED, fd, host_offset);
> > +                 host_len, prot, flags, fd, host_offset);
> >          if (p == MAP_FAILED)
> >              return -1;
> >          /* update start so that it points to the file position at 'offset' */
>
> Hello,
>
> Sorry, but I beleive your patch will break simulations where the targets pagesize is larger than the hosts.
>
> Would you mind trying the attach patched and let us know if it helps for you?
> If not, it would be great if you could provide a small test case that trigs the bug you are seeing so we can debug the problem.
>
> Best regards
> --
> Edgar E. Iglesias
> Axis Communications AB
>
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 6292826..78a8162 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -385,6 +385,9 @@ int target_munmap(abi_ulong start, abi_ulong len)
>              real_end -= qemu_host_page_size;
>      }
>
> +    if (start < mmap_next_start)
> +       mmap_next_start = start;
> +
>      /* unmap what we can */
>      if (real_start < real_end) {
>          ret = munmap(g2h(real_start), real_end - real_start);
>

I tried your patch and it still crashes.

I sent the details before:
http://article.gmane.org/gmane.comp.emulators.qemu/23314
http://article.gmane.org/gmane.comp.emulators.qemu/23328

Basically it was triggered by this change:
http://repo.or.cz/w/qemu.git?a=commitdiff;h=edbcc0b2eb1d4caee5f293e5c79f81023f3394e2

And it happens with some recursive Makefiles stuff.

Best regards.

-- 
Felipe Contreras

Re: [Qemu-devel] [PATCH] possible mmap regression

[Edgar E. Iglesias]

On Wed, Feb 20, 2008 at 03:03:39PM +0200, Felipe Contreras wrote:
> Hi Edgar,
> 
> On Feb 16, 2008 11:07 PM, Edgar E. Iglesias <edgar.iglesias@axis.com> wrote:
> >
> > On Tue, Feb 12, 2008 at 09:42:15PM +0200, Felipe Contreras wrote:
> > > Hi,
> > >
> > > I don't know what I'm doing but this seems to fix the weird issue I was having.
> > > http://article.gmane.org/gmane.comp.emulators.qemu/23314
> > >
> > > I've found out that this happens on linux 2.6.23, but not 2.6.24.
> > >
> > > Cheers.
> > >
> > > --
> > > Felipe Contreras
> > > diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> > > index 6292826..3050ad9 100644
> > > --- a/linux-user/mmap.c
> > > +++ b/linux-user/mmap.c
> > > @@ -251,7 +251,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
> > >             especially important if qemu_host_page_size >
> > >             qemu_real_host_page_size */
> > >          p = mmap(g2h(mmap_start),
> > > -                 host_len, prot, flags | MAP_FIXED, fd, host_offset);
> > > +                 host_len, prot, flags, fd, host_offset);
> > >          if (p == MAP_FAILED)
> > >              return -1;
> > >          /* update start so that it points to the file position at 'offset' */
> >
> > Hello,
> >
> > Sorry, but I beleive your patch will break simulations where the targets pagesize is larger than the hosts.
> >
> > Would you mind trying the attach patched and let us know if it helps for you?
> > If not, it would be great if you could provide a small test case that trigs the bug you are seeing so we can debug the problem.
> >
> > Best regards
> > --
> > Edgar E. Iglesias
> > Axis Communications AB
> >
> > diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> > index 6292826..78a8162 100644
> > --- a/linux-user/mmap.c
> > +++ b/linux-user/mmap.c
> > @@ -385,6 +385,9 @@ int target_munmap(abi_ulong start, abi_ulong len)
> >              real_end -= qemu_host_page_size;
> >      }
> >
> > +    if (start < mmap_next_start)
> > +       mmap_next_start = start;
> > +
> >      /* unmap what we can */
> >      if (real_start < real_end) {
> >          ret = munmap(g2h(real_start), real_end - real_start);
> >
> 
> I tried your patch and it still crashes.
> 
> I sent the details before:
> http://article.gmane.org/gmane.comp.emulators.qemu/23314
> http://article.gmane.org/gmane.comp.emulators.qemu/23328
> 
> Basically it was triggered by this change:
> http://repo.or.cz/w/qemu.git?a=commitdiff;h=edbcc0b2eb1d4caee5f293e5c79f81023f3394e2
> 
> And it happens with some recursive Makefiles stuff.

Thanks Felipe,

I was also seeing errors with that commit. Later that same evening I found a few more errors with the mmap code which tried to fix. Would you mind trying that patch too?

You can find it here:
http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00331.html

Best regards
- 
Edgar E. Iglesias
Axis Communications AB

Re: [Qemu-devel] [PATCH] possible mmap regression

[Felipe Contreras]

On Feb 20, 2008 3:13 PM, Edgar E. Iglesias <edgar.iglesias@axis.com> wrote:
>
> On Wed, Feb 20, 2008 at 03:03:39PM +0200, Felipe Contreras wrote:
> > Hi Edgar,
> >
> > On Feb 16, 2008 11:07 PM, Edgar E. Iglesias <edgar.iglesias@axis.com> wrote:
> > >
> > > On Tue, Feb 12, 2008 at 09:42:15PM +0200, Felipe Contreras wrote:
> > > > Hi,
> > > >
> > > > I don't know what I'm doing but this seems to fix the weird issue I was having.
> > > > http://article.gmane.org/gmane.comp.emulators.qemu/23314
> > > >
> > > > I've found out that this happens on linux 2.6.23, but not 2.6.24.
> > > >
> > > > Cheers.
> > > >
> > > > --
> > > > Felipe Contreras
> > > > diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> > > > index 6292826..3050ad9 100644
> > > > --- a/linux-user/mmap.c
> > > > +++ b/linux-user/mmap.c
> > > > @@ -251,7 +251,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
> > > >             especially important if qemu_host_page_size >
> > > >             qemu_real_host_page_size */
> > > >          p = mmap(g2h(mmap_start),
> > > > -                 host_len, prot, flags | MAP_FIXED, fd, host_offset);
> > > > +                 host_len, prot, flags, fd, host_offset);
> > > >          if (p == MAP_FAILED)
> > > >              return -1;
> > > >          /* update start so that it points to the file position at 'offset' */
> > >
> > > Hello,
> > >
> > > Sorry, but I beleive your patch will break simulations where the targets pagesize is larger than the hosts.
> > >
> > > Would you mind trying the attach patched and let us know if it helps for you?
> > > If not, it would be great if you could provide a small test case that trigs the bug you are seeing so we can debug the problem.
> > >
> > > Best regards
> > > --
> > > Edgar E. Iglesias
> > > Axis Communications AB
> > >
> > > diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> > > index 6292826..78a8162 100644
> > > --- a/linux-user/mmap.c
> > > +++ b/linux-user/mmap.c
> > > @@ -385,6 +385,9 @@ int target_munmap(abi_ulong start, abi_ulong len)
> > >              real_end -= qemu_host_page_size;
> > >      }
> > >
> > > +    if (start < mmap_next_start)
> > > +       mmap_next_start = start;
> > > +
> > >      /* unmap what we can */
> > >      if (real_start < real_end) {
> > >          ret = munmap(g2h(real_start), real_end - real_start);
> > >
> >
> > I tried your patch and it still crashes.
> >
> > I sent the details before:
> > http://article.gmane.org/gmane.comp.emulators.qemu/23314
> > http://article.gmane.org/gmane.comp.emulators.qemu/23328
> >
> > Basically it was triggered by this change:
> > http://repo.or.cz/w/qemu.git?a=commitdiff;h=edbcc0b2eb1d4caee5f293e5c79f81023f3394e2
> >
> > And it happens with some recursive Makefiles stuff.
>
> Thanks Felipe,
>
> I was also seeing errors with that commit. Later that same evening I found a few more errors with the mmap code which tried to fix. Would you mind trying that patch too?
>
> You can find it here:
> http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00331.html

Good to know I'm not the only one :)

I tried your patch, I still get the crash.

Best regards.

-- 
Felipe Contreras