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

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:
>>> --- a/xen/arch/riscv/traps.c
>>> +++ b/xen/arch/riscv/traps.c
>>> @@ -191,6 +191,67 @@ static void timer_interrupt(void)
>>>       raise_softirq(TIMER_SOFTIRQ);
>>>   }
>>>   
>>> +static always_inline unsigned long get_faulting_gpa(void)
>>
>> May I suggest to use always_inline only when inlining is _functionally_
>> required?
> 
> Sure. But it ins't clear to me why it isn't a case here? Is it connected 
> to that function is static and too simple so a compiler will do by itself?

Counter question: What is it that would functionally break if the function
ended up not being inlined? (This is the question you generally need to
answer to justify use of always_inline. Of course there's the additional
case of performance being affected, but I don't view that as applicable
here; I'm open to be proven wrong, though.)

>>> +{
>>> +    /*
>>> +     * 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.

> And is it true because:
> ```
> A value of zero in mtval signifies either that the feature is not 
> supported, or an illegal zero instruction was fetched.
> ```
> (yes, it is about mtval but I asssume that htval has the same behaviour').

Right, but what you quote is specific to illegal instruction exceptions.

Jan