Re: [PATCH 5/6] target/riscv: fix checks in hmode/hmode32

[Alistair Francis]

On Sun, Jul 10, 2022 at 6:24 PM Weiwei Li <liwei...@iscas.ac.cn> wrote:
>
> - It seems that there is no explicitly description about whether
> the Hypervisor CSRs requires S extension
> - Csrs only existed in RV32 will not trigger virtual instruction fault
> when not in RV32
>
> Signed-off-by: Weiwei Li <liwei...@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqi...@iscas.ac.cn>
> ---
>  target/riscv/csr.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0d8e98b7a9..975007f1ac 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -311,8 +311,7 @@ static int aia_smode32(CPURISCVState *env, int csrno)
>
>  static RISCVException hmode(CPURISCVState *env, int csrno)
>  {
> -    if (riscv_has_ext(env, RVS) &&
> -        riscv_has_ext(env, RVH)) {
> +    if (riscv_has_ext(env, RVH)) {

This doesn't seem right. The spec doesn't explicitly say this, but I
don't see how you can have the Hypervisor extension without S-mode

>          /* Hypervisor extension is supported */
>          if ((env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
>              env->priv == PRV_M) {
> @@ -328,11 +327,7 @@ static RISCVException hmode(CPURISCVState *env, int 
> csrno)
>  static RISCVException hmode32(CPURISCVState *env, int csrno)
>  {
>      if (riscv_cpu_mxl(env) != MXL_RV32) {
> -        if (!riscv_cpu_virt_enabled(env)) {
> -            return RISCV_EXCP_ILLEGAL_INST;
> -        } else {
> -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> -        }
> +        return RISCV_EXCP_ILLEGAL_INST;

Good catch

Alistair

>      }
>
>      return hmode(env, csrno);
> --
> 2.17.1
>
>