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(-)

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();
-- 
2.30.2

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~