Re: [PATCH v2 2/4] target/s390x: Make translator stop before the end of a page

[Richard Henderson]

On 8/5/22 09:09, Ilya Leoshkevich wrote:
> Right now translator stops right *after* the end of a page, which
> breaks reporting of fault locations when the last instruction of a
> multi-insn translation block crosses a page boundary.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   include/exec/translator.h    | 10 ++++++++++
>   target/s390x/tcg/translate.c | 35 ++++++++++++++++++++---------------
>   2 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 7db6845535..d27f8c33b6 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -187,4 +187,14 @@ FOR_EACH_TRANSLATOR_LD(GEN_TRANSLATOR_LD)
>   
>   #undef GEN_TRANSLATOR_LD
>   
> +/*
> + * Return whether addr is on the same page as where disassembly started.
> + * Translators can use this to enforce the rule that only single-insn
> + * translation blocks are allowed to cross page boundaries.
> + */
> +static inline bool is_same_page(DisasContextBase *db, target_ulong addr)
> +{
> +    return ((addr ^ db->pc_first) & TARGET_PAGE_MASK) == 0;
> +}
> +
>   #endif /* EXEC__TRANSLATOR_H */
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index e2ee005671..0cd0c932fb 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -6305,14 +6305,13 @@ static void extract_field(DisasFields *o, const 
> DisasField *f, uint64_t insn)
>       o->c[f->indexC] = r;
>   }
>   
> -/* Lookup the insn at the current PC, extracting the operands into O and
> -   returning the info struct for the insn.  Returns NULL for invalid insn.  */
> +/* Lookup the insn at the current PC, filling the info struct.  */
>   
> -static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
> +static DisasJumpType extract_insn(CPUS390XState *env, DisasContext *s,
> +                                  const DisasInsn **info)
>   {
>       uint64_t insn, pc = s->base.pc_next;
>       int op, op2, ilen;
> -    const DisasInsn *info;
>   
>       if (unlikely(s->ex_value)) {
>           /* Drop the EX data now, so that it's clear on exception paths.  */
> @@ -6325,9 +6324,13 @@ static const DisasInsn *extract_insn(CPUS390XState *env, 
> DisasContext *s)
>           ilen = s->ex_value & 0xf;
>           op = insn >> 56;
>       } else {
> +        assert(s->base.num_insns == 1 || is_same_page(&s->base, pc));
>           insn = ld_code2(env, s, pc);
>           op = (insn >> 8) & 0xff;
>           ilen = get_ilen(op);
> +        if (s->base.num_insns > 1 && !is_same_page(&s->base, pc + ilen - 1)) {
> +            return DISAS_TOO_MANY;
> +        }

This doesn't work.

You need to end the TB at the end of the previous insn, not at the beginning of the 
current insn.  Here, we have already committed to executing one instruction.

You need to model this similar to arm's insn_crosses_page, where we check at the end of 
one instruction whether we'll be able to run another.


r~