[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 16/17] xen/riscv: add guest load emulation for trapped MMIO accesses
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
- Date: Thu, 20 Aug 2026 15:38:45 +0200
- Authentication-results: eu.smtp.expurgate.cloud; dkim=pass header.s=20251104 header.d=gmail.com header.i="@gmail.com" header.h="Content-Transfer-Encoding:Content-Type:In-Reply-To:From:Content-Language:References:Cc:To:Subject:User-Agent:MIME-Version:Date:Message-ID"
- Cc: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>, Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>, Alistair Francis <alistair.francis@xxxxxxx>, Connor Davis <connojdavis@xxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Roger Pau Monné <roger@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- Delivery-date: Thu, 20 Aug 2026 13:38:58 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 8/20/26 9:34 AM, Jan Beulich wrote:
On 19.08.2026 18:06, Oleksii Kurochko wrote:
On 8/13/26 9:15 AM, Jan Beulich wrote:
On 29.07.2026 15:40, Oleksii Kurochko wrote:
@@ -210,9 +217,162 @@ static always_inline unsigned long get_faulting_gpa(void)
return (csr_read(CSR_HTVAL) << 2) | (csr_read(CSR_STVAL) & 0x3);
}
+/*
+ * Determine the trapped instruction which caused a guest MMIO trap.
+ *
+ * Returns true if the trap was redirected to the guest, in which case
+ * the caller must stop emulation and return success. Otherwise *insn
+ * and *insn_len are filled in and the caller should continue decoding.
+ */
+static bool decode_trapped_insn(unsigned long htinst, unsigned long *insn,
+ unsigned int *insn_len)
+{
+ if ( htinst & 0x1 )
+ {
+ /*
+ * Bit[0] == 1 implies trapped instruction value is
+ * transformed instruction or custom instruction.
+ */
+ *insn = htinst | INSN_16BIT_MASK;
+ *insn_len = (htinst & BIT(1, UL)) ? INSN_LEN(*insn) : 2;
In the if() you don't use BIT(), while here you do. Please be consistent.
Why the use of INSN_LEN(), when due to the earlier assignment it'll always
yield 4 here?
ld/sd instruction which we are trapping here at the moment here could be
2 bit and 4 bit depends on C extension so we need to pass correct
instruction length to advance_pc() after it is emulated.
Well, fine, but how does that matter? I pointed you at the preceding
assignment, which sets bits 0 and 1. With that INSN_LEN() is guaranteed
to return (at least) 4 (and it's not presently capable of returning
values larger than 4).
Oh, right. But considering that htinst can handle only max 31 bits so it
looks like it can't fit more then 32 bits instructions. But I think it
is needed ifdef around INSN_LEN to not miss add support for longer
instructions or just write now more generic macros (or static inline
function).
Finally, how would the caller know whether it looks at a transformed insn
or (as fetched below) a "normal" one?
According to the spec ((part from htinst ... ):
On a synchronous exception, if a nonzero value is written, one of the
following shall be true about the value:
• Bit 0 is 1, and replacing bit 1 with 1 makes the value into a valid
encoding of a standard instruction.
In this case, the instruction that trapped is the same kind as indicated
by the register value, and the register value is the transformation of
the trapping instruction, as defined later. For example, if bits 1:0 are
binary 11 and the register value is the encoding of a standard LW (load
word) instruction, then the trapping instruction is LW, and the register
value is the transformation of the trapping LW instruction.
• Bit 0 is 1, and replacing bit 1 with 1 makes the value into an
instruction encoding that is explicitly designated for a custom
instruction (not an unused reserved encoding). This is a custom value.
The instruction that trapped is a non-standard instruction. The
interpretation of a custom value is not otherwise specified by this
standard.
• The value is one of the special pseudoinstructions defined later, all
of which have bits 1:0 equal to 00.
So setting bit 0 to 1 we will guarantee that it is normal "normal"
instruction.
Right. Yet my question was how to distinguish the cases. Or are you trying
to tell me that distinguishing isn't going to be necessary, anywhere?
Yes, I don't see for now why such distinguish is necessary. But I will
re-check that point.
+ }
+ else
+ {
+ struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
Pointer-to-const.
+ struct trap_info utrap = { 0 };
Just {} please.
+ /*
+ * Bit[0] == 0 implies trapped instruction value is
+ * zero or special value.
+ */
How come you get away without dealing with pseudoinsns? The insn pointed at
by regs->sepc is of no interest for faults caused by implicit memory accesses
originating from VS-stage address translation.
It is really problem but I think it should be resolved much earlier in
handle_guest_page_fault(). I will add the following:
/*
* A guest page fault taken on an implicit memory access performed for
* VS-stage address translation (reading a PTE, or updating its A/D
bits)
* reports a pseudoinstruction in htinst rather than a transformed
* instruction. Such a fault can't be emulated: htval holds the guest
* physical address of a VS-stage PTE rather than of any access the
guest
* itself performed (and its two least significant bits are zero
instead
* of matching stval), while the instruction at sepc is unrelated
to the
* access which actually faulted.
*
* Report an access fault to the guest at the original virtual address,
* which is what stval already holds and what hardware would raise
for a
* page table walk hitting an inaccessible address.
*/
if ( (htinst == INSN_PSEUDO_VS_LOAD) || (htinst ==
INSN_PSEUDO_VS_STORE) )
{
struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
struct trap_info utrap = {
.scause = (htinst == INSN_PSEUDO_VS_LOAD) ? CAUSE_LOAD_ACCESS
: CAUSE_STORE_ACCESS,
.sepc = regs->sepc,
.stval = csr_read(CSR_STVAL),
};
riscv_trap_redirect(&utrap);
return;
}
That's not what would happen on bare hardware though, aiui. At least I don't
think I ever found it being spelled out anywhere what the supposed behavior
is when a page table resides in unpopulated space.
What do you mean here by "unpopulated space"? IIUC, it means that to
have things working we should have GVA -> GPA mapping, if there is no
such mapping then correspondent access bits aren't set and so a
page-fault exception corresponding to the original access type. So not
access fault should be here but just correspondent page fault.
If GPA itself is incorrect (it isn't mapped in G-stage) then it looks to
me that access fault should be generated. But in this case I think we
won't be here (in handle_guest_page_fault() at all) as just a page fault
will be generated (so G-stage fault), not guest page fault (VS-fault
what is the case in the code above but as I told in prev paragraph
access fault is too much in that case and just page fault will be enough
and it looks like it is correspond to hardware behavior).
+ *insn = riscv_vcpu_unpriv_read(true, regs->sepc, &utrap);
+ if ( utrap.scause )
+ {
+ /*
+ * A G-stage fault here would mean the P2M mapping of the page
+ * containing the trapped instruction disappeared after it was
+ * fetched.
Does it? What about, again, faults from VS-stage address translation while
hardware was trying to fetch an insn? That is ...
Nothing removes P2M mappings of a running domain yet,
+ * so this cannot happen.
... the necessary P2M mapping may never have been there.
If VS-stage failed then CAUSE_LOAD_PAGE_FAULT will happen so BUG_ON()
won't occur and it will be passed to guest to handle it.
Are you sure? So far it was my understanding that CAUSE_LOAD_PAGE_FAULT
would happen when VS-stage translation hits e.g. a non-present leaf
entry. But got an address translation failure while doing the VS-stage
page walk (i.e. failure to translate the address found in a VS-stage
PTE to a host address) would raise CAUSE_LOAD_GUEST_PAGE_FAULT.
Sorry, you are right. Then what I suggested before instead of BUG_ON()
will be enough:
if ( is_load_guest_page_fault(utrap.scause) )
utrap.scause = CAUSE_FETCH_ACCESS;
as if we don't have mapping in G-stage then it looks like guest is
trying to reach something wrong.
BUG_ON() here catches CAUSE_LOAD_GUEST_PAGE_FAULT (G-stage translation
failure).
Also, as I mentioned above I will change BUG_ON() too:
/*
* If during getting of trapped instruction a fault happen in
* G-stage translation then CAUSE_LOAD_GUEST_PAGE_FAULT is
* generated. Such faults during this operation is
considered as
* bus
*/
What is "bus" here (dym "bug"?), and why is the sentence unfinished?
+ * TODO: Revisit once P2M mappings can be removed at runtime.
+ */
+ BUG_ON(is_load_guest_page_fault(utrap.scause));
+
+ utrap.sepc = regs->sepc;
+ utrap.stval = utrap.sepc;
How do you know the fault was at .sepc? A 32-bit insn crossing a page boundary
(implying the C extension is available) may well fault only on its higher half.
According to the spec, if stval is written with a nonzero value when an
instruction access-fault or page-fault exception occurs on a system with
variable-length instructions, then stval will contain the virtual
address of the portion of the instruction that caused the fault, while
sepc will point to the beginning of the instruction.
So here, we are trying to emulate what real hardware will do in this
case. In regs->sepc, we have the start of the instruction that we didn't
touch. sepc is filled according to the spec in this case.
Right, but utrap.stval is set to the same value, which is explicitly not
in line with what you say above ("will contain the virtual address of the
portion of the instruction that caused the fault").>
Regarding utrap.stval, we know that utrap.sepc points to the correct
part of the faulting address, as we are reading the instruction in
16-bit chunks:
HLVX_HU(%[val], %[addr]) ; low 16 bits from sepc
andi %[tmp], %[val], 3
addi %[tmp], %[tmp], -3
bne %[tmp], zero, 2f ; if not (insn & 3) == 3 -> 16-bit, end
addi %[addr], %[addr], 2 ; <- addr is now sepc+2
HLVX_HU(%[tmp], %[addr]) ; high 16 bits, possibly from another page
So, if a trap happens while reading the high 16 bits (which may be
located on another page), then utrap.sepc, if the read fails, will point
to the high part of the instruction, which is what the spec requires.
Does that make sense?
Not really, no. As said above - the code as written guarantees
utrap.stval == utrap.sepc, and that cannot always be correct.
Oh, right, utrap.stval = utrap.sepc; should be just dropped utrap.stval
already has a correct value (from ex_handler_trap_info()) which should
be passed to guest.
+/*
+ * Check alignment and dispatch a decoded MMIO access to a registered
+ * handler. On success (0), info->data holds the read value for loads.
+ */
+static int do_mmio(mmio_info_t *info, unsigned long fault_addr,
+ unsigned int len)
+{
+ /* Fault address should be aligned to length of MMIO */
+ if ( fault_addr & (len - 1) )
+ return -EIO;
+
+ info->gpa = fault_addr;
+ info->len = len;
+
+ switch ( try_handle_mmio(info) )
+ {
+ case IO_HANDLED:
+ return 0;
+ case IO_ABORT:
+ return -EIO;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
And there's no indication of "retry needed", e.g. when something changed
between find_mmio_handler() and handle_{read,write}()?
I don't have any specific scenario where it is needed now so I don't
know what to say.
And there is no race between find_mmio_handler() and
handle_{read,write}() as find_mmio_handler() returns copy of the
structure under read_lock():
Oh, right, but that's not visible here at all and requires going back to
patch 04 to realize.
I will update the comment above function:
/*
* Check alignment and dispatch a decoded MMIO access to a registered
* handler. On success (0), info->data holds the read value for loads.
*
* There is no "retry" outcome to handle: find_mmio_handler() returns a
* copy of the matching handler taken under vmmio->lock and the ops
* structures are never freed, so the lookup result cannot go stale
* between finding the handler and invoking it.
*/
static int emulate_load(unsigned long fault_addr, unsigned long htinst)
{
- return -EOPNOTSUPP;
+ struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
+ mmio_info_t info = { .is_write = false };
+ unsigned long insn;
+ unsigned int shift = 0, len, insn_len;
+ bool is_unsigned = false;
+ int rc;
+
+ if ( decode_trapped_insn(htinst, &insn, &insn_len) )
+ return 0;
+
+ /* Decode length of MMIO and whether it is a sign- or zero-extending load
*/
+ if ( (insn & INSN_MASK_LB) == INSN_MATCH_LB )
+ len = 1;
+ else if ( (insn & INSN_MASK_LBU) == INSN_MATCH_LBU )
+ {
+ len = 1;
+ is_unsigned = true;
+ }
+ else if ( (insn & INSN_MASK_LH) == INSN_MATCH_LH )
+ len = 2;
+ else if ( (insn & INSN_MASK_LHU) == INSN_MATCH_LHU )
+ {
+ len = 2;
+ is_unsigned = true;
+ }
+ else if ( (insn & INSN_MASK_LW) == INSN_MATCH_LW )
+ len = 4;
Already up to here this demonstrates a weakness of the INSN_MASK_*
set of #define-s (which I similarly observe in binutils, and I expect it
all has the same questionable origin). All INSN_MASK_L* and INSN_MASK_FL*
(also INSN_MASK_S* and INSN_MASK_FS*) are identical, allowing for a nice
switch() to be used here in principle. That said, with access width
nicely encoded in FUNCT3, it's not even clear whether a switch() would
end up being needed / efficient.
.......
Otoh none of these masks cover the pseudoinsns that htinst may supply.
As I answered above we should handle that before this function will call
so here we won't deal with htinst at all. Of course, if what I wrote
above is correct. I will double check before applying that.
Further, what about A-extension insns? Some (if not all) of them can
plausibly be used on MMIO, I think.
I’m not really sure that the A-extension is actively used for MMIO.
Does the spec preclude their use? I'm unaware of such a restriction.
Definitely no. In this case hypervisor will tell that we can't emulate
this instruction and then extra handling should be added.
At
least, Linux doesn’t do that for now, which is why we don’t handle
A-extension instructions here.
Focusing on what present Linux needs is okay, but then remaining gaps
should (as said on various other occasions before) be clearly marked.
I think this is related to the fact that MMIO is usually (if not
always?) naturally aligned, and naturally aligned loads and stores are
guaranteed by RISC-V to execute atomically.
How does this matter, when a bit or field in MMIO may serve the purpose
of e.g. a semaphore?
Then yes it will be an issue and such instruction should be emulated
(when such use cases will come into play)
+ {
+ len = 4;
+ is_unsigned = true;
+ }
+#endif
+ else if ( (insn & INSN_MASK_C_LW) == INSN_MATCH_C_LW )
+ {
+ len = 4;
+ insn = RVC_RS2S(insn) << SH_RD;
+ }
+ else if ( (insn & INSN_MASK_C_LWSP) == INSN_MATCH_C_LWSP &&
+ RV_X(insn, SH_RD, 5) )
+ len = 4;
+#ifndef CONFIG_RISCV_32
+ else if ( (insn & INSN_MASK_LD) == INSN_MATCH_LD )
+ len = 8;
+ else if ( (insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD )
+ {
+ len = 8;
+ insn = RVC_RS2S(insn) << SH_RD;
+ }
+ else if ( (insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
+ RV_X(insn, SH_RD, 5) )
+ len = 8;
+#endif
+ else
+ return -EOPNOTSUPP;
Because you don't permit F/D/Q for guests (yet), FL* and FS* aren't
covered, I expect?
At the moment, I wrote this function with handling of MMIO instruction
in mind, which are at the moment ld and sd.
Even if to permit F/D/Q then do we really need to trap that
instructions? Hypervisor could allow access to FPU to guest and then it
will be just a question of context switch to properly save and restore FPU.
And how would you know FPU loads/stores aren't used against MMIO? Later
on, once V support is added, even its loads/stores might be used that way.
Think of video frame buffer accesses, for example.
We will get 'return -EOPNOTSUPP' and so guest will be crashed because we
don't support such work with MMIO.
And then yes it should be likely to be added in parallel with adding
F/D/Q support for guest. At the moment, KVM supports, for example, F/D/Q
but doesn't emulate FPU load/store but I agree that with your example it
could happen.
~ Oleksii
|
