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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

Also the spec reads as if htval only _may_ hold the original address of the
faulting access. What if htval ends up 0?

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

> +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?

Jan