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
