qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-devel] Re: qemu fix for ARM systems


From: Justin Fletcher
Subject: [Qemu-devel] Re: qemu fix for ARM systems
Date: Sat, 9 Sep 2006 10:47:08 +0100 (BST)

On Sat, 9 Sep 2006, Fabrice Bellard wrote:

Hi,

You should send your patches to the qemu-devel mailing list.

Fair enough; as I mentioned, I couldn't find any contact address in the documentation, so I used the one from the website.

The correct implementation for cache operations in QEMU is to do nothing because QEMU guaranties memory coherency, so you should remove tb_flush().

If you say so; without the flushes dynamic decompression code used in our executable programs has not functioned at all and results in the previously compiled code being executed.

Regarding the N and Z flags, there should be a discussion in the mailing list. I feel your solution is not more satisfactory than the current one. Maybe slowing down QEMU a bit would be acceptable by using different variables for Z and N.

The present solution uses N as the primary flags - if N is set then the value is non-zero. Within the OS I'm using, the N flag's value is seldom
intentionally changed, but the Z flag is regularly manipulated.

Justin Fletcher wrote:
Hiya,

I've been using qemu to test an ARM OS with. I hope this is the right
address to contact with patches; I could not find a contact address in
the supplied documentation.

The ARM emulation is probably only tested against Linux so there are
a couple of things which do not function correctly in the OS I am
working against.

The first of these is to make a big note and change to the manner in
which the Z and N flags function. In the OS which I am porting there are
a number of places where the PSR flags are directly modified to set
or clear the Z flag. In the current ARM emulation if the N flag is set,
the Z flag is treated as clear (presumably because a value cannot be
negative and zero). The change in cpu.h annotates this and changes the
Z flag to force the N flag clear. The Z and N cannot become set
simultaneously by any arithmetic operations but only by directly
modifying the PSR with an MSR instruction. Thus, such things will
probably not occur within a linux OS.

The second change is to the check_ap() function which incorrectly
checks the access permissions. The access permissions are a little
confusing, but I believe that this code has a simple inverted
condition. 'access_type' is 0 for read, 1 for write, and 2 for
a prefetch as far as I can tell. This being the case, the current
emulation is 'if (access_type != 1) return 0;' which means 'for
everything but write, fail'. This is the opposite of what we want.
If the AP = 0 then write is NEVER allowed, and read is dependant
on the S and R bits (bits 8 and 9 which are checked by the
subsequent switch). Thus, this check should be reversed. In the OS
I'm booting, the main code is marked as read only in all modes
through this access permission setting method. With the original
code the OS would halt after the change because the code being
executed would suddenly be inaccessible, and then the abort code
would be called and also be found to be inaccessible.

There's a minor typo fix for 'disabled' in there which I must have
changed and haven't noticed until these diffs.

And finally, I've added some rudimentary cache control
instructions in order to flush caches. I'm unsure whether the
flushing of the translation buffer (tb_flush()) is safe within
those operations, but I've done it there and the OS boots without
visible side effects. The OS is quite heavy on the dynamic code (!)
so these operations are vital to ensure that the instructions
that are compiled are the same as those which are being executed.
I could be misunderstanding the way in which the execution works,
but dynamic decompression code within the programs I was running
on the emulated environment was failing without these.

I have seen that 0.8.2 has been released but does not seem from the
changelog to contain any ARM-specific fixes so I assume that these
things have affected nobody else. I hope that they will be easy to
incorporate into a future version. Obviously I'm happy for any
changes to be made to the patch - I think that the indentation
format doesn't match the one you use (sorry), and you may wish to
remove my 'JRF' annotations, which are mostly to remind me which
bits I've changed.

Hope that helps anyhow.


------------------------------------------------------------------------

Only in qemu-0.8.1: armeb-user
Only in qemu-0.8.1: arm-softmmu
Only in qemu-0.8.1: armtest
Only in qemu-0.8.1: arm-user
Only in qemu-0.8.1: config-host.h
Only in qemu-0.8.1: config-host.mak
Only in qemu-0.8.1: dyngen
Only in qemu-0.8.1: i386-softmmu
Only in qemu-0.8.1: i386-user
Only in qemu-0.8.1: mipsel-softmmu
Only in qemu-0.8.1: mipsel-user
Only in qemu-0.8.1: mips-softmmu
Only in qemu-0.8.1: mips-user
Only in qemu-0.8.1: ppc-softmmu
Only in qemu-0.8.1: ppc-user
Only in qemu-0.8.1: qemu-img
Only in qemu-0.8.1: sparc-softmmu
Only in qemu-0.8.1: sparc-user
diff -r -wu qemu-0.8.1-original/target-arm/cpu.h qemu-0.8.1/target-arm/cpu.h --- qemu-0.8.1-original/target-arm/cpu.h 2006-05-03 21:32:58.000000000 +0100
+++ qemu-0.8.1/target-arm/cpu.h 2006-08-15 19:02:35.000000000 +0100
@@ -164,6 +164,20 @@
 {
     /* NOTE: N = 1 and Z = 1 cannot be stored currently */
     if (mask & CPSR_NZCV) {
+/* JRF: The N=1,Z=1 problem makes for real pain when the system is directly + modifying the PSR values without regard for this particular case - it
+        is quite common to directly manipulate the PSR to set Z, ignoring
+        that N is already set (because, well, the ARM doesn't care). This
+        change forces the Z flag to over-ride the N flag. If Z is set, N
+        becomes clear. This should just get things working which were
+        attempting to modify just the Z flag. Modifying the N flag through
+ the PSR operations is rarer, and seldom expected - HOWEVER it might + happen and this limitation of the emulator just has to be accepted. */
+        if (val & (1u<<30)) /* Z set ? */
+          val &= ~(1u<<31); /* if so, no N */
+        /* Of course, we could raise a warning here, but the results would
+           be so spamtastic that I really don't think it's wise. */
+/* End JRF */
         env->NZF = (val & 0xc0000000) ^ 0x40000000;
         env->CF = (val >> 29) & 1;
         env->VF = (val << 3) & 0x80000000;
diff -r -wu qemu-0.8.1-original/target-arm/helper.c qemu-0.8.1/target-arm/helper.c --- qemu-0.8.1-original/target-arm/helper.c 2006-05-03 21:32:58.000000000 +0100
+++ qemu-0.8.1/target-arm/helper.c      2006-09-07 22:12:50.000000000 +0100
@@ -239,7 +239,14 @@
    switch (ap) {
   case 0:
+ /* JRF: This is wrong:
       if (access_type != 1)
+         Which would guarentee that reads (=0) and prefetch (=2) would
+         always be faulted, and writes would obey the S and R bits.
+         Which is quite the reverse of what we should be doing.
+         New code :
+  */
+      if (access_type == 1)
           return 0;
       switch ((env->cp15.c1_sys >> 8) & 3) {
       case 1:
@@ -279,7 +286,7 @@
         address += env->cp15.c13_fcse;
      if ((env->cp15.c1_sys & 1) == 0) {
-        /* MMU diusabled.  */
+        /* MMU disabled.  */
         *phys_ptr = address;
         *prot = PAGE_READ | PAGE_WRITE;
     } else {
@@ -449,6 +456,70 @@
         break;
     case 7: /* Cache control.  */
         /* No cache, so nothing to do.  */
+/* JRF: Instruction cache flushing */
+        {
+          uint32_t crm = insn & 15;
+          switch (crm)
+          {
+            case 0:
+              if (op2 == 4)
+              {
+                /* Wait for interrupt */
+              }
+              break;
+
+            case 5:
+              /* Instruction operations:
+                    0 = invalidate entire icache
+                    1 = invalidate icache line by VA
+                    2 = invalidate icache line by set/index
+                    4 = flush prefetch buffer
+                    6 = flush entire branch target cache
+                    7 = flush branch target cache entry
+               */
+              /* printf("CP15: ICache invalidate\n"); */
+              /* ??? Is this safe when called from within a TB?  */
+              tb_flush(env);
+              break;
+
+            case 6:
+              /* Data operations:
+                    0 = invalidate entire dcache
+                    1 = invalidate dcache line by VA
+                    2 = invalidate dcache line by set/index
+               */
+              break;
+
+            case 7:
+              /* Unified cache operations:
+ 0 = invalidate entire unified cache, or both icache and
+                        dcache
+                    1 = invalidate unified cache line by VA
+                    2 = invalidate unified cache line by set/index
+               */
+              /* ??? Is this safe when called from within a TB?  */
+              /* printf("CP15: UCache invalidate\n"); */
+              tb_flush(env);
+              break;
+
+            case 8:
+              if (op2 == 2)
+              {
+                /* Wait for interrupt, deprecated encoding (already
+                   handled in translate.c) */
+              }
+              break;
+
+            /* Others not important to me...
+                   10 = clean / drain dcache operations
+                   11 = clean dcache operations
+                   13 = prefetch operations
+                   14 = clean and invalidate dcache
+                   15 = clean and invalidate unified cache
+             */
+          }
+        }
+/*      End patch */
         break;
     case 8: /* MMU TLB control.  */
         switch (op2) {
Only in qemu-0.8.1: x86_64-softmmu


--
Gerph <http://gerph.org/>
[ All information, speculation, opinion or data within, or attached to,
  this email is private and confidential. Such content may not be
  disclosed to third parties, or a public forum, without explicit
  permission being granted. ]




reply via email to

[Prev in Thread] Current Thread [Next in Thread]