| Message ID | 20211012093128.3909859-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] target/s390x: don't double ld_code() when reading instructions | expand |
On 10/12/21 2:31 AM, Alex Bennée wrote: > - case 6: > - insn = (insn << 48) | (ld_code4(env, s, pc + 2) << 16); > + case 6: > + insn = deposit64(insn, 16, 32, ld_code4(env, s, pc + 2)); > break; Looks like some of indentation error? Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 10/12/21 2:31 AM, Alex Bennée wrote: > For the 4 byte instruction case we started doing an ld_code2 and then > reloaded the data with ld_code4 once it was identified as a 4 byte op. > This is confusing for the plugin hooks which are expecting to see > simple sequential loading so end up reporting a malformed 6 byte > instruction buffer. I think the plugin stuff could be more clever, knowing where the read occurs within the sequence. Otherwise, we should simplify the interface so that it is not possible to make this mistake. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 10/12/21 2:31 AM, Alex Bennée wrote: >> For the 4 byte instruction case we started doing an ld_code2 and then >> reloaded the data with ld_code4 once it was identified as a 4 byte op. >> This is confusing for the plugin hooks which are expecting to see >> simple sequential loading so end up reporting a malformed 6 byte >> instruction buffer. > > I think the plugin stuff could be more clever, knowing where the read > occurs within the sequence. Otherwise, we should simplify the > interface so that it is not possible to make this mistake. It's plugin_insn_append which is doing the tracking here so we could extend the interface to include the current pc of the load and make the appropriate adjustments. That said it's a bunch hoops to jump every instruction when we could just as easily add an assert and fix up any cases where we do. I guess it comes down to how prevalent double dipping in the instruction stream is when constructing a translation? What happens if the protection of the code area changes half way through a translation? Could a mapping change in flight? > > > r~ -- Alex Bennée
On 10/12/21 7:52 AM, Alex Bennée wrote: >> I think the plugin stuff could be more clever, knowing where the read >> occurs within the sequence. Otherwise, we should simplify the >> interface so that it is not possible to make this mistake. > > It's plugin_insn_append which is doing the tracking here so we could > extend the interface to include the current pc of the load and make the > appropriate adjustments. That said it's a bunch hoops to jump every > instruction when we could just as easily add an assert and fix up any > cases where we do. I guess it comes down to how prevalent double dipping > in the instruction stream is when constructing a translation? Yes, which is why I suggested simplifying the interface to translate_ld*. It currently takes the DisasContextBase; it could potentially read from pc_next, and increment it. It would completely eliminate the problem you're encountering. > What happens if the protection of the code area changes half way through > a translation? Could a mapping change in flight? No, we hold mmap_lock. r~
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c index a2d6fa5cca..7fc870bbb9 100644 --- a/target/s390x/tcg/translate.c +++ b/target/s390x/tcg/translate.c @@ -6273,21 +6273,20 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s) /* Extract the values saved by EXECUTE. */ insn = s->ex_value & 0xffffffffffff0000ull; - ilen = s->ex_value & 0xf; - op = insn >> 56; + ilen = extract64(s->ex_value, 0, 8); + op = extract64(insn, 56, 8); } else { - insn = ld_code2(env, s, pc); - op = (insn >> 8) & 0xff; + insn = deposit64(0, 48, 16, ld_code2(env, s, pc)); + op = extract64(insn, 56, 8); ilen = get_ilen(op); switch (ilen) { case 2: - insn = insn << 48; break; case 4: - insn = ld_code4(env, s, pc) << 32; + insn = deposit64(insn, 32, 16, ld_code2(env, s, pc + 2)); break; - case 6: - insn = (insn << 48) | (ld_code4(env, s, pc + 2) << 16); + case 6: + insn = deposit64(insn, 16, 32, ld_code4(env, s, pc + 2)); break; default: g_assert_not_reached();
For the 4 byte instruction case we started doing an ld_code2 and then reloaded the data with ld_code4 once it was identified as a 4 byte op. This is confusing for the plugin hooks which are expecting to see simple sequential loading so end up reporting a malformed 6 byte instruction buffer. While we are at it lets clean up some of the shifts with nice deposit/extrac calls. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- target/s390x/tcg/translate.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) -- 2.30.2