From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1GLzR2-0002nF-6P for qemu-devel@nongnu.org; Sat, 09 Sep 2006 05:47:36 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1GLzR0-0002lR-IG for qemu-devel@nongnu.org; Sat, 09 Sep 2006 05:47:35 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1GLzR0-0002lD-DD for qemu-devel@nongnu.org; Sat, 09 Sep 2006 05:47:34 -0400 Received: from [194.217.242.91] (helo=anchor-post-33.mail.demon.net) by monty-python.gnu.org with esmtp (Exim 4.52) id 1GLzRn-0002q1-23 for qemu-devel@nongnu.org; Sat, 09 Sep 2006 05:48:23 -0400 Date: Sat, 9 Sep 2006 10:47:08 +0100 (BST) From: Justin Fletcher In-Reply-To: <45028BE0.4030000@bellard.org> Message-ID: References: <45028BE0.4030000@bellard.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Subject: [Qemu-devel] Re: qemu fix for ARM systems Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabrice Bellard Cc: qemu-devel@nongnu.org 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 [ 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. ]