Re: [RFC PATCH] target/s390x: fake instruction loading when handling 'ex'

[Richard Henderson <richard.henderson@linaro.org>]

On 10/19/22 21:35, Alex Bennée wrote:
> The s390x EXecute instruction is a bit weird as we synthesis the
> executed instruction from what we have stored in memory. When plugins
> are enabled this breaks because we detect the ld_code2() loading from
> a non zero offset without the rest of the instruction being there.

Hmm.  The fact that you see an ld_code2 at all is incorrect.

(1) translator_lduw, which ld_code2 is using, is supposed to
     be only for instructions that we're executing, per plugins.
     But the usage you are seeing is a probe for the next insn,
     which should not be included.

(2) We always exit the tb after EX, so the probe for the next
     insn is also wrong.  We've got the tests in the wrong order:

         if (!is_same_page(dcbase, dc->base.pc_next) ||
             !is_same_page(dcbase, get_next_pc(env, dc, dc->base.pc_next)) ||
             dc->ex_value) {

None of that takes away from the fact that we *should* have a way to report the EX 
instruction to the plugin.


> +/**
> + * translator_fake_ldw - fake instruction load
> + * @insn16: 2 byte instruction
> + * @pc: program counter of instruction
> + *
> + * This is a special case helper used where the instruction we are
> + * about to translate comes from somewhere else (e.g. being
> + * re-synthesised for s390x "ex"). It ensures we update other areas of
> + * the translator with details of the executed instruction.
> + */
> +
> +static inline void translator_fake_ldw(uint16_t insn16, abi_ptr pc)
> +{
> +    plugin_insn_append(pc, &insn16, sizeof(insn16));
> +}

You're not handing the endianness of the two bytes.
I think you should just decompose all the way to fake_ldb.

I don't know much sense you're going to get out of the PC.  The EX instruction is at a 
particular address A, having loaded data from B.  The EX instruction is 4 bytes, but the 
insn at B may be 6 bytes.  The next insn executed may well be PC = A+4, apparently 
overlapping with the 6 byte insn you gave to the plugin just a minute ago.

But I don't know what else to report except [PC, PC+5], as you're doing.



r~