[Qemu-devel] [PATCH, RFC] Disable implicit self-modifying code support for RISC CPUs

[Qemu-devel] [PATCH, RFC] Disable implicit self-modifying code support for RISC CPUs

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

Hi,

RISC CPUs don't support self-modifying code unless the affected area
is flushed explicitly. This patch disables the extra effort for SMC.
The changes in this version would affect all CPUs except x86, but I'd
like to see if there are problems with some target, so that the
committed change can be limited. Without comments, I'll just disable
SMC for Sparc, as there are no problems. So please comment, especially
if you want to "opt in".

For some reason, I can't disable all TB/TLB flushing, for example
there was already one line with TARGET_HAS_SMC || 1, but removing the
|| 1 part causes crashing. Does anyone know why?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: disable_smc.diff --]
[-- Type: text/x-diff; name=disable_smc.diff, Size: 11122 bytes --]

Index: qemu/exec.c
===================================================================
--- qemu.orig/exec.c	2007-11-03 11:39:44.000000000 +0000
+++ qemu/exec.c	2007-11-03 18:38:48.000000000 +0000
@@ -100,10 +100,12 @@
 typedef struct PageDesc {
     /* list of TBs intersecting this ram page */
     TranslationBlock *first_tb;
+#ifdef TARGET_HAS_SMC
     /* in order to optimize self modifying code, we count the number
        of lookups we do to a given page to use a bitmap */
     unsigned int code_write_count;
     uint8_t *code_bitmap;
+#endif
 #if defined(CONFIG_USER_ONLY)
     unsigned long flags;
 #endif
@@ -306,6 +308,7 @@
     *penv = env;
 }
 
+#ifdef TARGET_HAS_SMC
 static inline void invalidate_page_bitmap(PageDesc *p)
 {
     if (p->code_bitmap) {
@@ -314,6 +317,7 @@
     }
     p->code_write_count = 0;
 }
+#endif
 
 /* set to NULL all the 'first_tb' fields in all PageDescs */
 static void page_flush_tb(void)
@@ -326,7 +330,9 @@
         if (p) {
             for(j = 0; j < L2_SIZE; j++) {
                 p->first_tb = NULL;
+#ifdef TARGET_HAS_SMC
                 invalidate_page_bitmap(p);
+#endif
                 p++;
             }
         }
@@ -339,10 +345,11 @@
 {
     CPUState *env;
 #if defined(DEBUG_FLUSH)
-    printf("qemu: flush code_size=%d nb_tbs=%d avg_tb_size=%d\n",
-           code_gen_ptr - code_gen_buffer,
+    printf("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%ld\n",
+           (unsigned long)(code_gen_ptr - code_gen_buffer),
            nb_tbs,
-           nb_tbs > 0 ? (code_gen_ptr - code_gen_buffer) / nb_tbs : 0);
+           nb_tbs > 0 ? ((unsigned long)(code_gen_ptr - code_gen_buffer))
+           / nb_tbs : 0);
 #endif
     nb_tbs = 0;
 
@@ -502,12 +509,16 @@
     if (tb->page_addr[0] != page_addr) {
         p = page_find(tb->page_addr[0] >> TARGET_PAGE_BITS);
         tb_page_remove(&p->first_tb, tb);
+#ifdef TARGET_HAS_SMC
         invalidate_page_bitmap(p);
+#endif
     }
     if (tb->page_addr[1] != -1 && tb->page_addr[1] != page_addr) {
         p = page_find(tb->page_addr[1] >> TARGET_PAGE_BITS);
         tb_page_remove(&p->first_tb, tb);
+#ifdef TARGET_HAS_SMC
         invalidate_page_bitmap(p);
+#endif
     }
 
     tb_invalidated_flag = 1;
@@ -567,6 +578,7 @@
     }
 }
 
+#ifdef TARGET_HAS_SMC
 static void build_page_bitmap(PageDesc *p)
 {
     int n, tb_start, tb_end;
@@ -597,6 +609,7 @@
         tb = tb->page_next[n];
     }
 }
+#endif
 
 #ifdef TARGET_HAS_PRECISE_SMC
 
@@ -653,12 +666,14 @@
     p = page_find(start >> TARGET_PAGE_BITS);
     if (!p)
         return;
+#ifdef TARGET_HAS_SMC
     if (!p->code_bitmap &&
         ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD &&
         is_cpu_write_access) {
         /* build code bitmap */
         build_page_bitmap(p);
     }
+#endif
 
     /* we remove all the TBs in the range [start, end[ */
     /* XXX: see if in some cases it could be faster to invalidate all the code */
@@ -733,7 +748,9 @@
 #if !defined(CONFIG_USER_ONLY)
     /* if no code remaining, no need to continue to use slow writes */
     if (!p->first_tb) {
+#ifdef TARGET_HAS_SMC
         invalidate_page_bitmap(p);
+#endif
         if (is_cpu_write_access) {
             tlb_unprotect_code_phys(env, start, env->mem_write_vaddr);
         }
@@ -756,7 +773,9 @@
 static inline void tb_invalidate_phys_page_fast(target_ulong start, int len)
 {
     PageDesc *p;
+#ifdef TARGET_HAS_SMC
     int offset, b;
+#endif
 #if 0
     if (1) {
         if (loglevel) {
@@ -770,6 +789,7 @@
     p = page_find(start >> TARGET_PAGE_BITS);
     if (!p)
         return;
+#ifdef TARGET_HAS_SMC
     if (p->code_bitmap) {
         offset = start & ~TARGET_PAGE_MASK;
         b = p->code_bitmap[offset >> 3] >> (offset & 7);
@@ -777,11 +797,15 @@
             goto do_invalidate;
     } else {
     do_invalidate:
+#endif
         tb_invalidate_phys_page_range(start, start + len, 1);
+#ifdef TARGET_HAS_SMC
     }
+#endif
 }
 
 #if !defined(CONFIG_SOFTMMU)
+#ifndef TARGET_SPARC
 static void tb_invalidate_phys_page(target_ulong addr,
                                     unsigned long pc, void *puc)
 {
@@ -849,6 +873,7 @@
 #endif
 }
 #endif
+#endif
 
 /* add the tb in the target page and protect it if necessary */
 static inline void tb_alloc_page(TranslationBlock *tb,
@@ -862,11 +887,14 @@
     tb->page_next[n] = p->first_tb;
     last_first_tb = p->first_tb;
     p->first_tb = (TranslationBlock *)((long)tb | n);
+#ifdef TARGET_HAS_SMC
     invalidate_page_bitmap(p);
+#endif
 
 #if defined(TARGET_HAS_SMC) || 1
 
 #if defined(CONFIG_USER_ONLY)
+#if !defined(TARGET_SPARC)
     if (p->flags & PAGE_WRITE) {
         target_ulong addr;
         PageDesc *p2;
@@ -889,10 +917,11 @@
         mprotect(g2h(page_addr), qemu_host_page_size,
                  (prot & PAGE_BITS) & ~PAGE_WRITE);
 #ifdef DEBUG_TB_INVALIDATE
-        printf("protecting code page: 0x%08lx\n",
+        printf("protecting code page: 0x" TARGET_FMT_lx "\n",
                page_addr);
 #endif
     }
+#endif
 #else
     /* if some code is already present, then the pages are already
        protected. So we handle the case where only the first TB is
@@ -1540,6 +1569,7 @@
 #endif
 }
 
+#ifdef USE_KQEMU
 static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
 {
     ram_addr_t ram_addr;
@@ -1570,6 +1600,7 @@
 #endif
 #endif
 }
+#endif
 
 static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry,
                                   unsigned long start)
@@ -1737,6 +1768,7 @@
     return ret;
 }
 
+#ifndef TARGET_SPARC
 /* called from signal handler: invalidate the code and unprotect the
    page. Return TRUE if the fault was succesfully handled. */
 int page_unprotect(target_ulong addr, unsigned long pc, void *puc)
@@ -1777,6 +1809,7 @@
     return 0;
 #endif
 }
+#endif
 
 #else
 
@@ -1858,23 +1891,28 @@
 
     start = start & TARGET_PAGE_MASK;
     end = TARGET_PAGE_ALIGN(end);
+#ifdef TARGET_HAS_SMC
     if (flags & PAGE_WRITE)
         flags |= PAGE_WRITE_ORG;
+#endif
     spin_lock(&tb_lock);
     for(addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
         p = page_find_alloc(addr >> TARGET_PAGE_BITS);
         /* if the write protection is set, then we invalidate the code
            inside */
+#ifdef TARGET_HAS_SMC
         if (!(p->flags & PAGE_WRITE) &&
             (flags & PAGE_WRITE) &&
             p->first_tb) {
             tb_invalidate_phys_page(addr, 0, NULL);
         }
+#endif
         p->flags = flags;
     }
     spin_unlock(&tb_lock);
 }
 
+#ifndef TARGET_SPARC
 /* called from signal handler: invalidate the code and unprotect the
    page. Return TRUE if the fault was succesfully handled. */
 int page_unprotect(target_ulong address, unsigned long pc, void *puc)
@@ -1929,6 +1967,7 @@
         page_unprotect(addr, 0, NULL);
     }
 }
+#endif
 
 static inline void tlb_set_dirty(CPUState *env,
                                  unsigned long addr, target_ulong vaddr)
@@ -2549,6 +2588,7 @@
                 /* RAM case */
                 ptr = phys_ram_base + addr1;
                 memcpy(ptr, buf, l);
+#ifdef TARGET_HAS_SMC
                 if (!cpu_physical_memory_is_dirty(addr1)) {
                     /* invalidate code */
                     tb_invalidate_phys_page_range(addr1, addr1 + l, 0);
@@ -2556,6 +2596,7 @@
                     phys_ram_dirty[addr1 >> TARGET_PAGE_BITS] |=
                         (0xff & ~CODE_DIRTY_FLAG);
                 }
+#endif
             }
         } else {
             if ((pd & ~TARGET_PAGE_MASK) > IO_MEM_ROM &&
@@ -2794,6 +2835,7 @@
         /* RAM case */
         ptr = phys_ram_base + addr1;
         stl_p(ptr, val);
+#ifdef TARGET_HAS_SMC
         if (!cpu_physical_memory_is_dirty(addr1)) {
             /* invalidate code */
             tb_invalidate_phys_page_range(addr1, addr1 + 4, 0);
@@ -2801,6 +2843,7 @@
             phys_ram_dirty[addr1 >> TARGET_PAGE_BITS] |=
                 (0xff & ~CODE_DIRTY_FLAG);
         }
+#endif
     }
 }
 
Index: qemu/target-sparc/helper.c
===================================================================
--- qemu.orig/target-sparc/helper.c	2007-11-03 11:39:44.000000000 +0000
+++ qemu/target-sparc/helper.c	2007-11-03 11:47:19.000000000 +0000
@@ -203,12 +203,6 @@
 
     /* the page can be put in the TLB */
     *prot = perm_table[is_user][access_perms];
-    if (!(pde & PG_MODIFIED_MASK)) {
-        /* only set write access if already dirty... otherwise wait
-           for dirty access */
-        *prot &= ~PAGE_WRITE;
-    }
-
     /* Even if large ptes, we map only one 4KB page in the cache to
        avoid filling it too fast */
     *physical = ((target_phys_addr_t)(pde & PTE_ADDR_MASK) << 4) + page_offset;
Index: qemu/exec-all.h
===================================================================
--- qemu.orig/exec-all.h	2007-11-03 11:39:44.000000000 +0000
+++ qemu/exec-all.h	2007-11-03 14:34:14.000000000 +0000
@@ -109,7 +109,9 @@
                            void *puc);
 void cpu_resume_from_signal(CPUState *env1, void *puc);
 void cpu_exec_init(CPUState *env);
+#ifndef TARGET_SPARC
 int page_unprotect(target_ulong address, unsigned long pc, void *puc);
+#endif
 void tb_invalidate_phys_page_range(target_ulong start, target_ulong end,
                                    int is_cpu_write_access);
 void tb_invalidate_page_range(target_ulong start, target_ulong end);
@@ -122,8 +124,10 @@
                                target_phys_addr_t paddr, int prot,
                                int mmu_idx, int is_softmmu)
 {
+#ifndef TARGET_SPARC
     if (prot & PAGE_READ)
         prot |= PAGE_EXEC;
+#endif
     return tlb_set_page_exec(env, vaddr, paddr, prot, mmu_idx, is_softmmu);
 }
 
Index: qemu/cpu-exec.c
===================================================================
--- qemu.orig/cpu-exec.c	2007-11-03 14:22:28.000000000 +0000
+++ qemu/cpu-exec.c	2007-11-03 14:30:03.000000000 +0000
@@ -964,10 +964,6 @@
     printf("qemu: SIGSEGV pc=0x%08lx address=%08lx w=%d oldset=0x%08lx\n",
            pc, address, is_write, *(unsigned long *)old_set);
 #endif
-    /* XXX: locking issue */
-    if (is_write && page_unprotect(h2g(address), pc, puc)) {
-        return 1;
-    }
     /* see if it is an MMU fault */
     ret = cpu_sparc_handle_mmu_fault(env, address, is_write, MMU_USER_IDX, 0);
     if (ret < 0)
Index: qemu/linux-user/syscall.c
===================================================================
--- qemu.orig/linux-user/syscall.c	2007-11-03 14:36:28.000000000 +0000
+++ qemu/linux-user/syscall.c	2007-11-03 14:36:50.000000000 +0000
@@ -2628,7 +2628,9 @@
         ret = 0; /* avoid warning */
         break;
     case TARGET_NR_read:
+#ifndef TARGET_SPARC
         page_unprotect_range(arg2, arg3);
+#endif
         p = lock_user(arg2, arg3, 0);
         ret = get_errno(read(arg1, p, arg3));
         unlock_user(p, arg2, ret);
@@ -4377,7 +4379,9 @@
         break;
 #ifdef TARGET_NR_pread
     case TARGET_NR_pread:
+#ifndef TARGET_SPARC
         page_unprotect_range(arg2, arg3);
+#endif
         p = lock_user(arg2, arg3, 0);
         ret = get_errno(pread(arg1, p, arg3, arg4));
         unlock_user(p, arg2, ret);

Re: [Qemu-devel] [PATCH, RFC] Disable implicit self-modifying code support for RISC CPUs

[Thiemo Seufer]

Blue Swirl wrote:
> Hi,
> 
> RISC CPUs don't support self-modifying code unless the affected area
> is flushed explicitly.

Not entirely true. There are cacheless MIPS CPUs (the m4k), and also
cache-snooping MIPS CPUs (the R1x000).

> This patch disables the extra effort for SMC.
> The changes in this version would affect all CPUs except x86, but I'd
> like to see if there are problems with some target, so that the
> committed change can be limited. Without comments, I'll just disable
> SMC for Sparc, as there are no problems. So please comment, especially
> if you want to "opt in".

I prefer at least MIPS to stay as is.


Thiemo

Re: [Qemu-devel] [PATCH, RFC] Disable implicit self-modifying code support for RISC CPUs

[Fabrice Bellard]

Blue Swirl wrote:
> Hi,
> 
> RISC CPUs don't support self-modifying code unless the affected area
> is flushed explicitly. This patch disables the extra effort for SMC.
> The changes in this version would affect all CPUs except x86, but I'd
> like to see if there are problems with some target, so that the
> committed change can be limited. Without comments, I'll just disable
> SMC for Sparc, as there are no problems. So please comment, especially
> if you want to "opt in".
> 
> For some reason, I can't disable all TB/TLB flushing, for example
> there was already one line with TARGET_HAS_SMC || 1, but removing the
> || 1 part causes crashing. Does anyone know why?

With the current QEMU architecture, you cannot disable self-modifying
code as you did. This is why I did not fully supported the
TARGET_HAS_SMC flag. The problem is that the translator make the
assumption that the RAM and the TB contents are consistent for example
when handling exceptions. Suppressing this assumption is possible but
requires more work.

Regards,

Fabrice.

Re: [Qemu-devel] [PATCH, RFC] Disable implicit self-modifying code support for RISC CPUs

[Paul Brook]

> RISC CPUs don't support self-modifying code unless the affected area
> is flushed explicitly. 

For experience with ARM cpus, I think this is only true for userspace.

Many CPUs only require explicit flushes when the icache is enabled. It's not 
uncommon for bootloaders to leave the icache disabled and omit the cache 
flushes.

The ARM cache flush instructions/syscalls are currently implemented as a 
no-op, so nontrivial additional work would be required to disabled the qemu 
SMC detections. IIRC there are also special cases where a system call 
instruction guarantees some level of architectural consistency for backwards 
compatibility.

On some cores it is only necessary to flush the pipeline, but it's also common 
to know that e.g. a particular core has a 4-stage pipeline, so inserting 4 
NOPs is sufficient to ensure consistency.

Paul

Re: [Qemu-devel] [PATCH, RFC] Disable implicit self-modifying code support for RISC CPUs

[Blue Swirl]

On 11/4/07, Fabrice Bellard <fabrice@bellard.org> wrote:
> Blue Swirl wrote:
> > Hi,
> >
> > RISC CPUs don't support self-modifying code unless the affected area
> > is flushed explicitly. This patch disables the extra effort for SMC.
> > The changes in this version would affect all CPUs except x86, but I'd
> > like to see if there are problems with some target, so that the
> > committed change can be limited. Without comments, I'll just disable
> > SMC for Sparc, as there are no problems. So please comment, especially
> > if you want to "opt in".
> >
> > For some reason, I can't disable all TB/TLB flushing, for example
> > there was already one line with TARGET_HAS_SMC || 1, but removing the
> > || 1 part causes crashing. Does anyone know why?
>
> With the current QEMU architecture, you cannot disable self-modifying
> code as you did. This is why I did not fully supported the
> TARGET_HAS_SMC flag. The problem is that the translator make the
> assumption that the RAM and the TB contents are consistent for example
> when handling exceptions. Suppressing this assumption is possible but
> requires more work.

I think the conclusion is that we would need some kind of emulator for
i-cache for any accurate emulation. And handling the boot loader may
need an uncached mode.

The performance benefit from disabling SMC is unnoticeable according
to my benchmarks. Adding a TB flush to i-cache flushing made things
worse. Moreover, SMC is hardly ever used on Sparc.

I'll just commit the debug statement fixes and the fix that separates
PAGE_READ from PAGE_EXEC for Sparc.

Maybe this issue should be documented in qemu-tech.texi, there are
also frequently some questions about caches.

Re: [Qemu-devel] [PATCH, RFC] Disable implicit self-modifying code support for RISC CPUs

[J. Mayer]

On Sun, 2007-11-04 at 09:12 +0200, Blue Swirl wrote:
> On 11/4/07, Fabrice Bellard <fabrice@bellard.org> wrote:
> > Blue Swirl wrote:
> > > Hi,
> > >
> > > RISC CPUs don't support self-modifying code unless the affected area
> > > is flushed explicitly. This patch disables the extra effort for SMC.
> > > The changes in this version would affect all CPUs except x86, but I'd
> > > like to see if there are problems with some target, so that the
> > > committed change can be limited. Without comments, I'll just disable
> > > SMC for Sparc, as there are no problems. So please comment, especially
> > > if you want to "opt in".
> > >
> > > For some reason, I can't disable all TB/TLB flushing, for example
> > > there was already one line with TARGET_HAS_SMC || 1, but removing the
> > > || 1 part causes crashing. Does anyone know why?
> >
> > With the current QEMU architecture, you cannot disable self-modifying
> > code as you did. This is why I did not fully supported the
> > TARGET_HAS_SMC flag. The problem is that the translator make the
> > assumption that the RAM and the TB contents are consistent for example
> > when handling exceptions. Suppressing this assumption is possible but
> > requires more work.
> 
> I think the conclusion is that we would need some kind of emulator for
> i-cache for any accurate emulation. And handling the boot loader may
> need an uncached mode.

> The performance benefit from disabling SMC is unnoticeable according
> to my benchmarks. Adding a TB flush to i-cache flushing made things
> worse. Moreover, SMC is hardly ever used on Sparc.
> 
> I'll just commit the debug statement fixes and 

> the fix that separates
> PAGE_READ from PAGE_EXEC for Sparc.

This patch is absolutely not needed. You have to directly call
tlb_set_page_exec instead of tlb_set_page if you want to separate
PAGE_READ from PAGE_EXEC. 
#ifdef TARGET_xxx should never occur in generic code and in that
specific case, it's the Sparc target code that has to be fixed...

> Maybe this issue should be documented in qemu-tech.texi, there are
> also frequently some questions about caches.

Yes, some documentation on such tricks can never hurt !

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] [PATCH, RFC] Disable implicit self-modifying code support for RISC CPUs

[Blue Swirl]

On 11/4/07, J. Mayer <l_indien@magic.fr> wrote:
>
> On Sun, 2007-11-04 at 09:12 +0200, Blue Swirl wrote:
> > On 11/4/07, Fabrice Bellard <fabrice@bellard.org> wrote:
> > > Blue Swirl wrote:
> > > > Hi,
> > > >
> > > > RISC CPUs don't support self-modifying code unless the affected area
> > > > is flushed explicitly. This patch disables the extra effort for SMC.
> > > > The changes in this version would affect all CPUs except x86, but I'd
> > > > like to see if there are problems with some target, so that the
> > > > committed change can be limited. Without comments, I'll just disable
> > > > SMC for Sparc, as there are no problems. So please comment, especially
> > > > if you want to "opt in".
> > > >
> > > > For some reason, I can't disable all TB/TLB flushing, for example
> > > > there was already one line with TARGET_HAS_SMC || 1, but removing the
> > > > || 1 part causes crashing. Does anyone know why?
> > >
> > > With the current QEMU architecture, you cannot disable self-modifying
> > > code as you did. This is why I did not fully supported the
> > > TARGET_HAS_SMC flag. The problem is that the translator make the
> > > assumption that the RAM and the TB contents are consistent for example
> > > when handling exceptions. Suppressing this assumption is possible but
> > > requires more work.
> >
> > I think the conclusion is that we would need some kind of emulator for
> > i-cache for any accurate emulation. And handling the boot loader may
> > need an uncached mode.
>
> > The performance benefit from disabling SMC is unnoticeable according
> > to my benchmarks. Adding a TB flush to i-cache flushing made things
> > worse. Moreover, SMC is hardly ever used on Sparc.
> >
> > I'll just commit the debug statement fixes and
>
> > the fix that separates
> > PAGE_READ from PAGE_EXEC for Sparc.
>
> This patch is absolutely not needed. You have to directly call
> tlb_set_page_exec instead of tlb_set_page if you want to separate
> PAGE_READ from PAGE_EXEC.
> #ifdef TARGET_xxx should never occur in generic code and in that
> specific case, it's the Sparc target code that has to be fixed...

In fact Sparc code calls only tlb_set_page_exec, never tlb_set_page,
so no fix is necessary.

This reminds me that there is some TARGET_SPARC conditional code in
fdc.c, I'll change those.