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,  <address@hidden> wrote:
>> From: Chen Gang <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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