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.
--
Chen Gang (陈刚)
Open, share, and attitude like air, water, and life which God blessed
