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

[Fabrice Bellard <fabrice@bellard.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