Re: [PATCH v6 21/21] target/riscv: Make translator stop before the end of a page

[Alistair Francis]

On Fri, Aug 19, 2022 at 1:39 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Right now the 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.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1155
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/translate.c          | 17 +++++--
>  tests/tcg/riscv64/noexec.c        | 79 +++++++++++++++++++++++++++++++
>  tests/tcg/riscv64/Makefile.target |  1 +
>  3 files changed, 93 insertions(+), 4 deletions(-)
>  create mode 100644 tests/tcg/riscv64/noexec.c
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index a719aa6e63..f8af6daa70 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1154,12 +1154,21 @@ static void riscv_tr_translate_insn(DisasContextBase 
> *dcbase, CPUState *cpu)
>      }
>      ctx->nftemp = 0;
>
> +    /* Only the first insn within a TB is allowed to cross a page boundary. 
> */
>      if (ctx->base.is_jmp == DISAS_NEXT) {
> -        target_ulong page_start;
> -
> -        page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
> -        if (ctx->base.pc_next - page_start >= TARGET_PAGE_SIZE) {
> +        if (!is_same_page(&ctx->base, ctx->base.pc_next)) {
>              ctx->base.is_jmp = DISAS_TOO_MANY;
> +        } else {
> +            unsigned page_ofs = ctx->base.pc_next & ~TARGET_PAGE_MASK;
> +
> +            if (page_ofs > TARGET_PAGE_SIZE - MAX_INSN_LEN) {
> +                uint16_t next_insn = cpu_lduw_code(env, ctx->base.pc_next);
> +                int len = insn_len(next_insn);
> +
> +                if (!is_same_page(&ctx->base, ctx->base.pc_next + len)) {
> +                    ctx->base.is_jmp = DISAS_TOO_MANY;
> +                }
> +            }
>          }
>      }
>  }
> diff --git a/tests/tcg/riscv64/noexec.c b/tests/tcg/riscv64/noexec.c
> new file mode 100644
> index 0000000000..86f64b28db
> --- /dev/null
> +++ b/tests/tcg/riscv64/noexec.c
> @@ -0,0 +1,79 @@
> +#include "../multiarch/noexec.c.inc"
> +
> +static void *arch_mcontext_pc(const mcontext_t *ctx)
> +{
> +    return (void *)ctx->__gregs[REG_PC];
> +}
> +
> +static int arch_mcontext_arg(const mcontext_t *ctx)
> +{
> +    return ctx->__gregs[REG_A0];
> +}
> +
> +static void arch_flush(void *p, int len)
> +{
> +    __builtin___clear_cache(p, p + len);
> +}
> +
> +extern char noexec_1[];
> +extern char noexec_2[];
> +extern char noexec_end[];
> +
> +asm(".option push\n"
> +    ".option norvc\n"
> +    "noexec_1:\n"
> +    "   li a0,1\n"       /* a0 is 0 on entry, set 1. */
> +    "noexec_2:\n"
> +    "   li a0,2\n"      /* a0 is 0/1; set 2. */
> +    "   ret\n"
> +    "noexec_end:\n"
> +    ".option pop");
> +
> +int main(void)
> +{
> +    struct noexec_test noexec_tests[] = {
> +        {
> +            .name = "fallthrough",
> +            .test_code = noexec_1,
> +            .test_len = noexec_end - noexec_1,
> +            .page_ofs = noexec_1 - noexec_2,
> +            .entry_ofs = noexec_1 - noexec_2,
> +            .expected_si_ofs = 0,
> +            .expected_pc_ofs = 0,
> +            .expected_arg = 1,
> +        },
> +        {
> +            .name = "jump",
> +            .test_code = noexec_1,
> +            .test_len = noexec_end - noexec_1,
> +            .page_ofs = noexec_1 - noexec_2,
> +            .entry_ofs = 0,
> +            .expected_si_ofs = 0,
> +            .expected_pc_ofs = 0,
> +            .expected_arg = 0,
> +        },
> +        {
> +            .name = "fallthrough [cross]",
> +            .test_code = noexec_1,
> +            .test_len = noexec_end - noexec_1,
> +            .page_ofs = noexec_1 - noexec_2 - 2,
> +            .entry_ofs = noexec_1 - noexec_2 - 2,
> +            .expected_si_ofs = 0,
> +            .expected_pc_ofs = -2,
> +            .expected_arg = 1,
> +        },
> +        {
> +            .name = "jump [cross]",
> +            .test_code = noexec_1,
> +            .test_len = noexec_end - noexec_1,
> +            .page_ofs = noexec_1 - noexec_2 - 2,
> +            .entry_ofs = -2,
> +            .expected_si_ofs = 0,
> +            .expected_pc_ofs = -2,
> +            .expected_arg = 0,
> +        },
> +    };
> +
> +    return test_noexec(noexec_tests,
> +                       sizeof(noexec_tests) / sizeof(noexec_tests[0]));
> +}
> diff --git a/tests/tcg/riscv64/Makefile.target 
> b/tests/tcg/riscv64/Makefile.target
> index d41bf6d60d..b5b89dfb0e 100644
> --- a/tests/tcg/riscv64/Makefile.target
> +++ b/tests/tcg/riscv64/Makefile.target
> @@ -3,3 +3,4 @@
>
>  VPATH += $(SRC_PATH)/tests/tcg/riscv64
>  TESTS += test-div
> +TESTS += noexec
> --
> 2.34.1
>
>