[Qemu-devel] ARM load/store multiple bug

[Qemu-devel] ARM load/store multiple bug

[Justin Fletcher]

Hiya,

I have found a bug in the implementation of the load/store multiple 
instructions in ARM (LDM and STM). These are defined in the ARM ARM to 
ignore bits 0 and 1 of the address when the load takes place - that is the 
base register for these operations is always treated as a 32bit aligned 
value (although its value is only rounded internally). This differs from 
the LDR/STR operation which uses the full width of instructions.

In other words :

    MOV   r0, #9
    LDMIA r0, {r1,r2}

Is equivalent to loading r1 with the value at 8, and r2 with the value at 
12. Contrast this with the following :

    MOV   r0, #9
    LDR   r1, [r0]
    LDR   r2, [r0,#4]

which would load r1 with the value at 8, rotated right 8 bits, and r2 with 
the value at 12, rotated right 8 bits.

I have not confirmed the behaviour or the LDR operation, but have found 
problems with the multiple register operations. My solution would be to 
add the equivalent of a BIC instruction in to the target-arm/translate.c 
to clear off the bottom two bits, around line 1695 :

---8<---
                         if (n != 1)
                             gen_op_addl_T1_im(-((n - 1) * 4));
                     }
                 }
                 j = 0;
/* Insert something like gen_op_bicl_T1_im(3); here */
                 for(i=0;i<16;i++) {
                     if (insn & (1 << i)) {
                         if (insn & (1 << 20)) {
---8<---

However, there isn't any such function and I'm unsure how to make that 
change. Any suggestions would be greatfully received.

-- 
Gerph <http://gerph.org/>
... Find answers on the street, in cracks beneath my feet.

Re: [Qemu-devel] ARM load/store multiple bug

[Paul Brook]

> ---8<---
>                          if (n != 1)
>                              gen_op_addl_T1_im(-((n - 1) * 4));
>                      }
>                  }
>                  j = 0;
> /* Insert something like gen_op_bicl_T1_im(3); here */
>                  for(i=0;i<16;i++) {
>                      if (insn & (1 << i)) {
>                          if (insn & (1 << 20)) {
> ---8<---

This is not sufficient. It breaks base register writeback.

I'll also note that the behavior is dependent on alignment traps being 
disabled (and unaligned access on some cores). ie. for linux user mode 
emulation the current behavior is acceptable.

Paul

Re: [Qemu-devel] ARM load/store multiple bug

[Justin Fletcher]

On Sun, 10 Sep 2006, Paul Brook wrote:

>> ---8<---
>>                          if (n != 1)
>>                              gen_op_addl_T1_im(-((n - 1) * 4));
>>                      }
>>                  }
>>                  j = 0;
>> /* Insert something like gen_op_bicl_T1_im(3); here */
>>                  for(i=0;i<16;i++) {
>>                      if (insn & (1 << i)) {
>>                          if (insn & (1 << 20)) {
>> ---8<---
>
> This is not sufficient. It breaks base register writeback.

Doh! Of course, yes.

> I'll also note that the behavior is dependent on alignment traps being
> disabled (and unaligned access on some cores). ie. for linux user mode
> emulation the current behavior is acceptable.

Fair enough; it fails badly on some of the code in the OS I'm running on 
it. With a bit of looking around I decided the easiest way to do the 
operation and maintain the compatibility with the writeback was to 
introduce a pair of new operations for load and store, forced to aligned 
addresses. This is NOT, as you note, ideal because it moves the problem of 
the alignment checks and should probably be done better - I don't know the 
code well enough, I'm afraid, to know the right or best way to do it.

My solution is to add new operations to target-arm/op_mem.h :

----8<----
/* Load from aligned address T1 into T0 (used for LDM) */
#define MEM_LDA_OP(name) \
void OPPROTO glue(op_lda##name,MEMSUFFIX)(void) \
{ \
     /* JRF: Note should raise alignment fault if alignment in use and \
             b0 or b1 set */ \
     T0 = glue(ld##name,MEMSUFFIX)(T1 &~3); \
     FORCE_RET(); \
}
MEM_LDA_OP(l)

#undef MEM_LDA_OP

/* Store aligned address T0 into T1 (used for STM) */
#define MEM_STA_OP(name) \
void OPPROTO glue(op_sta##name,MEMSUFFIX)(void) \
{ \
     /* JRF: Note should raise alignment fault if alignment in use and \
             b0 or b1 set */ \
     glue(st##name,MEMSUFFIX)(T1 &~3, T0); \
     FORCE_RET(); \
}
MEM_STA_OP(l)

#undef MEM_STA_OP
----8<----

And to replace the load and store operations in the LDM/STM implementation 
in target-arm/translate.c to use these, eg :

        if (insn & (1 << 20)) {
          /* load */
          gen_ldst(ldal, s); /* JRF: was ldl */
          if (i == 15)

and :

            gen_movl_T0_reg(s, i);
          }
          gen_ldst(stal, s); /* JRF: was stl */
        }
        j++;

I realise that these are not good for the generic use, but they solve my 
problems and it may solve other people's if they happen to be using a 
similarly constructed OS.

-- 
Gerph <http://gerph.org/>
... Hand to mouth, we're picking up the thread

Re: [Qemu-devel] ARM load/store multiple bug

[Fabrice Bellard]

Note that QEMU supports specific unaligned access handling when using 
the softmmu code. It is possible to implement the ARM specific unaligned 
accesses without slowing down the aligned case. See the mips case with 
do_unaligned_access().

Regards,

Fabrice.

Justin Fletcher wrote:
> On Sun, 10 Sep 2006, Paul Brook wrote:
> 
>>> ---8<---
>>>                          if (n != 1)
>>>                              gen_op_addl_T1_im(-((n - 1) * 4));
>>>                      }
>>>                  }
>>>                  j = 0;
>>> /* Insert something like gen_op_bicl_T1_im(3); here */
>>>                  for(i=0;i<16;i++) {
>>>                      if (insn & (1 << i)) {
>>>                          if (insn & (1 << 20)) {
>>> ---8<---
>>
>>
>> This is not sufficient. It breaks base register writeback.
> 
> 
> Doh! Of course, yes.
> 
>> I'll also note that the behavior is dependent on alignment traps being
>> disabled (and unaligned access on some cores). ie. for linux user mode
>> emulation the current behavior is acceptable.
> 
> 
> Fair enough; it fails badly on some of the code in the OS I'm running on 
> it. With a bit of looking around I decided the easiest way to do the 
> operation and maintain the compatibility with the writeback was to 
> introduce a pair of new operations for load and store, forced to aligned 
> addresses. This is NOT, as you note, ideal because it moves the problem 
> of the alignment checks and should probably be done better - I don't 
> know the code well enough, I'm afraid, to know the right or best way to 
> do it.
> 
> My solution is to add new operations to target-arm/op_mem.h :
> 
> ----8<----
> /* Load from aligned address T1 into T0 (used for LDM) */
> #define MEM_LDA_OP(name) \
> void OPPROTO glue(op_lda##name,MEMSUFFIX)(void) \
> { \
>     /* JRF: Note should raise alignment fault if alignment in use and \
>             b0 or b1 set */ \
>     T0 = glue(ld##name,MEMSUFFIX)(T1 &~3); \
>     FORCE_RET(); \
> }
> MEM_LDA_OP(l)
> 
> #undef MEM_LDA_OP
> 
> /* Store aligned address T0 into T1 (used for STM) */
> #define MEM_STA_OP(name) \
> void OPPROTO glue(op_sta##name,MEMSUFFIX)(void) \
> { \
>     /* JRF: Note should raise alignment fault if alignment in use and \
>             b0 or b1 set */ \
>     glue(st##name,MEMSUFFIX)(T1 &~3, T0); \
>     FORCE_RET(); \
> }
> MEM_STA_OP(l)
> 
> #undef MEM_STA_OP
> ----8<----
> 
> And to replace the load and store operations in the LDM/STM 
> implementation in target-arm/translate.c to use these, eg :
> 
>        if (insn & (1 << 20)) {
>          /* load */
>          gen_ldst(ldal, s); /* JRF: was ldl */
>          if (i == 15)
> 
> and :
> 
>            gen_movl_T0_reg(s, i);
>          }
>          gen_ldst(stal, s); /* JRF: was stl */
>        }
>        j++;
> 
> I realise that these are not good for the generic use, but they solve my 
> problems and it may solve other people's if they happen to be using a 
> similarly constructed OS.
> 

Re: [Qemu-devel] ARM load/store multiple bug

[Paul Brook]

On Sunday 10 September 2006 17:46, Fabrice Bellard wrote:
> Note that QEMU supports specific unaligned access handling when using
> the softmmu code. It is possible to implement the ARM specific unaligned
> accesses without slowing down the aligned case. See the mips case with
> do_unaligned_access().

I think additional bits will be required. The behavior of unaligned accesses 
depends on the instruction performing the access.

Paul