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

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.

Also let addr increasing step be TARGET_PAGE_SIZE, just like another
areas have done.

Also remove useless variable p.

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

On 31 December 2015 at 08:12,  <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.
>
> Also let addr increasing step be TARGET_PAGE_SIZE, just like another
> areas have done.
>
> Also remove useless variable p.

Please don't do multiple things in a single patch. This patch
has all of:
 * a fix for an unnecessary inefficiency
 * a coding style change with no functional effects
 * a bug fix

Mixing them up together like this makes it harder to evaluate
the bug fix, which is the important part of the change.

> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  linux-user/mmap.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 445e8c6..7920c5e 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -151,17 +151,19 @@ 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);
>      }
>
>      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,
> +                       prot | PAGE_VALID);

I'm not convinced this is right -- it will mean that we set
the page flags for every target page in this host page to
be the same thing (the ORed together result we calculated).
I don't think we want to update the page flags like that --
if one target page was read-only and the other read-write then
we need to make the whole host page read-write (since it's
bigger and covers both target pages), but we don't want to
incorrectly record that the first target-page is read-write.

I don't really understand this mmap code, though -- that's just
the result of looking at it for fifteen minutes or so.

>          prot1 = prot;
>      }
>      prot1 &= PAGE_BITS;
> --
> 1.7.3.4
>

thanks
-- PMM

On 2016年01月08日 03:35, Peter Maydell wrote:
> 
> Please don't do multiple things in a single patch. This patch
> has all of:
>  * a fix for an unnecessary inefficiency
>  * a coding style change with no functional effects
>  * a bug fix
> 
> Mixing them up together like this makes it harder to evaluate
> the bug fix, which is the important part of the change.
> 

OK, thanks.

>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>> index 445e8c6..7920c5e 100644
>> --- a/linux-user/mmap.c
>> +++ b/linux-user/mmap.c
>> @@ -151,17 +151,19 @@ 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);
>>      }
>>
>>      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,
>> +                       prot | PAGE_VALID);
> 
> I'm not convinced this is right -- it will mean that we set
> the page flags for every target page in this host page to
> be the same thing (the ORed together result we calculated).
> I don't think we want to update the page flags like that --
> if one target page was read-only and the other read-write then
> we need to make the whole host page read-write (since it's
> bigger and covers both target pages), but we don't want to
> incorrectly record that the first target-page is read-write.
> 

OK, thanks. It looks this patch is not quite precise. I guess, we need
use PAGE_VALID instead of "prot | PAGE_VALID" for page_set_flags() after
mmap() in mmap_frag().

So when the next call for the same location, prot1 will be PAGE_VALID (
now, it may be 0), then can protect to enter "if (prot1 == 0)", again.


> I don't really understand this mmap code, though -- that's just
> the result of looking at it for fifteen minutes or so.
> 

OK, I can understand. 

Thanks.