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

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.

Work around this with a special helper to inform the rest of the
translator about the instruction so things stay consistent.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/translator.h    | 17 +++++++++++++++++
 target/s390x/tcg/translate.c |  4 ++++
 2 files changed, 21 insertions(+)

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~

On 19/10/22 13: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.
> 
> Work around this with a special helper to inform the rest of the
> translator about the instruction so things stay consistent.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/exec/translator.h    | 17 +++++++++++++++++
>   target/s390x/tcg/translate.c |  4 ++++
>   2 files changed, 21 insertions(+)
> 
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 3b77f5f4aa..156f568701 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -211,6 +211,23 @@ translator_ldq_swap(CPUArchState *env, DisasContextBase *db,
>       return ret;
>   }
>   
> +/**
> + * 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));
> +}
> +
> +
>   /*
>    * Return whether addr is on the same page as where disassembly started.
>    * Translators can use this to enforce the rule that only single-insn
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 1d2dddab1c..a07b8b2d23 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -6317,12 +6317,16 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
>       if (unlikely(s->ex_value)) {
>           /* Drop the EX data now, so that it's clear on exception paths.  */
>           TCGv_i64 zero = tcg_const_i64(0);
> +        int i;
>           tcg_gen_st_i64(zero, cpu_env, offsetof(CPUS390XState, ex_value));
>           tcg_temp_free_i64(zero);
>   
>           /* Extract the values saved by EXECUTE.  */
>           insn = s->ex_value & 0xffffffffffff0000ull;
>           ilen = s->ex_value & 0xf;
> +        for (i = 0; i < ilen; i += 2) {

Is it worth guarding with #ifdef CONFIG_PLUGIN?

> +            translator_fake_ldw(extract64(insn, 48 - (i * 8), 16), pc + i);
> +        }
>           op = insn >> 56;
>       } else {
>           insn = ld_code2(env, s, pc);

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 19/10/22 13: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.
>> Work around this with a special helper to inform the rest of the
>> translator about the instruction so things stay consistent.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   include/exec/translator.h    | 17 +++++++++++++++++
>>   target/s390x/tcg/translate.c |  4 ++++
>>   2 files changed, 21 insertions(+)
>> diff --git a/include/exec/translator.h b/include/exec/translator.h
>> index 3b77f5f4aa..156f568701 100644
>> --- a/include/exec/translator.h
>> +++ b/include/exec/translator.h
>> @@ -211,6 +211,23 @@ translator_ldq_swap(CPUArchState *env, DisasContextBase *db,
>>       return ret;
>>   }
>>   +/**
>> + * 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));
>> +}
>> +
>> +
>>   /*
>>    * Return whether addr is on the same page as where disassembly started.
>>    * Translators can use this to enforce the rule that only single-insn
>> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
>> index 1d2dddab1c..a07b8b2d23 100644
>> --- a/target/s390x/tcg/translate.c
>> +++ b/target/s390x/tcg/translate.c
>> @@ -6317,12 +6317,16 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
>>       if (unlikely(s->ex_value)) {
>>           /* Drop the EX data now, so that it's clear on exception paths.  */
>>           TCGv_i64 zero = tcg_const_i64(0);
>> +        int i;
>>           tcg_gen_st_i64(zero, cpu_env, offsetof(CPUS390XState, ex_value));
>>           tcg_temp_free_i64(zero);
>>             /* Extract the values saved by EXECUTE.  */
>>           insn = s->ex_value & 0xffffffffffff0000ull;
>>           ilen = s->ex_value & 0xf;
>> +        for (i = 0; i < ilen; i += 2) {
>
> Is it worth guarding with #ifdef CONFIG_PLUGIN?

I don't think so. It all gets nicely dead coded away when plugins aren't
enabled.

>
>> +            translator_fake_ldw(extract64(insn, 48 - (i * 8), 16), pc + i);
>> +        }
>>           op = insn >> 56;
>>       } else {
>>           insn = ld_code2(env, s, pc);