Re: [PATCH v1 14/17] xen/riscv: add guest page fault handling stub

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2026 10:59, Oleksii Kurochko wrote:
> On 8/18/26 6:04 PM, Oleksii Kurochko wrote:
>> On 8/18/26 10:29 AM, Jan Beulich wrote:
>>> On 17.08.2026 18:10, Oleksii Kurochko wrote:
>>>> On 8/12/26 5:48 PM, Jan Beulich wrote:
>>>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>>>> +{
>>>>>> +    /*
>>>>>> +     * According to RISC-V spec:
>>>>>> +     *  18.2.8. Hypervisor Trap Value Register (htval)
>>>>>> +     *   ...
>>>>>> +     *   A guest physical address written to htval is shifted 
>>>>>> right by 2 bits
>>>>>> +     *   to accommodate addresses wider than the current XLEN.
>>>>>> +     *   ...
>>>>>> +     *   If the least-significant two bits of a faulting guest 
>>>>>> physical address
>>>>>> +     *   are needed, these bits are ordinarily the same as the
>>>>>> +     *   least-significant two bits of the faulting virtual 
>>>>>> address in stval.
>>>>>> +     *   For faults due to implicit memory accesses for VS-stage 
>>>>>> address
>>>>>> +     *   translation, the least-significant two bits are instead 
>>>>>> zeros. These
>>>>>> +     *   cases can be distinguished using the value provided in 
>>>>>> register htinst.
>>>>>> +     */
>>>>>> +    return (csr_read(CSR_HTVAL) << 2) | (csr_read(CSR_STVAL) & 0x3);
>>>>>
>>>>> Well, okay, but instead of not losing the bottom two bits you're now 
>>>>> losing
>>>>> the top two ones.
>>>>
>>>> Oh, right, I will add a cast ((uint64_t)csr_read(CSR_HTVAL) << 2) | ...
>>>>
>>>> It will cover all the cases RV32 which has 34-bit guest address and it
>>>> will be enough for RV64 where GPA is 59bit (the highest possible for 
>>>> Sv59).
>>>
>>> Only if the function return type then also changes.
>>>
>>>>> Also the spec reads as if htval only _may_ hold the original address 
>>>>> of the
>>>>> faulting access. What if htval ends up 0?
>>>>
>>>> good point. then we have to emulate fault instruction and get an address
>>>> from an instruction. I think that for now it will be enough just to
>>>> support platforms which always write GPA to HTVAL.
>>>>
>>>> If I understand correctly if htval is supported by platform then htval
>>>> will be always filled for guest page fault. To verify if HTVAL is
>>>> supported we could do:
>>>>
>>>> 'Unless it has reason to assume otherwise (such as a platform standard),
>>>> software that writes a value to htval should read back from htval to
>>>> confirm the stored value.'
>>>
>>> How does this matter here? It's one thing for htval to be capable of
>>> holding (all?) non-zero values, and another that it would always be
>>> written. If the platform doesn't indicate the behavior, I fear you have
>>> to assume that you may (perhaps even randomly) observe 0.
>>
>> So to be very sure we could check for two extensions: Sstval and Shtval.
>> They will guarantee that under any circumstances it will be filled.
>>
>> Also, as an option we could check that htinst value isn't zero as 
>> according to the spec:
>>
>> For guest-page faults, the trap instruction register is written with a 
>> special pseudoinstruction value if:
>> (a) the fault is caused by an implicit memory access for VS-stage 
>> address translation, and (b) a nonzero
>> value (the faulting guest physical address) is written to mtval2 or htval.
>>
>> So if htinst != 0 then htval is filled with GPA and a nonzero guest 
>> physical address written to mtval2/htval shall correspond to the exact 
>> virtual address written to mtval/stval.
> 
> I've re-read SPEC again and it looks like htinst != 0 doesn't guarantee 
> that htval and stval will contain necessary for me here faulty 
> instruction. What I wrote above guarantee that if a fault during 
> VS-stage translation failed then it htval will contatain GPA of PTE with 
> which was an issue.
> 
> So I have to blindly believe that HTVAL and STVAL will always contain 
> necessary for me data as KVM and other hypervisor does or introduce here 
> software guest page table walker if Sstvala and Shtvala aren't provided 
> by platform.

Why "blindly believe"? Checking for the necessary extension(s) should be
an option. Adding fallback code for when an extension isn't available can
come later, can't it?

Jan