Re: [PATCH v2 18/39] xen/riscv: add guest page fault handling stub

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2026 17:09, Oleksii Kurochko wrote:
> On 9/8/26 4:10 PM, Jan Beulich wrote:
>> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/emulate.c
>>> @@ -0,0 +1,179 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +
>>> +/*
>>> + * RISC-V instruction emulation for trapped guest accesses
>>> + */
>>> +
>>> +#include <xen/bug.h>
>>> +#include <xen/errno.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <asm/csr.h>
>>> +#include <asm/current.h>
>>> +#include <asm/emulate.h>
>>> +#include <asm/riscv_encoding.h>
>>> +#include <asm/traps.h>
>>> +
>>> +/*
>>> + * The hardware-reported details of a guest page fault, gathered once by
>>> + * handle_guest_page_fault() and passed down to the emulation of the 
>>> faulted
>>> + * access.
>>> + */
>>> +struct guest_fault {
>>> +    /* The guest register state as saved on entry to do_trap(). */
>>> +    struct cpu_user_regs *regs;
>>
>> If the comment was true, this could be pointer-to-const.
> 
> I think it can't be pointer-to-const as emulate_load/store functions 
> wants to change PC register after MMIO access emulation is finished to 
> not trap again.

Of course, hence how I started the sentence.

>>> +    /* scause: a fetch, a load or a store/AMO guest page fault. */
>>> +    unsigned long cause;
>>> +    /*
>>> +     * htinst: the trapped instruction in its transformed form, or one of 
>>> the
>>> +     * special values (zero, or a pseudoinstruction).
>>> +     */
>>> +    unsigned long htinst;
>>> +    /* htval: as written by hardware; see resolve_faulting_gpa(). */
>>> +    unsigned long htval;
>>> +    /* stval: the guest virtual address of the faulting access. */
>>> +    unsigned long stval;
>>> +    /* The faulting guest physical address, filled by 
>>> resolve_faulting_gpa(). */
>>> +    paddr_t gpa;
>>> +};
>>> +
>>> +/*
>>> + * Is @htinst one of the pseudoinstructions reported for a guest page fault
>>> + * taken on an implicit memory access done for VS-stage address 
>>> translation?
>>> + *
>>> + * All four values are recognized regardless of the hypervisor's XLEN: the
>>> + * width they encode is that of a VS-stage PTE, i.e. it follows the guest's
>>> + * paging mode (4 bytes for Sv32, 8 otherwise). On RV32 the 64-bit forms
>>> + * simply never occur.
>>> + */
>>> +static bool htinst_is_pseudo(unsigned long htinst)
>>> +{
>>> +    switch ( htinst )
>>> +    {
>>> +    case INSN_PSEUDO_VS_LOAD32:
>>> +    case INSN_PSEUDO_VS_STORE32:
>>> +    case INSN_PSEUDO_VS_LOAD64:
>>> +    case INSN_PSEUDO_VS_STORE64:
>>> +        return true;
>>> +
>>> +    default:
>>> +        return false;
>>> +    }
>>> +}
>>
>> This feels fragile. New pseudo-insns can appear at any time. If the value as
>> a whole is non-zero, aiui the low two bits being zero indicate a pseudo-insn.
>> In which case enumerating pseudo-insns we are currently aware of isn't
>> necessary.
> 
> I will write it simpler        then:
> 
> /*
>   * Is @htinst one of the special pseudoinstruction values, reported for 
> a guest
>   * page fault taken on an implicit memory access done for VS-stage address
>   * translation?
>   *
>   * It is enough to check only bits[1:0] as according to the spec:
>   *
>   * The value is one of the special pseudoinstructions defined later, all of
>   * which have bits 1:0 equal to 00.
>   */
> static bool htinst_is_pseudo(unsigned long htinst)
> {
>      return htinst && ((htinst & 3) == 0);
> }

And preferably

     return htinst && !(htinst & 3);

to be self-consistent.

>>> +    /*
>>> +     * A guest-page fault may arise due to an implicit memory access during
>>> +     * first-stage (VS-stage) address translation, in which case a guest
>>> +     * physical address written to htval is that of the implicit memory
>>> +     * access that faulted - for example, the address of a VS-level page
>>> +     * table entry that could not be read. (The guest physical address
>>> +     * corresponding to the original virtual address is unknown when
>>> +     * VS-stage translation fails to complete)
>>> +     *
>>> +     * In such cases htinst reports one of the pseudoinstructions 
>>> recognized
>>> +     * by htinst_is_pseudo(), and the fault requires separate handling 
>>> (since
>>> +     * G-stage translation failed on an unpopulated/unmapped guest physical
>>> +     * address during a hardware page-table walk). To match bare hardware
>>> +     * behavior, we must inject an access fault of the ORIGINAL access type
>>> +     * (Instruction, Load, or Store/AMO) that initiated the address
>>> +     * translation.
>>> +     */
>>> +    if ( htinst_is_pseudo(gf.htinst) )
>>> +    {
>>> +        inject_access_fault(&gf);
>>> +
>>> +        return;
>>> +    }
>>
>> I.e. you imply that guests won't put their page tables in MMIO? That's
>> fragile imo; I have seen OSes to use video frame buffers for all kinds
>> of (transient) purposes, for example.
> 
> I think it is okay for now and if it will a real use case then an update 
> of this code will be needed.

May I then ask that you leave a remark (maybe even fixme) to this effect?

>>> +    resolve_faulting_gpa(&gf);
>>
>> Since the function is only a stub right now - how is one to tell whether
>> this indeed can never fail?
> 
> It can't be tell. But what is wrong if it could fail? (Actually with 
> current implementation introduced in later patches you can find it can 
> fail if a necessary extension or software page walk isn't introduced).

Well, quite obviously if it can fail, its return value would need checking
here.

Jan