[Qemu-devel] [PATCH] translate-all.c: Remove writable protection feature for tb_alloc_page()

[Qemu-devel] [PATCH] translate-all.c: Remove writable protection feature for tb_alloc_page()

[chengang]

From: Chen Gang <chengang@emindsoft.com.cn>

Guest may allocate a readable, writable, and executable page, then write
data on the page, and execute data as code on the page too, then write
anther data still within the page.

So remove this feature from linux-user: it not only consumes a little
performance, but also causes issue with the old Linux kernel under some
of architectures (they will directly generate segment fault for it).

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 translate-all.c | 29 +----------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 042a857..1b6e95d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1430,34 +1430,7 @@ static inline void tb_alloc_page(TranslationBlock *tb,
     p->first_tb = (TranslationBlock *)((uintptr_t)tb | n);
     invalidate_page_bitmap(p);
 
-#if defined(CONFIG_USER_ONLY)
-    if (p->flags & PAGE_WRITE) {
-        target_ulong addr;
-        PageDesc *p2;
-        int prot;
-
-        /* force the host page as non writable (writes will have a
-           page fault + mprotect overhead) */
-        page_addr &= qemu_host_page_mask;
-        prot = 0;
-        for (addr = page_addr; addr < page_addr + qemu_host_page_size;
-            addr += TARGET_PAGE_SIZE) {
-
-            p2 = page_find(addr >> TARGET_PAGE_BITS);
-            if (!p2) {
-                continue;
-            }
-            prot |= p2->flags;
-            p2->flags &= ~PAGE_WRITE;
-          }
-        mprotect(g2h(page_addr), qemu_host_page_size,
-                 (prot & PAGE_BITS) & ~PAGE_WRITE);
-#ifdef DEBUG_TB_INVALIDATE
-        printf("protecting code page: 0x" TARGET_FMT_lx "\n",
-               page_addr);
-#endif
-    }
-#else
+#if !defined(CONFIG_USER_ONLY)
     /* if some code is already present, then the pages are already
        protected. So we handle the case where only the first TB is
        allocated in a physical page */
-- 
1.9.1

Re: [Qemu-devel] [PATCH] translate-all.c: Remove writable protection feature for tb_alloc_page()

[Peter Maydell]

On 14 January 2016 at 06:03,  <chengang@emindsoft.com.cn> wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
>
> Guest may allocate a readable, writable, and executable page, then write
> data on the page, and execute data as code on the page too, then write
> anther data still within the page.
>
> So remove this feature from linux-user: it not only consumes a little
> performance, but also causes issue with the old Linux kernel under some
> of architectures (they will directly generate segment fault for it).

If we don't mark the page as non-writeable when we generate a TB
from it, how do we detect when guest code later writes to that
page (which means we need to invalidate the TB) ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] translate-all.c: Remove writable protection feature for tb_alloc_page()

[Chen Gang]

On 2016年01月14日 18:05, Peter Maydell wrote:
> On 14 January 2016 at 06:03,  <chengang@emindsoft.com.cn> wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> Guest may allocate a readable, writable, and executable page, then write
>> data on the page, and execute data as code on the page too, then write
>> anther data still within the page.
>>
>> So remove this feature from linux-user: it not only consumes a little
>> performance, but also causes issue with the old Linux kernel under some
>> of architectures (they will directly generate segment fault for it).
> 
> If we don't mark the page as non-writeable when we generate a TB
> from it, how do we detect when guest code later writes to that
> page (which means we need to invalidate the TB) ?
> 

For me, what you said above sounds reasonable, at present, that's really
valuable to me :-)

I guess, you also mean: our qemu will catch the host page fault signal
and invalidate the TB.

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] translate-all.c: Remove writable protection feature for tb_alloc_page()

[Peter Maydell]

On 14 January 2016 at 10:26, Chen Gang <chengang@emindsoft.com.cn> wrote:
> On 2016年01月14日 18:05, Peter Maydell wrote:
>> If we don't mark the page as non-writeable when we generate a TB
>> from it, how do we detect when guest code later writes to that
>> page (which means we need to invalidate the TB) ?
>>
>
> For me, what you said above sounds reasonable, at present, that's really
> valuable to me :-)
>
> I guess, you also mean: our qemu will catch the host page fault signal
> and invalidate the TB.

Yes, this is how it works for user-mode. (For softmmu we can catch
writes and send them via the slow path which does the check for
whether TBs need to be invalidated; for linux-user we have no
emulated MMU so we must rely on the host kernel sending us the
SIGSEGV.) The bit of code that does this is at the top of
handle_cpu_signal():

    if (is_write && h2g_valid(address)
        && page_unprotect(h2g(address), pc, puc)) {
        return 1;
    }

thanks
-- PMM

Re: [Qemu-devel] [PATCH] translate-all.c: Remove writable protection feature for tb_alloc_page()

[Chen Gang]

On 2016年01月14日 18:30, Peter Maydell wrote:
> On 14 January 2016 at 10:26, Chen Gang <chengang@emindsoft.com.cn> wrote:
>> On 2016年01月14日 18:05, Peter Maydell wrote:
>>> If we don't mark the page as non-writeable when we generate a TB
>>> from it, how do we detect when guest code later writes to that
>>> page (which means we need to invalidate the TB) ?
>>>
>>
>> For me, what you said above sounds reasonable, at present, that's really
>> valuable to me :-)
>>
>> I guess, you also mean: our qemu will catch the host page fault signal
>> and invalidate the TB.
> 
> Yes, this is how it works for user-mode. (For softmmu we can catch
> writes and send them via the slow path which does the check for
> whether TBs need to be invalidated; for linux-user we have no
> emulated MMU so we must rely on the host kernel sending us the
> SIGSEGV.) The bit of code that does this is at the top of
> handle_cpu_signal():
> 
>     if (is_write && h2g_valid(address)
>         && page_unprotect(h2g(address), pc, puc)) {
>         return 1;
>     }
> 

OK, thank you very much!  :-)


Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed