[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] target-arm: implement LDA/STL instructions
|
From: |
Peter Maydell |
|
Subject: |
Re: [Qemu-devel] [PATCH v2] target-arm: implement LDA/STL instructions |
|
Date: |
Thu, 20 Jun 2013 18:51:27 +0100 |
On 17 June 2013 17:50, Mans Rullgard <address@hidden> wrote:
> This adds support for the ARMv8 load acquire/store release instructions.
> Since qemu does nothing special for memory barriers, these can be
> emulated like their non-acquire/release counterparts.
Couple more minor issues, otherwise looks good.
> addr = tcg_temp_local_new_i32();
> load_reg_var(s, addr, rn);
> - if (insn & (1 << 20)) {
> +
> + /* Since the emulation does not have barriers,
> + the acquire/release semantics need no special
> + handling */
> + if (op2 == 0) {
> + tmp = tcg_temp_new_i32();
This line needs to go inside the next if(), otherwise we leak a temp
in the stl/stlb/stlh case. (load_reg() does a temp_new for you, so
you need to pair temp_new/store_reg and load_reg/temp_free.)
> + if (insn & (1 << 20)) {
> + switch (op1) {
> + case 0: /* lda */
> + tcg_gen_qemu_ld32u(tmp, addr,
> IS_USER(s));
> + break;
> + case 2: /* ldab */
> + tcg_gen_qemu_ld8u(tmp, addr, IS_USER(s));
> + break;
> + case 3: /* ldah */
> + tcg_gen_qemu_ld16u(tmp, addr,
> IS_USER(s));
> + break;
> + default:
> + abort();
> + }
> + store_reg(s, rd, tmp);
> + } else {
> + rm = insn & 0xf;
> + tmp = load_reg(s, rm);
> + switch (op1) {
> + case 0: /* stl */
> + tcg_gen_qemu_st32(tmp, addr, IS_USER(s));
> + break;
> + case 2: /* stlb */
> + tcg_gen_qemu_st8(tmp, addr, IS_USER(s));
> + break;
> + case 3: /* stlh */
> + tcg_gen_qemu_st16(tmp, addr, IS_USER(s));
> + break;
> + default:
> + abort();
> + }
> + tcg_temp_free_i32(tmp);
> + }
> + } else if (insn & (1 << 20)) {
> switch (op1) {
> case 0: /* ldrex */
> gen_load_exclusive(s, rd, 15, addr, 2);
> @@ -8152,15 +8210,63 @@ static int disas_thumb2_insn(CPUARMState *env,
> DisasContext *s, uint16_t insn_hw
> tcg_gen_addi_i32(tmp, tmp, s->pc);
> store_reg(s, 15, tmp);
> } else {
> - /* Load/store exclusive byte/halfword/doubleword. */
> - ARCH(7);
> + int op2 = (insn >> 6) & 0x3;
> op = (insn >> 4) & 0x3;
> - if (op == 2) {
> + switch (op2) {
> + case 0:
> goto illegal_op;
> + case 1:
> + /* Load/store exclusive byte/halfword/doubleword */
> + ARCH(7);
> + break;
> + case 2:
> + /* Load-acquire/store-release */
> + if (op == 3) {
> + goto illegal_op;
> + }
> + /* Fall through */
> + case 3:
> + /* Load-acquire/store-release exclusive */
> + ARCH(8);
> + break;
> }
This change has lost the check for op==2 being illegal in the
load/store exclusive case (ie case op2==1).
> addr = tcg_temp_local_new_i32();
> load_reg_var(s, addr, rn);
> - if (insn & (1 << 20)) {
> + if (!(op2 & 1)) {
> + tmp = tcg_temp_new_i32();
This needs to be inside the following if(), otherwise we leak
a temp in the stlb/stlh/stl case.
> + if (insn & (1 << 20)) {
> + switch (op) {
> + case 0: /* ldab */
> + tcg_gen_qemu_ld8u(tmp, addr, IS_USER(s));
> + break;
> + case 1: /* ldah */
> + tcg_gen_qemu_ld16u(tmp, addr, IS_USER(s));
> + break;
> + case 2: /* lda */
> + tcg_gen_qemu_ld32u(tmp, addr, IS_USER(s));
> + break;
> + default:
> + abort();
> + }
> + store_reg(s, rs, tmp);
> + } else {
> + tmp = load_reg(s, rs);
> + switch (op) {
> + case 0: /* stlb */
> + tcg_gen_qemu_st8(tmp, addr, IS_USER(s));
> + break;
> + case 1: /* stlh */
> + tcg_gen_qemu_st16(tmp, addr, IS_USER(s));
> + break;
> + case 2: /* stl */
> + tcg_gen_qemu_st32(tmp, addr, IS_USER(s));
> + break;
> + default:
> + abort();
> + }
> + tcg_temp_free_i32(tmp);
> + }
> + } else if (insn & (1 << 20)) {
> gen_load_exclusive(s, rs, rd, addr, op);
> } else {
> gen_store_exclusive(s, rm, rs, rd, addr, op);
thanks
-- PMM