| Message ID | 1452502867-12148-1-git-send-email-chengang@emindsoft.com.cn |
|---|---|
| State | New |
| Headers | show |
On 11 January 2016 at 09:01, <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. > > 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 <gang.chen.5i5j@gmail.com> > --- > 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(). It would help if you could explain what the failure case is that you're trying to fix, and how the current code fails. thanks -- PMM
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, <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.
>>
>> 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 <gang.chen.5i5j@gmail.com>
>> ---
>> 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.
On 26 January 2016 at 02:58, Chen Gang <chengang@emindsoft.com.cn> wrote: > 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. The comment means specifically "allocate a 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. But in what case will we call mmap_frag() again before we call page_set_flags() at the bottom of target_mmap()? That is what is not clear to me, and why I asked you to describe what the case is that you're seeing problems with. Reading the target_mmap() code, its intention seems to be: (a) if the whole allocation fits in one host page, call mmap_frag() once and then "goto the_end1" (b) otherwise, we'll call mmap_frag() once for the start of the guest mapping, and once for the end, which must be two different host pages So if you're seeing mmap_frag() called twice for the same host page then something is going wrong, but I'm not sure what. thanks -- PMM
On 2016年01月26日 17:11, Peter Maydell wrote: > On 26 January 2016 at 02:58, Chen Gang <chengang@emindsoft.com.cn> wrote: >> 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. > > The comment means specifically "allocate a host page". > OK, thanks. >> 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. > > But in what case will we call mmap_frag() again before we > call page_set_flags() at the bottom of target_mmap()? > That is what is not clear to me, and why I asked you to describe > what the case is that you're seeing problems with. > When I run WeChat.exe with i386 wine with qemu-i386 under sw_64 arch. - The related command: "./i386-linux-user/qemu-i386 -strace -L /upstream/i386_wine /upstream/i386_wine/usr/local/bin/wine "C:\\Program Files\\Tencent\\WeChat\\WeChat.exe" > ana/try/info-strace.log 2>&1" - The related output (no any munmap, 135168 = 128KB + 4KB): 4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000 4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000 4600 rt_sigprocmask(SIG_SETMASK,0x0033f574,NULL) = 0 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f5d0) = 0 4600 write(3,0x33f6cc,64) = 64 4600 read(4,0x33f6cc,64) = 1 4600 rt_sigprocmask(SIG_SETMASK,0x0033f5d0,NULL) = 0 4600 close(8) = 0 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f674) = 0 4600 mprotect(0x00160000,65536,PROT_READ|PROT_WRITE) = 0 4600 rt_sigprocmask(SIG_SETMASK,0x0033f674,NULL) = 0 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f990) = 0 4600 mmap2(0x00340000,135168,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0) = 0x00340000 wine often does like above, map the same position multiple times. > Reading the target_mmap() code, its intention seems to be: > (a) if the whole allocation fits in one host page, call > mmap_frag() once and then "goto the_end1" Also yes to me. > (b) otherwise, we'll call mmap_frag() once for the start > of the guest mapping, and once for the end, which must > be two different host pages > Also yes to me. > So if you're seeing mmap_frag() called twice for the same > host page then something is going wrong, but I'm not sure what. > For the case I provide above, it can call mmap_frag() twice for the same host page. By the way, after have a full test again, all related issues are OK, it seems we need not this patch to fix current issues, it is really very strange to me!(maybe it is fixed by my other patches? I don't know) At present, our sw_64 qemu-i386 status: - Can run notepad.exe correctly, can run acdsee5.0.exe setup program successfully. - The performance is acceptable, after optimize the wine code (simply use 32 split times instead of 2 for reserve_area recursion), the initialization speed is really quick enough. :-) - When run WeChat.exe, it can popup connection GUI box, but will quit under sw_64. But for x86_64 qemu-i386, it can run WeChat.exe correctly (although after enter main gui, it is not stable enough). Thanks.
On 26 January 2016 at 10:19, Chen Gang <chengang@emindsoft.com.cn> wrote: > When I run WeChat.exe with i386 wine with qemu-i386 under sw_64 arch. > > - The related command: > > "./i386-linux-user/qemu-i386 -strace -L /upstream/i386_wine /upstream/i386_wine/usr/local/bin/wine "C:\\Program Files\\Tencent\\WeChat\\WeChat.exe" > ana/try/info-strace.log 2>&1" > > - The related output (no any munmap, 135168 = 128KB + 4KB): > > 4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000 > 4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000 > 4600 rt_sigprocmask(SIG_SETMASK,0x0033f574,NULL) = 0 > 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f5d0) = 0 > 4600 write(3,0x33f6cc,64) = 64 > 4600 read(4,0x33f6cc,64) = 1 > 4600 rt_sigprocmask(SIG_SETMASK,0x0033f5d0,NULL) = 0 > 4600 close(8) = 0 > 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f674) = 0 > 4600 mprotect(0x00160000,65536,PROT_READ|PROT_WRITE) = 0 > 4600 rt_sigprocmask(SIG_SETMASK,0x0033f674,NULL) = 0 > 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f990) = 0 > 4600 mmap2(0x00340000,135168,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0) = 0x00340000 > > wine often does like above, map the same position multiple times. That output seems to show all the mmap calls working fine, though. >> Reading the target_mmap() code, its intention seems to be: >> (a) if the whole allocation fits in one host page, call >> mmap_frag() once and then "goto the_end1" > > Also yes to me. > >> (b) otherwise, we'll call mmap_frag() once for the start >> of the guest mapping, and once for the end, which must >> be two different host pages >> > > Also yes to me. > >> So if you're seeing mmap_frag() called twice for the same >> host page then something is going wrong, but I'm not sure what. >> > > For the case I provide above, it can call mmap_frag() twice for the same > host page. For the same single call to target_mmap() ? What is the code flow within QEMU that causes this? thanks -- PMM
On 2016年01月26日 18:26, Peter Maydell wrote: > On 26 January 2016 at 10:19, Chen Gang <chengang@emindsoft.com.cn> wrote: >> When I run WeChat.exe with i386 wine with qemu-i386 under sw_64 arch. >> >> - The related command: >> >> "./i386-linux-user/qemu-i386 -strace -L /upstream/i386_wine /upstream/i386_wine/usr/local/bin/wine "C:\\Program Files\\Tencent\\WeChat\\WeChat.exe" > ana/try/info-strace.log 2>&1" >> >> - The related output (no any munmap, 135168 = 128KB + 4KB): >> >> 4600 mmap2(0x00340000,135168,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00340000 >> 4600 mmap2(0x00340000,135168,PROT_READ,MAP_SHARED|MAP_FIXED,8,0) = 0x00340000 >> 4600 rt_sigprocmask(SIG_SETMASK,0x0033f574,NULL) = 0 >> 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f5d0) = 0 >> 4600 write(3,0x33f6cc,64) = 64 >> 4600 read(4,0x33f6cc,64) = 1 >> 4600 rt_sigprocmask(SIG_SETMASK,0x0033f5d0,NULL) = 0 >> 4600 close(8) = 0 >> 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f674) = 0 >> 4600 mprotect(0x00160000,65536,PROT_READ|PROT_WRITE) = 0 >> 4600 rt_sigprocmask(SIG_SETMASK,0x0033f674,NULL) = 0 >> 4600 rt_sigprocmask(SIG_BLOCK,0x7bced7e0,0x0033f990) = 0 >> 4600 mmap2(0x00340000,135168,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0) = 0x00340000 >> >> wine often does like above, map the same position multiple times. > > That output seems to show all the mmap calls working fine, though. > OK, thanks. >> >> For the case I provide above, it can call mmap_frag() twice for the same >> host page. > > For the same single call to target_mmap() ? What is the code flow > within QEMU that causes this? > 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. - 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". - But in fact, the first mmap_frag() has already allocated one page at 0x00340000 + 128KB. Thanks.
On 27 January 2016 at 01:37, Chen Gang <chengang@emindsoft.com.cn> 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
On 2016年01月28日 22:54, Peter Maydell wrote: > On 27 January 2016 at 01:37, Chen Gang <chengang@emindsoft.com.cn> 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.
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;