Re: [PATCH 2/4] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode

[Anup Patel]

On Thu, May 5, 2022 at 3:21 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Apr 29, 2022 at 1:38 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > Currently, QEMU does not set hstatus.GVA bit for traps taken from
> > HS-mode into HS-mode which breaks the Xvisor nested MMU test suite
> > on QEMU. This was working previously.
> >
> > This patch updates riscv_cpu_do_interrupt() to fix the above issue.
> >
> > Fixes: 86d0c457396b ("target/riscv: Fixup setting GVA")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  target/riscv/cpu_helper.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index e1aa4f2097..d83579accf 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -1434,7 +1434,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >                  /* Trap into HS mode */
> >                  env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
> >                  htval = env->guest_phys_fault_addr;
> > -                write_gva = false;
>
> This doesn't seem right.
>
> "Field GVA (Guest Virtual Address) is written by the implementation
> whenever a trap is taken
> into HS-mode. For any trap (breakpoint, address misaligned, access
> fault, page fault, or guest-
> page fault) that writes a guest virtual address to stval, GVA is set
> to 1. For any other trap into
> HS-mode, GVA is set to 0"
>
> So if we are trapping from HS to HS, the address in stval should not
> be a guest virtual address, at least in general.

That's not correct. The HLV/HSV instructions executed by hypervisor
(HS-mode) take guest virtual address. These instructions can trap
from HS-mode to HS-mode.

>
> We probably aren't correctly setting GVA if MPRV is set though, as
> then the page faults should be guest addresses. That's probably the
> issue you are seeing.

The Xvisor nested MMU test-suit is broken on QEMU because it
uses HLV/HSV instructions in HS-mode.

Regards,
Anup

>
> Alistair
>
> >              }
> >              env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva);
> >          }
> > --
> > 2.34.1
> >
> >