On Thu, Jan 6, 2022 at 12:04 PM Alistair Francis <alistai...@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 5:30 PM Bin Meng <bmeng...@gmail.com> wrote:
> >
> > On Mon, Dec 20, 2021 at 2:49 PM Alistair Francis
> > <alistair.fran...@opensource.wdc.com> wrote:
> > >
> > > From: Alistair Francis <alistair.fran...@wdc.com>
> > >
> > > In preperation for adding support for the illegal instruction address
> >
> > typo: preparation
>
> Fixed
>
> >
> > > let's fixup the Hypervisor extension setting GVA logic and improve the
> > > variable names.
> > >
> > > Signed-off-by: Alistair Francis <alistair.fran...@wdc.com>
> > > ---
> > > target/riscv/cpu_helper.c | 21 ++++++---------------
> > > 1 file changed, 6 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 9eeed38c7e..9e1f5ee177 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -967,6 +967,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > >
> > > RISCVCPU *cpu = RISCV_CPU(cs);
> > > CPURISCVState *env = &cpu->env;
> > > + bool write_gva = false;
> > > uint64_t s;
> > >
> > > /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits
> > > wide
> > > @@ -975,7 +976,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > > bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG);
> > > target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
> > > target_ulong deleg = async ? env->mideleg : env->medeleg;
> > > - bool write_tval = false;
> > > target_ulong tval = 0;
> > > target_ulong htval = 0;
> > > target_ulong mtval2 = 0;
> > > @@ -1004,7 +1004,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > > case RISCV_EXCP_INST_PAGE_FAULT:
> > > case RISCV_EXCP_LOAD_PAGE_FAULT:
> > > case RISCV_EXCP_STORE_PAGE_FAULT:
> > > - write_tval = true;
> > > + write_gva = true;
> > > tval = env->badaddr;
> > > break;
> > > default:
> > > @@ -1041,18 +1041,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > > if (riscv_has_ext(env, RVH)) {
> > > target_ulong hdeleg = async ? env->hideleg : env->hedeleg;
> > >
> > > - if (env->two_stage_lookup && write_tval) {
> > > - /*
> > > - * If we are writing a guest virtual address to stval,
> > > set
> > > - * this to 1. If we are trapping to VS we will set this
> > > to 0
> > > - * later.
> > > - */
> > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
> > > - } else {
> > > - /* For other HS-mode traps, we set this to 0. */
> > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
> > > - }
> > > -
> > > if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
> > > /* Trap to VS mode */
> > > /*
> > > @@ -1063,7 +1051,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > > cause == IRQ_VS_EXT) {
> > > cause = cause - 1;
> > > }
> > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
> > > + write_gva = false;
> > > } else if (riscv_cpu_virt_enabled(env)) {
> > > /* Trap into HS mode, from virt */
> > > riscv_cpu_swap_hypervisor_regs(env);
> > > @@ -1072,6 +1060,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > > env->hstatus = set_field(env->hstatus, HSTATUS_SPV,
> > > riscv_cpu_virt_enabled(env));
> > >
> > > +
> > > htval = env->guest_phys_fault_addr;
> > >
> > > riscv_cpu_set_virt_enabled(env, 0);
> > > @@ -1079,7 +1068,9 @@ 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;
> > > }
> > > + env->hstatus = set_field(env->hstatus, HSTATUS_GVA,
> > > write_gva);
> >
> > This does not look correct to me.
> >
> > The env->hstatus[GVA] should remain untouched in the 2nd and 3rd
> > branch. It only needs to be set in the first branch.
>
> The RISC-V spec says:
>
> ""'
> 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 it has to be set in the second and third branches as they are
> trapping into HS mode. I guess we could not touch it in the first
> branch (Trap to VS mode), but that means we would need to ensure it is
> updated later, so it seems easiest to just clear it here.
Thanks for the info.
>
> In the second branch (Trap into HS mode, from virt) we set GVA based
> on the trap cause, which seems correct.
>
> In the third case (Trap into HS mode) we are trapping from HS to HS so
> we want to clear GVA.
With these explanations, I feel this patch mixed up two things into one patch.
The changes to 2nd and 3rd branch probably merit a separate patch with
a better commit message as it *explicitely* clear or set GVA field as
per the spec while current codes don't do that.
A follow-up patch can be made to consolidate other "write_tval" cases.
But that's up to you. FWIW:
Reviewed-by: Bin Meng <bmeng...@gmail.com>
Regards,
Bin
