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

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?

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

> 
> 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.'

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').

Otherwise if it won't work then we can't distinguish if it is zero 
because h/w doesn't update htval or it zero because faulty GPA is zero.

So we could add this check under #ifdef CONFIG_DEBUG here and if HTVAL 
isn't supported then we can't work on this platform.

> 
> Further, nit: There's (once again) no real value in the 0x prefix, I don't
> think.

Sure I will drop then.

> 
>> +static int emulate_load(unsigned long fault_addr, unsigned long htinst)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static int emulate_store(unsigned long fault_addr, unsigned long htinst)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static void handle_guest_page_fault(unsigned long cause,
>> +                                    struct cpu_user_regs *regs)
>> +{
>> +    unsigned long addr;
>> +    int rc;
>> +
>> +    addr = get_faulting_gpa();
> 
> Can't this become the initializer of the variable?

Sure, it can. I will do that.

Thanks.

~ Oleksii