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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 9/10/26 8:38 AM, Jan Beulich wrote:
> 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.
> 

Good point. I will apply your suggestion.

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

Sure, I will then add the following to the comment above if ():

      * FIXME: This assumes that guest page tables never reside in an 
emulated
      * MMIO region, i.e. that an implicit access faulting at G-stage always
      * targets an unpopulated GPA. Guests may (even transiently) place page
      * tables in MMIO-backed memory, e.g. a video frame buffer. Supporting
      * that would require walking the VS-stage page tables in software,
      * accessing the PTEs (including A/D updates) through the MMIO 
handlers,
      * and then emulating the original access, instead of injecting a 
fault.

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

That what I thought about after I sent my e-mail as a possible option.

I will update the prototype to:

+static int resolve_faulting_gpa(struct guest_fault *gf)
  {
-    BUG_ON("unimplemented");
+    return -EOPNOTSUPP;
  }

And handle an error code in the following way:

@@ -136,7 +144,8 @@ void handle_guest_page_fault(struct cpu_user_regs 
*regs, unsigned long cause
)
          return;
      }

-    resolve_faulting_gpa(&gf);
+    if ( rc = resolve_faulting_gpa(&gf) )
+        goto out;

      switch ( cause )
      {
@@ -163,6 +172,7 @@ void handle_guest_page_fault(struct cpu_user_regs 
*regs, unsigned long cause
)
          break;
      }

+ out:
      if ( rc )
          domain_crash(current->domain,

and then in the next patch "[PATCH v2 21/39] xen/riscv: resolve the 
faulting guest physical address" I will do "return -EOPNOTSUPP" instead 
of panic():

  -        panic("Shtvala isn't supported by h/w; s/w VS-stage walk 
required\n");
++    {
++        printk_once(XENLOG_WARNING
++                    "Shtvala isn't supported by h/w; s/w VS-stage walk 
required\n");
++        return -EOPNOTSUPP;
++    }

~ Oleksii