[Qemu-devel] [Bug 670883] Re: ARM : ldrexd and strexd implementation flawed

[Aurelien Jarno]

** Changed in: qemu
       Status: Fix Committed => Fix Released

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/670883

Title:
  ARM : ldrexd and strexd implementation flawed

Status in QEMU:
  Fix Released

Bug description:
  The ldrexd / strexd instructions have a flawed implementation : it
  never works properly.  For example, the most simple code something
  like:

  ldrexd r0, r1, [r2]
  strexd r4, r0, r1, [r2]

  which should usually have r4 as zero (successfully done the exclusive
  store).  However, the current implementation always returns one
  (unsuccessfully done the exclusive store).

  The current trunk (function gen_load_exclusive, gen_store_exclusive,
  both from target-arm/translate.c) looks like an incorrect
  implementation:

  static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
                                 TCGv addr, int size)
  {
      TCGv tmp;

      switch (size) {
      case 0:
          tmp = gen_ld8u(addr, IS_USER(s));
          break;
      case 1:
          tmp = gen_ld16u(addr, IS_USER(s));
          break;
      case 2:
      case 3:
          tmp = gen_ld32(addr, IS_USER(s));
          break;
      default:
          abort();
      }
      tcg_gen_mov_i32(cpu_exclusive_val, tmp);
      store_reg(s, rt, tmp);
      if (size == 3) {
          tcg_gen_addi_i32(addr, addr, 4);
          tmp = gen_ld32(addr, IS_USER(s));
          tcg_gen_mov_i32(cpu_exclusive_high, tmp);
          store_reg(s, rt2, tmp);
      }
      tcg_gen_mov_i32(cpu_exclusive_addr, addr);
  }

  The problem lies when size is 3 (=ldrexd) : normally,
  cpu_exclusive_addr should be set as addr, but since the current
  implementation increments addr by 4 (when size is 3) before
  cpu_exclusive_addr is updated, it results in a wrong value stored on
  cpu_exclusive_addr.

  Another error on gen_store_exclusive():

  ...
      if (size == 3) {
          TCGv tmp2 = new_tmp();
          tcg_gen_addi_i32(tmp2, addr, 4);
          tmp = gen_ld32(addr, IS_USER(s));
          dead_tmp(tmp2);
          tcg_gen_brcond_i32(TCG_COND_NE, tmp, cpu_exclusive_high, fail_label);
          dead_tmp(tmp);
      }
  ....

  the current code assigns tmp2 as addr + 4, but loads from addr on the
  next line, not from tmp2(=addr+4).