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 <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.

>  - 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