From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1GLzJ9-0003V8-9g for qemu-devel@nongnu.org; Sat, 09 Sep 2006 05:39:27 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1GLzJ8-0003Ra-7X for qemu-devel@nongnu.org; Sat, 09 Sep 2006 05:39:26 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1GLzJ8-0003RF-53 for qemu-devel@nongnu.org; Sat, 09 Sep 2006 05:39:26 -0400 Received: from [84.96.92.56] (helo=smTp.neuf.fr) by monty-python.gnu.org with esmtp (Exim 4.52) id 1GLzJu-00024x-Uk for qemu-devel@nongnu.org; Sat, 09 Sep 2006 05:40:15 -0400 Received: from [84.102.211.78] by sp604003mt.gpm.neuf.ld (Sun Java System Messaging Server 6.2-5.05 (built Feb 16 2006)) with ESMTP id <0J5B00BDPK5EDM80@sp604003mt.gpm.neuf.ld> for qemu-devel@nongnu.org; Sat, 09 Sep 2006 11:39:15 +0200 (CEST) Date: Sat, 09 Sep 2006 11:39:44 +0200 From: Fabrice Bellard In-reply-to: Message-id: <45028BE0.4030000@bellard.org> MIME-version: 1.0 Content-type: text/plain; charset=us-ascii; format=flowed Content-transfer-encoding: 7BIT References: 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: Justin Fletcher Cc: qemu-devel@nongnu.org Hi, You should send your patches to the qemu-devel mailing list. The correct implementation for cache operations in QEMU is to do nothing because QEMU guaranties memory coherency, so you should remove tb_flush(). 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. Regards, Fabrice. 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