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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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@gmail.com>
>>
>> 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