[Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[chengang]

From: Chen Gang <chengang@emindsoft.com.cn>

mmap() size in mmap_frag() is qemu_host_page_size, but the outside calls
page_set_flags() may be not with qemu_host_page_size. So after mmap(),
call page_set_flags() in time.

After this fix,  for the next call for the same region, prot1 will be
PAGE_VALID (not 0), so can avoid to enter "if (prot1 == 0)" case, again.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 linux-user/mmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 445e8c6..7807ed0 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -162,6 +162,8 @@ static int mmap_frag(abi_ulong real_start,
                        flags | MAP_ANONYMOUS, -1, 0);
         if (p == MAP_FAILED)
             return -1;
+        page_set_flags(real_start, real_start + qemu_host_page_size,
+                       PAGE_VALID);
         prot1 = prot;
     }
     prot1 &= PAGE_BITS;
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/3] linux-user/mmap.c: Remove useless variable p for mmap_frag

[chengang]

From: Chen Gang <chengang@emindsoft.com.cn>

It is useless.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 linux-user/mmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 7807ed0..51c381d 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -158,10 +158,10 @@ static int mmap_frag(abi_ulong real_start,
 
     if (prot1 == 0) {
         /* no page was there, so we allocate one */
-        void *p = mmap(host_start, qemu_host_page_size, prot,
-                       flags | MAP_ANONYMOUS, -1, 0);
-        if (p == MAP_FAILED)
+        if (mmap(host_start, qemu_host_page_size, prot, flags | MAP_ANONYMOUS,
+                 -1, 0) == MAP_FAILED) {
             return -1;
+        }
         page_set_flags(real_start, real_start + qemu_host_page_size,
                        PAGE_VALID);
         prot1 = prot;
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/3] linux-user/mmap.c: Use TARGET_PAGE_SIZE as the increasing step

[chengang]

From: Chen Gang <chengang@emindsoft.com.cn>

Just like another areas have done.
---
 linux-user/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 51c381d..86c270b 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -151,7 +151,7 @@ static int mmap_frag(abi_ulong real_start,
 
     /* get the protection of the target pages outside the mapping */
     prot1 = 0;
-    for(addr = real_start; addr < real_end; addr++) {
+    for (addr = real_start; addr < real_end; addr += TARGET_PAGE_SIZE) {
         if (addr < start || addr >= end)
             prot1 |= page_get_flags(addr);
     }
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Peter Maydell]

On 11 January 2016 at 09:01,  <chengang@emindsoft.com.cn> wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
>
> mmap() size in mmap_frag() is qemu_host_page_size, but the outside calls
> page_set_flags() may be not with qemu_host_page_size. So after mmap(),
> call page_set_flags() in time.
>
> After this fix,  for the next call for the same region, prot1 will be
> PAGE_VALID (not 0), so can avoid to enter "if (prot1 == 0)" case, again.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  linux-user/mmap.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 445e8c6..7807ed0 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -162,6 +162,8 @@ static int mmap_frag(abi_ulong real_start,
>                         flags | MAP_ANONYMOUS, -1, 0);
>          if (p == MAP_FAILED)
>              return -1;
> +        page_set_flags(real_start, real_start + qemu_host_page_size,
> +                       PAGE_VALID);
>          prot1 = prot;
>      }
>      prot1 &= PAGE_BITS;

I'm confused about this change, because as far as I can see
page_set_flags/page_get_flags work on guest pages, not host pages.
So setting the flags for the whole of the host page to PAGE_VALID
doesn't seem right -- the other guest pages within this host page
are not valid. And the VALID bit should be set for the guest page
that is within the mapping as part of the call to page_set_flags()
at the bottom of target_mmap().

It would help if you could explain what the failure case is that
you're trying to fix, and how the current code fails.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Chen Gang]

Firstly, thank you very much to still focus on this patch, which will
be much helpful for my current work! 

And I modified some code, but did not send patches to upstream, e.g.
sw_64 related code, use TARGET_PAGE_SIZE instead of HOST_PAGE_SIZE ...).

  I skipped MAP_SHARED with PROT_WRITE check to skip some another issues,
  (since I guess for running wine, it should be OK). maybe it be related
  with this issue?

  diff --git a/linux-user/mmap.c b/linux-user/mmap.c
  index 86c270b..a76450a 100644
  --- a/linux-user/mmap.c
  +++ b/linux-user/mmap.c
  @@ -170,12 +170,13 @@ static int mmap_frag(abi_ulong real_start,
   
       prot_new = prot | prot1;
       if (!(flags & MAP_ANONYMOUS)) {
  +#if 0
           /* msync() won't work here, so we return an error if write is
              possible while it is a shared mapping */
           if ((flags & MAP_TYPE) == MAP_SHARED &&
               (prot & PROT_WRITE))
               return -1;
  -
  +#endif
           /* adjust protection to be able to read */
           if (!(prot1 & PROT_WRITE))
               mprotect(host_start, qemu_host_page_size, prot1 | PROT_WRITE);
  

And the other related reply for this thread is at the bottom of this
mail, please check.

On 2016年01月25日 23:07, Peter Maydell wrote:
> On 11 January 2016 at 09:01,  <chengang@emindsoft.com.cn> wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> mmap() size in mmap_frag() is qemu_host_page_size, but the outside calls
>> page_set_flags() may be not with qemu_host_page_size. So after mmap(),
>> call page_set_flags() in time.
>>
>> After this fix,  for the next call for the same region, prot1 will be
>> PAGE_VALID (not 0), so can avoid to enter "if (prot1 == 0)" case, again.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  linux-user/mmap.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>> index 445e8c6..7807ed0 100644
>> --- a/linux-user/mmap.c
>> +++ b/linux-user/mmap.c
>> @@ -162,6 +162,8 @@ static int mmap_frag(abi_ulong real_start,
>>                         flags | MAP_ANONYMOUS, -1, 0);
>>          if (p == MAP_FAILED)
>>              return -1;
>> +        page_set_flags(real_start, real_start + qemu_host_page_size,
>> +                       PAGE_VALID);
>>          prot1 = prot;
>>      }
>>      prot1 &= PAGE_BITS;
> 
> I'm confused about this change, because as far as I can see
> page_set_flags/page_get_flags work on guest pages, not host pages.
> So setting the flags for the whole of the host page to PAGE_VALID
> doesn't seem right -- the other guest pages within this host page
> are not valid. And the VALID bit should be set for the guest page
> that is within the mapping as part of the call to page_set_flags()
> at the bottom of target_mmap().
> 

The related comments for  "if (prot1 == 0)" code block is "no page was
there, so we allocate one".

So I guess this code block is not only allocate page for guest, but also
for host. So prot1 is not only for the guest page, but also for host
page.

If we do not page_set_flags with PAGE_VALID, The next call in mmap_frag
for the same area will let prot1 be 0, so still fall into "if (prot1 ==
0)" code block.

> It would help if you could explain what the failure case is that
> you're trying to fix, and how the current code fails.
> 

I shall give a full test again, and provide the test result later.

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Peter Maydell]

On 26 January 2016 at 02:58, Chen Gang <chengang@emindsoft.com.cn> wrote:
> The related comments for  "if (prot1 == 0)" code block is "no page was
> there, so we allocate one".
>
> So I guess this code block is not only allocate page for guest, but also
> for host. So prot1 is not only for the guest page, but also for host
> page.

The comment means specifically "allocate a host page".

> If we do not page_set_flags with PAGE_VALID, The next call
> in mmap_frag for the same area will let prot1 be 0, so still
> fall into "if (prot1 == 0)" code block.

But in what case will we call mmap_frag() again before we
call page_set_flags() at the bottom of target_mmap()?
That is what is not clear to me, and why I asked you to describe
what the case is that you're seeing problems with.

Reading the target_mmap() code, its intention seems to be:
 (a) if the whole allocation fits in one host page, call
     mmap_frag() once and then "goto the_end1"
 (b) otherwise, we'll call mmap_frag() once for the start
     of the guest mapping, and once for the end, which must
     be two different host pages

So if you're seeing mmap_frag() called twice for the same
host page then something is going wrong, but I'm not sure what.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Chen Gang]

On 2016年01月26日 17:11, Peter Maydell wrote:
> On 26 January 2016 at 02:58, Chen Gang <chengang@emindsoft.com.cn> wrote:
>> The related comments for  "if (prot1 == 0)" code block is "no page was
>> there, so we allocate one".
>>
>> So I guess this code block is not only allocate page for guest, but also
>> for host. So prot1 is not only for the guest page, but also for host
>> page.
> 
> The comment means specifically "allocate a host page".
> 

OK, thanks.

>> If we do not page_set_flags with PAGE_VALID, The next call
>> in mmap_frag for the same area will let prot1 be 0, so still
>> fall into "if (prot1 == 0)" code block.
> 
> But in what case will we call mmap_frag() again before we
> call page_set_flags() at the bottom of target_mmap()?
> That is what is not clear to me, and why I asked you to describe
> what the case is that you're seeing problems with.
> 

When I run WeChat.exe with i386 wine with qemu-i386 under sw_64 arch.

 - The related command:

   "./i386-linux-user/qemu-i386 -strace -L /upstream/i386_wine /upstream/i386_wine/usr/local/bin/wine "C:\\Program Files\\Tencent\\WeChat\\WeChat.exe" > ana/try/info-strace.log 2>&1"

 - The related output (no any munmap, 135168 = 128KB + 4KB):

   4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000
   4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000
   4600 rt_sigprocmask(SIG_SETMASK,0x0033f574,NULL) = 0
   4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f5d0) = 0
   4600 write(3,0x33f6cc,64) = 64
   4600 read(4,0x33f6cc,64) = 1
   4600 rt_sigprocmask(SIG_SETMASK,0x0033f5d0,NULL) = 0
   4600 close(8) = 0
   4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f674) = 0
   4600 mprotect(0x00160000,65536,PROT_READ|PROT_WRITE) = 0
   4600 rt_sigprocmask(SIG_SETMASK,0x0033f674,NULL) = 0
   4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f990) = 0
   4600 mmap2(0x00340000,135168,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0) = 0x00340000

wine often does like above, map the same position multiple times.

> Reading the target_mmap() code, its intention seems to be:
>  (a) if the whole allocation fits in one host page, call
>      mmap_frag() once and then "goto the_end1"

Also yes to me.

>  (b) otherwise, we'll call mmap_frag() once for the start
>      of the guest mapping, and once for the end, which must
>      be two different host pages
> 

Also yes to me.

> So if you're seeing mmap_frag() called twice for the same
> host page then something is going wrong, but I'm not sure what.
> 

For the case I provide above, it can call mmap_frag() twice for the same
host page.

By the way, after have a full test again, all related issues are OK, it
seems we need not this patch to fix current issues, it is really very
strange to me!(maybe it is fixed by my other patches? I don't know)

At present, our sw_64 qemu-i386 status:

 - Can run notepad.exe correctly, can run acdsee5.0.exe setup program
   successfully.

 - The performance is acceptable, after optimize the wine code (simply
   use 32 split times instead of 2 for reserve_area recursion), the
   initialization speed is really quick enough. :-)

 - When run WeChat.exe, it can popup connection GUI box, but will quit
   under sw_64. But for x86_64 qemu-i386, it can run WeChat.exe
   correctly (although after enter main gui, it is not stable enough).


Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Peter Maydell]

On 26 January 2016 at 10:19, Chen Gang <chengang@emindsoft.com.cn> wrote:
> When I run WeChat.exe with i386 wine with qemu-i386 under sw_64 arch.
>
>  - The related command:
>
>    "./i386-linux-user/qemu-i386 -strace -L /upstream/i386_wine /upstream/i386_wine/usr/local/bin/wine "C:\\Program Files\\Tencent\\WeChat\\WeChat.exe" > ana/try/info-strace.log 2>&1"
>
>  - The related output (no any munmap, 135168 = 128KB + 4KB):
>
>    4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000
>    4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000
>    4600 rt_sigprocmask(SIG_SETMASK,0x0033f574,NULL) = 0
>    4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f5d0) = 0
>    4600 write(3,0x33f6cc,64) = 64
>    4600 read(4,0x33f6cc,64) = 1
>    4600 rt_sigprocmask(SIG_SETMASK,0x0033f5d0,NULL) = 0
>    4600 close(8) = 0
>    4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f674) = 0
>    4600 mprotect(0x00160000,65536,PROT_READ|PROT_WRITE) = 0
>    4600 rt_sigprocmask(SIG_SETMASK,0x0033f674,NULL) = 0
>    4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f990) = 0
>    4600 mmap2(0x00340000,135168,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0) = 0x00340000
>
> wine often does like above, map the same position multiple times.

That output seems to show all the mmap calls working fine, though.

>> Reading the target_mmap() code, its intention seems to be:
>>  (a) if the whole allocation fits in one host page, call
>>      mmap_frag() once and then "goto the_end1"
>
> Also yes to me.
>
>>  (b) otherwise, we'll call mmap_frag() once for the start
>>      of the guest mapping, and once for the end, which must
>>      be two different host pages
>>
>
> Also yes to me.
>
>> So if you're seeing mmap_frag() called twice for the same
>> host page then something is going wrong, but I'm not sure what.
>>
>
> For the case I provide above, it can call mmap_frag() twice for the same
> host page.

For the same single call to target_mmap() ? What is the code flow
within QEMU that causes this?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Chen Gang]

On 2016年01月26日 18:26, Peter Maydell wrote:
> On 26 January 2016 at 10:19, Chen Gang <chengang@emindsoft.com.cn> wrote:
>> When I run WeChat.exe with i386 wine with qemu-i386 under sw_64 arch.
>>
>>  - The related command:
>>
>>    "./i386-linux-user/qemu-i386 -strace -L /upstream/i386_wine /upstream/i386_wine/usr/local/bin/wine "C:\\Program Files\\Tencent\\WeChat\\WeChat.exe" > ana/try/info-strace.log 2>&1"
>>
>>  - The related output (no any munmap, 135168 = 128KB + 4KB):
>>
>>    4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000
>>    4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000
>>    4600 rt_sigprocmask(SIG_SETMASK,0x0033f574,NULL) = 0
>>    4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f5d0) = 0
>>    4600 write(3,0x33f6cc,64) = 64
>>    4600 read(4,0x33f6cc,64) = 1
>>    4600 rt_sigprocmask(SIG_SETMASK,0x0033f5d0,NULL) = 0
>>    4600 close(8) = 0
>>    4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f674) = 0
>>    4600 mprotect(0x00160000,65536,PROT_READ|PROT_WRITE) = 0
>>    4600 rt_sigprocmask(SIG_SETMASK,0x0033f674,NULL) = 0
>>    4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f990) = 0
>>    4600 mmap2(0x00340000,135168,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0) = 0x00340000
>>
>> wine often does like above, map the same position multiple times.
> 
> That output seems to show all the mmap calls working fine, though.
>

OK, thanks.
 
>>
>> For the case I provide above, it can call mmap_frag() twice for the same
>> host page.
> 
> For the same single call to target_mmap() ? What is the code flow
> within QEMU that causes this?
> 

Within one single call to target_mmap(), it should be OK.

But multiple call to target_mmap(), may call mmap_frag() multiple times
for the same host page (also for the same target page). In our case:

 - 4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000

   It will call mmap_frag() with start address 0x00340000 + 128KB, and
   set the target page with PAGE_VALID. But left the half below host
   page without PAGE_VALID.

 - 4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000

   It will call mmap_frag() with start address 0x00340000 + 128KB, and
   check the half below host page which has no PAGE_VALID, then "prot1
   == 0", mmap_frag() thinks "no page was there, so we allocate one".

 - But in fact, the first mmap_frag() has already allocated one page at
   0x00340000 + 128KB.

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Peter Maydell]

On 27 January 2016 at 01:37, Chen Gang <chengang@emindsoft.com.cn> wrote:
> Within one single call to target_mmap(), it should be OK.
>
> But multiple call to target_mmap(), may call mmap_frag() multiple times
> for the same host page (also for the same target page). In our case:
>
>  - 4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000
>
>    It will call mmap_frag() with start address 0x00340000 + 128KB, and
>    set the target page with PAGE_VALID. But left the half below host
>    page without PAGE_VALID.

So, just to put some numbers in here:

 0x340000 .. 0x34ffff      0x350000 .. 0x35ffff     0x360000 .. 0x360fff
   (64k, first host page)   (64k, second host page)  (4k guest page)

and we call mmap_frag() once for that last 4K fragment. It should:
 * allocate a host page (since none is there yet)
 * return to target_mmap, which will mark the range
   0x3f0000 .. 0x360fff as PROT_VALID (together with the other
   read/write/etc permissions)

I think this part is definitely correct.

>  - 4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000
>
>    It will call mmap_frag() with start address 0x00340000 + 128KB, and
>    check the half below host page which has no PAGE_VALID, then "prot1
>    == 0", mmap_frag() thinks "no page was there, so we allocate one".

On the second call, we again call mmap_frag for that last 4K.
We check for any other valid guest pages in the 64k host page,
and there aren't any. This will indeed cause us to mmap() again,
which ideally we would not. But:

(1) Is this actually causing anything to fail? Calling host
mmap() again is ever so slightly inefficient, but I don't think
that it causes the guest to see anything wrong.

(2) If we do want to fix this, your fix is doing the wrong thing.
It is correct that we don't mark the areas outside the guest page
as PROT_VALID, because they are not valid guest memory. If you
want to avoid the mmap() you need to change the condition we're using
to decide whether to mmap() a fresh host page (it would need to
look at the PROT_VALID bits within the new guest mapping, not just
the ones outside it). Something like:

    /* get the protection of the target pages outside the mapping,
     * and check whether we already have a host page allocated
     */
    prot1 = 0;
    havevalid = 0;
    for(addr = real_start; addr < real_end; addr++) {
        int pageprot = page_get_flags(addr);
        if (addr < start || addr >= end) {
            prot1 |= pageprot;
        }
        havevalid |= pageprot;
    }

    if (!havevalid) {
        /* no page was there, so ... */
        ...
    }

But I think it's only worth making this change if we're fixing
a real bug where the guest behaves wrongly.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] linux-user/mmap.c: Set prot page flags for the correct region in mmap_frag()

[Chen Gang]

On 2016年01月28日 22:54, Peter Maydell wrote:
> On 27 January 2016 at 01:37, Chen Gang <chengang@emindsoft.com.cn> wrote:
>> Within one single call to target_mmap(), it should be OK.
>>
>> But multiple call to target_mmap(), may call mmap_frag() multiple times
>> for the same host page (also for the same target page). In our case:
>>
>>  - 4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000
>>
>>    It will call mmap_frag() with start address 0x00340000 + 128KB, and
>>    set the target page with PAGE_VALID. But left the half below host
>>    page without PAGE_VALID.
> 
> So, just to put some numbers in here:
> 
>  0x340000 .. 0x34ffff      0x350000 .. 0x35ffff     0x360000 .. 0x360fff
>    (64k, first host page)   (64k, second host page)  (4k guest page)
> 
> and we call mmap_frag() once for that last 4K fragment. It should:
>  * allocate a host page (since none is there yet)
>  * return to target_mmap, which will mark the range
>    0x3f0000 .. 0x360fff as PROT_VALID (together with the other
>    read/write/etc permissions)
> 
> I think this part is definitely correct.
> 

Yes to me.

>>  - 4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000
>>
>>    It will call mmap_frag() with start address 0x00340000 + 128KB, and
>>    check the half below host page which has no PAGE_VALID, then "prot1
>>    == 0", mmap_frag() thinks "no page was there, so we allocate one".
> 
> On the second call, we again call mmap_frag for that last 4K.
> We check for any other valid guest pages in the 64k host page,
> and there aren't any. This will indeed cause us to mmap() again,
> which ideally we would not. But:
> 

OK.

> (1) Is this actually causing anything to fail? Calling host
> mmap() again is ever so slightly inefficient, but I don't think
> that it causes the guest to see anything wrong.
> 

For me, something may be a little complex (assume 8KB host page, 4KB
guest page):

 - 1st mmap2() is for MAP_PRIVATE, 2nd mmap2() is for MAP_SHARED.

 - So, if 2nd call mmap_frag() with the same start address only calls
   mprotect(), doesn't call mmap2() again, the target page will be
   still MAP_PRIVATE? (but caller wants it to be MAP_SHARED).

And theoretically, if the caller wants the 2 target pages within a host
page have different mapping attributes (e.g. half top host page is
MAP_SHARED, but half bottom host page is MAP_PRIVATE):

 - I guess, our current softmmu can not do that (we have to implment
   softmmu again, just like rth said originally).

 - But lucky to me, Wine will manage the whole memory by its own, and
   also windows its own also manage its whole memory. They try to be as
   simple as they can. So I guess, current softmmu is enough to me.

> (2) If we do want to fix this, your fix is doing the wrong thing.
> It is correct that we don't mark the areas outside the guest page
> as PROT_VALID, because they are not valid guest memory. If you
> want to avoid the mmap() you need to change the condition we're using
> to decide whether to mmap() a fresh host page (it would need to
> look at the PROT_VALID bits within the new guest mapping, not just
> the ones outside it). Something like:
> 
>     /* get the protection of the target pages outside the mapping,
>      * and check whether we already have a host page allocated
>      */
>     prot1 = 0;
>     havevalid = 0;
>     for(addr = real_start; addr < real_end; addr++) {
>         int pageprot = page_get_flags(addr);
>         if (addr < start || addr >= end) {
>             prot1 |= pageprot;
>         }
>         havevalid |= pageprot;
>     }
> 
>     if (!havevalid) {
>         /* no page was there, so ... */
>         ...
>     }
>
 
What you said above sounds OK to me, if we don't consider about
MAP_PRIVATE or MAP_SHARED.

After think of again, for me: we need keep the current code no touch,
but the related comments "/* no page was there, so ... */" need be
improved, I guess, it should be:

 - If there is no host page or only one target page, we need call mmap2
   again, which will satisfy the parameter 'flags' (e.g. MAP_PRIVATE or
   MAP_SHARED), else FIXME: at present, the 'flags' has to be skipped.


> But I think it's only worth making this change if we're fixing
> a real bug where the guest behaves wrongly.
> 

It sounds OK to me.

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed