Re: [PATCH v2 21/39] xen/riscv: resolve the faulting guest physical address

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> Take the guest physical address from htval and stval: on a guest-page fault
> htval holds it shifted right by 2, so that an address wider than XLEN fits,
> and stval holds the faulting guest virtual address, whose two least
> significant bits are those of the guest physical address. The shift is done
> on paddr_t rather than on the raw register: a guest physical address is 34
> bits wide on RV32 with Sv32x4, so shifting an XLEN-wide value would drop its
> top two bits.
>
> Those two low bits come from stval only for a fault on an explicit access.
> Where one is taken on an implicit access made for VS-stage translation htval
> holds the address of the VS-stage PTE which could not be read, while stval
> still holds the guest virtual address which started the walk, and the low
> bits of the address written to htval are zero instead. htinst tells the two
> apart, which is what the spec points at it for.
> 
I'd just precise (the spec is also not clear on this point though), what
is "current XLEN" here, clearly indicate that htval holds the GPA >> 2 and
stval holds VGA and also put the two different cases we need to
distinguish clearly:
```
Recover the guest physical address from htval and stval. On a guest-page
fault to hypervisor, htval holds the guest physical address shifted
right by 2, so that an address wider than HSXLEN fits, and stval holds
the faulting guest virtual address. The shift is done on paddr_t rather
than on the raw register: a guest physical address is 34 bits wide on
RV32 with Sv32x4, so shifting an XLEN-wide value would drop its top two
bits.

However, there are two cases to distinguish when recovering the
faulting GPA:
    - Explicit memory access: we use the two least significant bits of
    stval, which are the same as those of the guest physical
    address.
    - Implicit memory access for VS-stage translation: the two least
    significant bits of htval are zero.

These two cases can be distinguished using the value provided in
register htinst.
```
> 
> stval needs no check against an ISA extension: a guest-page fault writes it
> with the faulting guest virtual address regardless. Sstvala would not be the
> right thing to test for either (it covers stval across every trap type
> which writes it, a wider guarantee than what is needed here).
> 
> htval does need one. The H extension lets an implementation write it with
> either the faulting address or zero, so without Shtvala a zero htval cannot
> be told apart from a genuine fault on guest physical address 0-3, and the
> address has to be recovered by decoding the access and walking the VS-stage
> page tables in software instead. That is left as a TODO, and until it is
> written such hardware panics rather than acting on an address which may not
> be the one which faulted.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/emulate.c b/xen/arch/riscv/emulate.c
> index f9da075104..ff530ef2df 100644
> --- a/xen/arch/riscv/emulate.c
> +++ b/xen/arch/riscv/emulate.c
> @@ -9,6 +9,7 @@
>  #include <xen/sched.h>
>  #include <xen/types.h>
>  
> +#include <asm/cpufeature.h>
>  #include <asm/csr.h>
>  #include <asm/current.h>
>  #include <asm/emulate.h>
> @@ -62,10 +63,28 @@ static bool htinst_is_pseudo(unsigned long htinst)
>      }
>  }
>  
> -/* Reconstruct the guest physical address of the access which faulted. */
> +/* Resolves the guest physical address the access faulted on into @gf->gpa. 
> */
Why did you change the comment apart for adding @gf->gpa? I think the
"reconstruct" is more clear but there might be another reason why you
changed it.

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>