Re: [PATCH v2 2/2] target/riscv/cpu_helper.c: Fix mxr bit behavior

[Alistair Francis]

On Tue, Nov 21, 2023 at 6:53 PM Ivan Klokov <ivan.klokov@syntacore.com> wrote:
>
> According to RISCV Specification sect 9.5 on two stage translation when
> V=1 the vsstatus(mstatus in QEMU's terms) field MXR, which makes
> execute-only pages readable, only overrides VS-stage page protection.
> Setting MXR at HS-level(mstatus_hs), however, overrides both VS-stage
> and G-stage execute-only permissions.
>
> The hypervisor extension changes the behavior of MXR\MPV\MPRV bits.
> Due to RISCV Specification sect. 9.4.1 when MPRV=1, explicit memory
> accesses are translated and protected, and endianness is applied, as
> though the current virtualization mode were set to MPV and the current
> nominal privilege mode were set to MPP. vsstatus.MXR makes readable
> those pages marked executable at the VS translation stage.
>
> Fixes: 36a18664ba ("target/riscv: Implement second stage MMU")
>
> Signed-off-by: Ivan Klokov <ivan.klokov@syntacore.com>

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

Alistair

> ---
>  target/riscv/cpu_helper.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 9ff0952e46..e7e23b34f4 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1032,13 +1032,29 @@ restart:
>          prot |= PAGE_WRITE;
>      }
>      if (pte & PTE_X) {
> -        bool mxr;
> +        bool mxr = false;
>
> -        if (first_stage == true) {
> +        /*
> +         * Use mstatus for first stage or for the second stage without
> +         * virt_enabled (MPRV+MPV)
> +         */
> +        if (first_stage || !env->virt_enabled) {
>              mxr = get_field(env->mstatus, MSTATUS_MXR);
> -        } else {
> -            mxr = get_field(env->vsstatus, MSTATUS_MXR);
>          }
> +
> +        /* MPRV+MPV case, check VSSTATUS */
> +        if (first_stage && two_stage && !env->virt_enabled) {
> +            mxr |= get_field(env->vsstatus, MSTATUS_MXR);
> +        }
> +
> +        /*
> +         * Setting MXR at HS-level overrides both VS-stage and G-stage
> +         * execute-only permissions
> +         */
> +        if (env->virt_enabled) {
> +            mxr |= get_field(env->mstatus_hs, MSTATUS_MXR);
> +        }
> +
>          if (mxr) {
>              prot |= PAGE_READ;
>          }
> --
> 2.34.1
>
>