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

[Justin Fletcher <gerph@gerph.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 <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. ]