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 <address@hidden> 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