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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/9/26 2:04 PM, Baptiste Le Duc wrote:
> > 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
My understanding was that "current XLEN" == hypervisor XLEN as htval is 
hypervisor register and has HSXLEN size so it was okay for me to have 
just XLEN in the original commit message. But I am okay to use your 
suggestion ...
> 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
... here.

> 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.
The original phrasing "the two least significant bits of htval are zero" 
is inaccurate for the following reasons:
- htval holds a shifted address (GPA >> 2): The htval CSR stores the 
faulting Guest Physical Address (GPA) shifted right by 2 bits. As a 
result, the two least significant bits of htval (htval[1:0]) actually 
correspond to bits 2 and 3 of the original GPA (GPA[3:2]).
- htval bits are not guaranteed to be zero: Implicit memory accesses 
during VS-stage address translation fetch PTEs that are 4-byte aligned 
(Sv32) or 8-byte aligned (Sv39/Sv48/Sv57). Since PTEs can reside at 
offsets like 0x04, 0x08, or 0x0C, GPA (and therefore htval[1:0]) can be 
non-zero.
- What is actually guaranteed to be zero: Because all VS-stage PTE 
accesses are at least 4-byte aligned, it is the lowest two bits of the 
unshifted Guest Physical Address (GPA[1:0]) that are guaranteed to be 
00, not the low bits of htval.
So here I think we want to clarify then:
```
Implicit memory access for VS-stage translation: the two least 
significant bits of the guest physical address (GPA[1:0]) 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.
I thought that it will just a little bit clearer (and anyway it should a 
part of another patch...) so I will revert the change here and go with 
"reconstruct".
Thanks.

~ Oleksii