[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] target-arm: implement LDA/STL instructions
|
From: |
Peter Maydell |
|
Subject: |
Re: [Qemu-devel] [PATCH 2/3] target-arm: implement LDA/STL instructions |
|
Date: |
Thu, 13 Jun 2013 15:01:02 +0100 |
On 7 June 2013 13:06, 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.
A brief comment to this effect in the appropriate parts of the
code as well as the commit message would be good.
>
> Signed-off-by: Mans Rullgard <address@hidden>
> ---
> target-arm/translate.c | 91
> ++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 85 insertions(+), 6 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 96ac5bc..f529257 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7274,14 +7274,54 @@ static void disas_arm_insn(CPUARMState * env,
> DisasContext *s)
> rd = (insn >> 12) & 0xf;
> if (insn & (1 << 23)) {
> /* load/store exclusive */
> + int excl = (insn >> 9) & 1;
> op1 = (insn >> 21) & 0x3;
> - if (op1)
> + if (!excl)
> + ARCH(8);
> + else if (op1)
> ARCH(6K);
> else
> ARCH(6);
This if statement needs braces on all its arms. (QEMU coding
style mandates braces, though some existing code is not in
line with this; our general policy is that where patches touch
such existing code they update it accordingly.)
You can use scripts/checkpatch.pl to check for this.
This is also continuing to underdecode this space: some
of the new exclusive insns are ARCH(8) only but will be
allowed on ARCH(6) or 6K by this check. I know we've historically
underdecoded, but if we're touching this bit of the decoder
I'd prefer it if we tightened it up in the process.
> addr = tcg_temp_local_new_i32();
> load_reg_var(s, addr, rn);
> - if (insn & (1 << 20)) {
> + if (!excl) {
> + if (op1 == 1)
> + goto illegal_op;
This check needs to happen earlier to avoid leaking a tcg temp.
> + tmp = tcg_temp_new_i32();
> + 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);
> @@ -8126,7 +8166,7 @@ static int disas_thumb2_insn(CPUARMState *env,
> DisasContext *s, uint16_t insn_hw
> gen_store_exclusive(s, rd, rs, 15, addr, 2);
> }
> tcg_temp_free_i32(addr);
> - } else if ((insn & (1 << 6)) == 0) {
> + } else if ((insn & (3 << 6)) == 0) {
While we're fiddling with the decode here let's actually narrow
it down completely, by making this condition
((insn & (7 << 5)) == 0)
...
> /* Table Branch. */
> if (rn == 15) {
> addr = tcg_temp_new_i32();
> @@ -8153,14 +8193,53 @@ static int disas_thumb2_insn(CPUARMState *env,
> DisasContext *s, uint16_t insn_hw
> store_reg(s, 15, tmp);
> } else {
> /* Load/store exclusive byte/halfword/doubleword. */
> - ARCH(7);
> + if (((insn >> 6) & 3) != 1)
> + ARCH(8);
> + else
> + ARCH(7);
...and then here do something like:
switch ((insn >> 6) & 3) {
case 0:
goto illegal_op;
case 1:
/* Load/store exclusive byte/halfword/doubleword */
ARCH(7);
break;
case 2:
case 3:
/* Load-acquire/store-release */
ARCH(8);
break;
}
> op = (insn >> 4) & 0x3;
> - if (op == 2) {
> + if (((insn >> 7) & 1) == 0 && op == 2) {
> goto illegal_op;
> }
It might be better to just pull out the op3 field
and then do tests on it rather than combinations of
testing op (op3's lower 2 bits) and bits directly from
insn.
> addr = tcg_temp_local_new_i32();
> load_reg_var(s, addr, rn);
> - if (insn & (1 << 20)) {
> + if ((insn & (1 << 6)) == 0) {
> + if (op == 3)
> + goto illegal_op;
This check needs to go earlier, otherwise we leak a tcg temp
if we take the illegal_op path. You might be able to roll all
the illegal-op checks and ARCH checks into a single switch on
the whole op3 field.
> + tmp = tcg_temp_new_i32();
> + 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);
Have you tested these with risu yet?
thanks
-- PMM