* [Qemu-devel] Re: qemu fix for ARM systems
2006-09-09 9:39 ` [Qemu-devel] Re: qemu fix for ARM systems Fabrice Bellard
@ 2006-09-09 9:47 ` Justin Fletcher
0 siblings, 0 replies; 2+ messages in thread
From: Justin Fletcher @ 2006-09-09 9:47 UTC (permalink / raw)
To: Fabrice Bellard; +Cc: qemu-devel
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. ]
^ permalink raw reply [flat|nested] 2+ messages in thread