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

From: Anup Patel <apatel@ventanamicro.com>
To: Alistair Francis <alistair23@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	 Alistair Francis <Alistair.Francis@wdc.com>,
	Sagar Karandikar <sagark@eecs.berkeley.edu>,
	 Anup Patel <anup@brainfault.org>,
	"open list:RISC-V" <qemu-riscv@nongnu.org>,
	 "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
	Atish Patra <atishp@atishpatra.org>
Subject: Re: [PATCH 2/4] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode
Date: Mon, 9 May 2022 17:30:14 +0530	[thread overview]
Message-ID: <CAK9=C2XDHvWCcBdML6auRkRfmhuhzL8wtwjPrzjAGJZfJbws5w@mail.gmail.com> (raw)
In-Reply-To: <CAKmqyKO3cqA+diXVuz8es_0FTiPJyufPpfTxQU_bqw6vHDdFQw@mail.gmail.com>

On Mon, May 9, 2022 at 2:54 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 5, 2022 at 12:36 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > 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.
>
> Ah, I forgot about those instructions, but still they are the
> exception. In general we would expect a trap from HS to HS to contain
> HS addresses. We should just handle the other cases specially

I see your point. Let me re-work this patch to ensure that the GVA bit
is only set when we have a guest virtual address.

Regards,
Anup

>
> Alistair
>
> >
> > >
> > > 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
> > > >
> > > >