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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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