| Message ID | 20190819213755.26175-19-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Convert aa32 base isa to decodetree | expand |
On Mon, 19 Aug 2019 at 22:38, Richard Henderson <richard.henderson@linaro.org> wrote: In subject, typo: "Miscellaneous". > This fixes an exiting bug with the T5 encoding of SUBS PC, LR, #IMM, "existing" > in that it may be executed from user mode as with any other encoding > of SUBS, not as ERET. Should this paragraph be in the commit message for the previous patch? This change doesn't touch SUBS/ERET. Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 8/23/19 5:03 AM, Peter Maydell wrote: > On Mon, 19 Aug 2019 at 22:38, Richard Henderson > <richard.henderson@linaro.org> wrote: > > In subject, typo: "Miscellaneous". > >> This fixes an exiting bug with the T5 encoding of SUBS PC, LR, #IMM, > > "existing" > >> in that it may be executed from user mode as with any other encoding >> of SUBS, not as ERET. > > Should this paragraph be in the commit message for the previous > patch? This change doesn't touch SUBS/ERET. > > Otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Indeed that text should not be here. And also, as you might be able to tell from the text of the previous patch, it isn't an existing bug. It took me a while to see that, and that's why I think that passing pre-v7m through the usual SUBS path is clearer. r~
On Mon, 19 Aug 2019 at 22:38, Richard Henderson <richard.henderson@linaro.org> wrote: > > This fixes an exiting bug with the T5 encoding of SUBS PC, LR, #IMM, > in that it may be executed from user mode as with any other encoding > of SUBS, not as ERET. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/translate.c | 119 +++++++++++++---------------------------- > target/arm/a32.decode | 8 +++ > target/arm/t32.decode | 5 ++ > 3 files changed, 50 insertions(+), 82 deletions(-) > +static bool trans_HVC(DisasContext *s, arg_HVC *a) > +{ > + if (!ENABLE_ARCH_7 || IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) { > + return false; > + } > + gen_hvc(s, a->imm); > + return true; > +} I was wondering about for these trans_ functions the difference between returning 'false' and calling unallocated_encoding() and then returning 'true'. If I understand the decodetree right this will only make a difference when the pattern is inside a {} group. So for instance here we have { [...] { HVC 1111 0111 1110 .... 1000 .... .... .... \ &i imm=%imm16_16_0 CPS 1111 0011 1010 1111 1000 0 imod:2 M:1 A:1 I:1 F:1 mode:5 \ &cps UDF 1111 0111 1111 ---- 1010 ---- ---- ---- } B_cond_thumb 1111 0. cond:4 ...... 10.0 ............ &ci imm=%imm21 } which means that if the HVC returns 'false' we'll end up trying the insn as a B_cond_thumb. In this case the trans function for the B_cond_thumb pattern will correctly return false itself for a->cond >= 0xe, so it makes no difference. But maybe it would be more robust for a pattern like HVC to be self-contained so it doesn't fall through for cases that really do belong to it but happen to be required to UNDEF (like IS_USER() == true) ? OTOH I suppose you could say that when you're writing patterns like the B_cond_thumb one you know you've underdecoded and must catch all the theoretical overlaps by writing checks in the trans function, so as long as you do that correctly you're fine. I guess this isn't a request for a change, just wondering if there is a general principle for how we should structure this sort of thing. I didn't run into it with the VFP stuff because none of the decode needed groups. thanks -- PMM
On 8/27/19 3:32 AM, Peter Maydell wrote: >> +static bool trans_HVC(DisasContext *s, arg_HVC *a) >> +{ >> + if (!ENABLE_ARCH_7 || IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) { >> + return false; >> + } >> + gen_hvc(s, a->imm); >> + return true; >> +} > > I was wondering about for these trans_ functions the > difference between returning 'false' and calling > unallocated_encoding() and then returning 'true'. > If I understand the decodetree right this will only > make a difference when the pattern is inside a {} group. Correct. > So for instance here we have > > { > [...] > { > HVC 1111 0111 1110 .... 1000 .... .... .... \ > &i imm=%imm16_16_0 > CPS 1111 0011 1010 1111 1000 0 imod:2 M:1 A:1 I:1 F:1 mode:5 \ > &cps > UDF 1111 0111 1111 ---- 1010 ---- ---- ---- > } > B_cond_thumb 1111 0. cond:4 ...... 10.0 ............ &ci imm=%imm21 > } > > which means that if the HVC returns 'false' we'll end up > trying the insn as a B_cond_thumb. Correct. > In this case the > trans function for the B_cond_thumb pattern will correctly > return false itself for a->cond >= 0xe, so it makes no > difference. But maybe it would be more robust for a pattern > like HVC to be self-contained so it doesn't fall through > for cases that really do belong to it but happen to be > required to UNDEF (like IS_USER() == true) ? I agree this should be the rule. E.g. for this IS_USER case, we have successfully decoded HVC and so should not return false. The fact that HVC should raise SIGILL if IS_USER should not be confused with decoding HVC. Other constraints, such as rd != 15 or imod != 0, should continue to return false so that a (potential) grouped insn can match. > OTOH I suppose you could say that when you're writing patterns > like the B_cond_thumb one you know you've underdecoded and must > catch all the theoretical overlaps by writing checks in the trans > function, so as long as you do that correctly you're fine. Yes. r~
On 8/27/19 1:01 PM, Richard Henderson wrote: > Other constraints, such as rd != 15 or imod != 0, should continue to return > false so that a (potential) grouped insn can match. Eh. This is not the answer that the TT example suggests. So far we are able to order the grouped insns such that decoding directives like if t == 15 then SEE "TT"; are respected. Since we do not generally do a very good job of diagnosing all of the UNPREDICTABLE behavior, we should not rely on getting all of it, e.g. by requiring that if TT diagnoses some UNPRED that STREX also diagnoses similar UNPRED. I'm going to walk through the patch set and fix these. r~
diff --git a/target/arm/translate.c b/target/arm/translate.c index cb7b35489f..cb6296dc12 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -8491,6 +8491,39 @@ static bool trans_ERET(DisasContext *s, arg_ERET *a) return true; } +static bool trans_HLT(DisasContext *s, arg_HLT *a) +{ + gen_hlt(s, a->imm); + return true; +} + +static bool trans_BKPT(DisasContext *s, arg_BKPT *a) +{ + if (!ENABLE_ARCH_5) { + return false; + } + gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false)); + return true; +} + +static bool trans_HVC(DisasContext *s, arg_HVC *a) +{ + if (!ENABLE_ARCH_7 || IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) { + return false; + } + gen_hvc(s, a->imm); + return true; +} + +static bool trans_SMC(DisasContext *s, arg_SMC *a) +{ + if (!ENABLE_ARCH_6K || IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) { + return false; + } + gen_smc(s); + return true; +} + /* * Legacy decoder. */ @@ -8771,68 +8804,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn) } else if ((insn & 0x0f900000) == 0x01000000 && (insn & 0x00000090) != 0x00000090) { /* miscellaneous instructions */ - op1 = (insn >> 21) & 3; - sh = (insn >> 4) & 0xf; - rm = insn & 0xf; - switch (sh) { - case 0x0: - /* MSR/MRS (banked/register) */ - /* All done in decodetree. Illegal ops already signalled. */ - g_assert_not_reached(); - case 0x1: /* bx, clz */ - case 0x2: /* bxj */ - case 0x3: /* blx */ - case 0x4: /* crc32 */ - /* All done in decodetree. Illegal ops reach here. */ - goto illegal_op; - case 0x5: /* Saturating addition and subtraction. */ - case 0x6: /* ERET */ - /* All done in decodetree. Reach here for illegal ops. */ - goto illegal_op; - case 7: - { - int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4); - switch (op1) { - case 0: - /* HLT */ - gen_hlt(s, imm16); - break; - case 1: - /* bkpt */ - ARCH(5); - gen_exception_bkpt_insn(s, syn_aa32_bkpt(imm16, false)); - break; - case 2: - /* Hypervisor call (v7) */ - ARCH(7); - if (IS_USER(s)) { - goto illegal_op; - } - gen_hvc(s, imm16); - break; - case 3: - /* Secure monitor call (v6+) */ - ARCH(6K); - if (IS_USER(s)) { - goto illegal_op; - } - gen_smc(s); - break; - default: - g_assert_not_reached(); - } - break; - } - case 0x8: - case 0xa: - case 0xc: - case 0xe: - /* Halfword multiply and multiply accumulate. */ - /* All done in decodetree. Reach here for illegal ops. */ - goto illegal_op; - default: - goto illegal_op; - } + /* All done in decodetree. Illegal ops reach here. */ + goto illegal_op; } else if (((insn & 0x0e000000) == 0 && (insn & 0x00000090) != 0x90) || ((insn & 0x0e000000) == (1 << 25))) { @@ -10493,26 +10466,8 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn) goto illegal_op; if (insn & (1 << 26)) { - if (arm_dc_feature(s, ARM_FEATURE_M)) { - goto illegal_op; - } - if (!(insn & (1 << 20))) { - /* Hypervisor call (v7) */ - int imm16 = extract32(insn, 16, 4) << 12 - | extract32(insn, 0, 12); - ARCH(7); - if (IS_USER(s)) { - goto illegal_op; - } - gen_hvc(s, imm16); - } else { - /* Secure monitor call (v6+) */ - ARCH(6K); - if (IS_USER(s)) { - goto illegal_op; - } - gen_smc(s); - } + /* hvc, smc, in decodetree */ + goto illegal_op; } else { op = (insn >> 20) & 7; switch (op) { diff --git a/target/arm/a32.decode b/target/arm/a32.decode index 52a66dd1d5..c7f156be6d 100644 --- a/target/arm/a32.decode +++ b/target/arm/a32.decode @@ -31,6 +31,7 @@ &rrr rd rn rm &rr rd rm &r rm +&i imm &msr_reg rn r mask &mrs_reg rd r &msr_bank rn r sysm @@ -196,9 +197,11 @@ CRC32CW .... 0001 0100 .... .... 0010 0100 .... @rndm # Miscellaneous instructions %sysm 8:1 16:4 +%imm16_8_0 8:12 0:4 @rm ---- .... .... .... .... .... .... rm:4 &r @rdm ---- .... .... .... rd:4 .... .... rm:4 &rr +@i16 ---- .... .... .... .... .... .... .... &i imm=%imm16_8_0 MRS_bank ---- 0001 0 r:1 00 .... rd:4 001. 0000 0000 &mrs_bank %sysm MSR_bank ---- 0001 0 r:1 10 .... 1111 001. 0000 rn:4 &msr_bank %sysm @@ -213,3 +216,8 @@ BLX_r .... 0001 0010 1111 1111 1111 0011 .... @rm CLZ .... 0001 0110 1111 .... 1111 0001 .... @rdm ERET ---- 0001 0110 0000 0000 0000 0110 1110 + +HLT .... 0001 0000 .... .... .... 0111 .... @i16 +BKPT .... 0001 0010 .... .... .... 0111 .... @i16 +HVC .... 0001 0100 .... .... .... 0111 .... @i16 +SMC ---- 0001 0110 0000 0000 0000 0111 imm:4 &i diff --git a/target/arm/t32.decode b/target/arm/t32.decode index 6236d28b99..5116c6165a 100644 --- a/target/arm/t32.decode +++ b/target/arm/t32.decode @@ -28,6 +28,7 @@ &rrr !extern rd rn rm &rr !extern rd rm &r !extern rm +&i !extern imm &msr_reg !extern rn r mask &mrs_reg !extern rd r &msr_bank !extern rn r sysm @@ -189,6 +190,7 @@ CLZ 1111 1010 1011 ---- 1111 .... 1000 .... @rdm %msr_sysm 4:1 8:4 %mrs_sysm 4:1 16:4 +%imm16_16_0 16:4 0:12 { { @@ -226,4 +228,7 @@ CLZ 1111 1010 1011 ---- 1111 .... 1000 .... @rdm SUB_rri 1111 0011 1101 1110 1000 1111 imm:8 \ &s_rri_rot rot=0 s=1 rd=15 rn=14 } + SMC 1111 0111 1111 imm:4 1000 0000 0000 0000 &i + HVC 1111 0111 1110 .... 1000 .... .... .... \ + &i imm=%imm16_16_0 }
This fixes an exiting bug with the T5 encoding of SUBS PC, LR, #IMM, in that it may be executed from user mode as with any other encoding of SUBS, not as ERET. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/translate.c | 119 +++++++++++++---------------------------- target/arm/a32.decode | 8 +++ target/arm/t32.decode | 5 ++ 3 files changed, 50 insertions(+), 82 deletions(-) -- 2.17.1