[Qemu-devel] Buggy wfi support for ARM user

[Qemu-devel] Buggy wfi support for ARM user

[Laurent Desnogues]

The wfi instruction for user mode ARM results in:

qemu: unhandled CPU exception 0x10001 - aborting

I propose that for user mode this instruction be considered
as a NOP.  Any thoughts?


Laurent

Re: [Qemu-devel] Buggy wfi support for ARM user

[Paul Brook]

On Sunday 08 June 2008, Laurent Desnogues wrote:
> The wfi instruction for user mode ARM results in:
>
> qemu: unhandled CPU exception 0x10001 - aborting
>
> I propose that for user mode this instruction be considered
> as a NOP.  Any thoughts?

Allowing usermode to issue WFI sounds like a bug.

Paul

Re: [Qemu-devel] Buggy wfi support for ARM user

[Laurent Desnogues]

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

>> I propose that for user mode this instruction be considered
>> as a NOP.  Any thoughts?
>
> Allowing usermode to issue WFI sounds like a bug.

Here is a proposal that makes WFI no-ops for user mode emulation.
Comments welcome, especially as I am not used to send patches :)


Laurent

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

diff -ru -x .svn trunk/config-host.mak edit/config-host.mak
--- trunk/config-host.mak	2008-06-08 14:47:33.000000000 +0200
+++ edit/config-host.mak	2008-06-08 17:39:24.000000000 +0200
@@ -27,7 +27,7 @@
 CONFIG_VNC_TLS_CFLAGS= 
 CONFIG_VNC_TLS_LIBS=-lgnutls  
 VERSION=0.9.1
-SRC_PATH=/home/ldesnogu/work/Emu/qemu/trunk
+SRC_PATH=/home/ldesnogu/work/Emu/qemu/edit
 TARGET_DIRS=arm-linux-user
 CONFIG_SDL=yes
 SDL_LIBS=-L/usr/lib64 -lSDL -lpthread
diff -ru -x .svn trunk/target-arm/helpers.h edit/target-arm/helpers.h
--- trunk/target-arm/helpers.h	2008-04-17 23:10:21.000000000 +0200
+++ edit/target-arm/helpers.h	2008-06-08 17:51:23.000000000 +0200
@@ -122,7 +122,9 @@
 
 DEF_HELPER_1_3(sel_flags, uint32_t, (uint32_t, uint32_t, uint32_t))
 DEF_HELPER_0_1(exception, void, (uint32_t))
+#ifndef CONFIG_USER_ONLY
 DEF_HELPER_0_0(wfi, void, (void))
+#endif
 
 DEF_HELPER_0_2(cpsr_write, void, (uint32_t, uint32_t))
 DEF_HELPER_1_0(cpsr_read, uint32_t, (void))
diff -ru -x .svn trunk/target-arm/op_helper.c edit/target-arm/op_helper.c
--- trunk/target-arm/op_helper.c	2008-06-01 12:29:17.000000000 +0200
+++ edit/target-arm/op_helper.c	2008-06-08 17:50:55.000000000 +0200
@@ -247,12 +247,14 @@
     return res;
 }
 
+#ifndef CONFIG_USER_ONLY
 void HELPER(wfi)(void)
 {
     env->exception_index = EXCP_HLT;
     env->halted = 1;
     cpu_loop_exit();
 }
+#endif
 
 void HELPER(exception)(uint32_t excp)
 {
diff -ru -x .svn trunk/target-arm/translate.c edit/target-arm/translate.c
--- trunk/target-arm/translate.c	2008-06-07 13:59:11.000000000 +0200
+++ edit/target-arm/translate.c	2008-06-08 17:50:09.000000000 +0200
@@ -69,7 +69,10 @@
 
 /* These instructions trap after executing, so defer them until after the
    conditional executions state has been updated.  */
+/* for user mode hosted emulation wfi is treated as a no-op */
+#ifndef CONFIG_USER_ONLY
 #define DISAS_WFI 4
+#endif
 #define DISAS_SWI 5
 
 /* XXX: move that elsewhere */
@@ -2619,8 +2622,12 @@
     if ((insn & 0x0fff0fff) == 0x0e070f90
         || (insn & 0x0fff0fff) == 0x0e070f58) {
         /* Wait for interrupt.  */
+        /* for user mode hosted emulation wfi is treated as a no-op */
+        /* FIXME:  for ARMv7 this should always be a no-op */
+#ifndef CONFIG_USER_ONLY
         gen_set_pc_im(s->pc);
         s->is_jmp = DISAS_WFI;
+#endif
         return 0;
     }
     rd = (insn >> 12) & 0xf;
@@ -3499,8 +3506,11 @@
 {
     switch (val) {
     case 3: /* wfi */
+        /* for user mode hosted emulation wfi is treated as a no-op */
+#ifndef CONFIG_USER_ONLY
         gen_set_pc_im(s->pc);
         s->is_jmp = DISAS_WFI;
+#endif
         break;
     case 2: /* wfe */
     case 4: /* sev */
@@ -8712,9 +8722,11 @@
         case DISAS_TB_JUMP:
             /* nothing more to generate */
             break;
+#ifndef CONFIG_USER_ONLY
         case DISAS_WFI:
             gen_helper_wfi();
             break;
+#endif
         case DISAS_SWI:
             gen_exception(EXCP_SWI);
             break;

Re: [Qemu-devel] Buggy wfi support for ARM user

[Blue Swirl]

On 6/8/08, Laurent Desnogues <laurent.desnogues@gmail.com> wrote:
> >> I propose that for user mode this instruction be considered
>  >> as a NOP.  Any thoughts?
>  >
>  > Allowing usermode to issue WFI sounds like a bug.
>
>  Here is a proposal that makes WFI no-ops for user mode emulation.
>  Comments welcome, especially as I am not used to send patches :)

Generally, the config-host.mak part should be suppressed in patches.

I don't know about this WFI case or even much of ARM at all, but if
enabling WFI is controversial, maybe it could be implemented as a CPU
feature so that it is disabled by default.

Re: [Qemu-devel] Buggy wfi support for ARM user

[Laurent Desnogues]

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

On Sun, Jun 8, 2008 at 6:38 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On 6/8/08, Laurent Desnogues <laurent.desnogues@gmail.com> wrote:
>> >> I propose that for user mode this instruction be considered
>>  >> as a NOP.  Any thoughts?
>>  >
>>  > Allowing usermode to issue WFI sounds like a bug.
>>
>>  Here is a proposal that makes WFI no-ops for user mode emulation.
>>  Comments welcome, especially as I am not used to send patches :)
>
> Generally, the config-host.mak part should be suppressed in patches.

Sorry about that, I used make clean instead of distclean.

> I don't know about this WFI case or even much of ARM at all, but if
> enabling WFI is controversial, maybe it could be implemented as a CPU
> feature so that it is disabled by default.

It's not controversial I think.  It's just that it's architected to be
executable by user processes.  However it only makes sense
if a process can receive interruptions which is not really the
case for qemu user mode emulation.  This instruction is used
to reduce power in idle loops.


Laurent

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

Only in trunk: build.log
diff -ru -x .svn trunk/target-arm/helpers.h edit/target-arm/helpers.h
--- trunk/target-arm/helpers.h	2008-04-17 23:10:21.000000000 +0200
+++ edit/target-arm/helpers.h	2008-06-08 17:51:23.000000000 +0200
@@ -122,7 +122,9 @@
 
 DEF_HELPER_1_3(sel_flags, uint32_t, (uint32_t, uint32_t, uint32_t))
 DEF_HELPER_0_1(exception, void, (uint32_t))
+#ifndef CONFIG_USER_ONLY
 DEF_HELPER_0_0(wfi, void, (void))
+#endif
 
 DEF_HELPER_0_2(cpsr_write, void, (uint32_t, uint32_t))
 DEF_HELPER_1_0(cpsr_read, uint32_t, (void))
diff -ru -x .svn trunk/target-arm/op_helper.c edit/target-arm/op_helper.c
--- trunk/target-arm/op_helper.c	2008-06-01 12:29:17.000000000 +0200
+++ edit/target-arm/op_helper.c	2008-06-08 17:50:55.000000000 +0200
@@ -247,12 +247,14 @@
     return res;
 }
 
+#ifndef CONFIG_USER_ONLY
 void HELPER(wfi)(void)
 {
     env->exception_index = EXCP_HLT;
     env->halted = 1;
     cpu_loop_exit();
 }
+#endif
 
 void HELPER(exception)(uint32_t excp)
 {
diff -ru -x .svn trunk/target-arm/translate.c edit/target-arm/translate.c
--- trunk/target-arm/translate.c	2008-06-07 13:59:11.000000000 +0200
+++ edit/target-arm/translate.c	2008-06-08 17:50:09.000000000 +0200
@@ -69,7 +69,10 @@
 
 /* These instructions trap after executing, so defer them until after the
    conditional executions state has been updated.  */
+/* for user mode hosted emulation wfi is treated as a no-op */
+#ifndef CONFIG_USER_ONLY
 #define DISAS_WFI 4
+#endif
 #define DISAS_SWI 5
 
 /* XXX: move that elsewhere */
@@ -2619,8 +2622,12 @@
     if ((insn & 0x0fff0fff) == 0x0e070f90
         || (insn & 0x0fff0fff) == 0x0e070f58) {
         /* Wait for interrupt.  */
+        /* for user mode hosted emulation wfi is treated as a no-op */
+        /* FIXME:  for ARMv7 this should always be a no-op */
+#ifndef CONFIG_USER_ONLY
         gen_set_pc_im(s->pc);
         s->is_jmp = DISAS_WFI;
+#endif
         return 0;
     }
     rd = (insn >> 12) & 0xf;
@@ -3499,8 +3506,11 @@
 {
     switch (val) {
     case 3: /* wfi */
+        /* for user mode hosted emulation wfi is treated as a no-op */
+#ifndef CONFIG_USER_ONLY
         gen_set_pc_im(s->pc);
         s->is_jmp = DISAS_WFI;
+#endif
         break;
     case 2: /* wfe */
     case 4: /* sev */
@@ -8712,9 +8722,11 @@
         case DISAS_TB_JUMP:
             /* nothing more to generate */
             break;
+#ifndef CONFIG_USER_ONLY
         case DISAS_WFI:
             gen_helper_wfi();
             break;
+#endif
         case DISAS_SWI:
             gen_exception(EXCP_SWI);
             break;