|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 13/17] xen/riscv: add unprivileged guest memory read helper
On 8/12/26 5:30 PM, Jan Beulich wrote: On 20.07.2026 18:02, Oleksii Kurochko wrote:Introduce riscv_vcpu_unpriv_read() to allow Xen to safely read guest memory using HLV/HLVX instructions while reliably capturing trap context.Both for the title and the function name: How does "unprivileged" matter here? Unprivileged because HLV/HLVX reads guest memory as if it were accessed from a less-privileged (guest) context, rather than by the hypervisor in HS mode. I think I am okay generally to drop "unpriv..." from the function name. The same functions would be use for reading Dom0's memory, wouldn't they? Yes, I don't see any issue to let this function to read DomO's memory too. But Dom0 could be counted as "unprivileged" too as it is executed in VS-mode which is less-privileged then HS-mode.
Agree, it will be better. I will update prototype of the function.
Agree, the wording is incorrect what I meant it is that we don't want to corrupt hstatus which was saved during guest exit to hypervisor. I will just put the following comment "As hstatus is going to be changed we don't want an interrupt to change it". + local_irq_save(flags); + + /* + * The hypervisor virtual-machine load and store instructions are valid + * only in M-mode or HS-mode, or in U-mode when hstatus.HU=1. Each + * instruction performs an explicit memory access as though V=1; i.e., + * with the address translation and protection, and the endianness, + * that apply to memory accesses in either VS-mode or VU-mode. + * Field SPVP of hstatus controls the privilege level of the access. + * The explicit memory access is done as though in VU-mode when SPVP=0, + * and as though in VS-mode when SPVP=1. + * + * So it is necessary to restore vCPU's hstatus before execution of + * hlv* instruction. + */ + old_hstatus = csr_swap(CSR_HSTATUS, + vcpu_guest_cpu_user_regs(current)->hstatus);As you're limiting use of the function to the current vCPU, why would hstatus need fiddling with? The fields of interest aren't being altered between exit from guest and making it here, are they? You're right, it doesn't. handle_trap() only saves hstatus into the trap frame on entry and restores it before sret; nothing in between installs a hypervisor-specific value. So the guest's hstatus (in particular SPVP, the only field HLV cares about here (HU only matters in U-mode) ) is still live when we get here. Traps taken from HS-mode update only SPV and GVA, neither of which affects HLV. Without that IRQs also wouldn't need turning off (what about NMIs, btw, once supported on Xen?), which would help real-time use cases (latency here can otherwise be affected by guests, by wait of forcing exceptions to be raised). Correct, and I'll drop local_irq_save() too. In fact it is already a no-op at both call sites: do_trap() runs with interrupts disabled by the trap entry itself. And even with the hstatus write in place it would not have been needed: any nested trap goes through the same entry path, which saves and restores hstatus (an NMI path built the same way would be safe for the same reason, rather than relying on interrupts being masked). What I will do instead is document and assert the actual precondition: the function may only be called on the trap-handling path of the current vCPU, before returning to the guest. That is where hstatus, vsatp and hgatp are guaranteed to still be that vCPU's. do_trap() reaches check_for_pcpu_work(), and hence any reschedule, only after handling is done. The following check I will add instead csr_swap() and local_irq_save(): ASSERT(vcpu_guest_cpu_user_regs(current)->hstatus & HSTATUS_SPV);
I will add the following comment above the function:* At most two halfwords are fetched when @read_insn is true, i.e. encodings * wider than 32 bits are not supported. Such an encoding cannot be completed * by calling this function again at @guest_addr + 4: the length check is* applied to the first halfword read, which would then be a continuation of * the instruction rather than its opcode. It is up to the caller to reject * anything that is neither a 16- nor a 32-bit encoding. (How they would do that is entirely unclear to me, as they can't simply invoke this function again passing guest_addr + 4.)
Then it will be needed to update the code of riscv_unpriv_read().
For now we could something like:
/*
* Only two halfwords are fetched, so an encoding wider than 32
bits
* would have been truncated. Report it as illegal with a zero
stval:
* a nonzero one would have to hold the actual faulting
instruction,
* whereas zero simply means the value isn't provided.
*/
if ( !INSN_IS_16BIT(insn) && !INSN_IS_32BIT(insn) )
return truly_illegal_insn(v, 0);
+ : [val] "=&r" (val), [tmp] "=&r" (tmp), [addr] "+&r" (guest_addr) + : [ti] "r" (trap) : "memory" );You want to tell the compiler that *trap is written. Instead I don't see why a memory clobber would be needed: You access a different address space, i.e. nothing the compiler can make any assumptions about. memory clobber tells the compiler that the assembly code performs memory reads or writes to items other than those listed in the input and output operands and so I don't tell here that *trap will be changed.
Why this understanding is wrong?
Alternative, I think, could be:
: [val] "+r" (val), "+m" (*trap)
: [addr] "r" (guest_addr), [ti] "r" (trap) );
And then memory clobber could be dropped.
You also need to take precautions for not returning an uninitialized "val". I think the variable wants initializing (perhaps to ~0) and "+r" wants using as constraint. (Afaik & isn't necessary to use together with +.) I agree with '+' if we will initialize val with some value. Regarding, '&' my understanding is that I have to use it always when
Sure, I will do the following:
#if defined(CONFIG_RISCV_64)
"hlv.d %[val], (%[addr])\n"
#elif defined(CONFIG_RISCV_32)
"hlv.w %[val], (%[addr])\n"
#else
#error "unsupported RISC-V variant: no hlv for a machine word"
#endif
+ "2:\n" + ASM_EXTABLE_TRAP_INFO(1b, 2b, %[ti]) + : [val] "=&r" (val) + : [addr] "r" (guest_addr), [ti] "r" (trap) : "memory" ); + } + + csr_write(CSR_HSTATUS, old_hstatus); + + local_irq_restore(flags); + + return val; +}For both reads and fetches - are there no alignment constraints at all on the incoming guest_addr? For the fetch path there is an implicit constraint, but the architecture guarantees it: guest_addr is always the guest's sepc, and IALIGN is 16 bits (32 without the C extension), so it cannot be odd, the guest wouldhave taken an instruction-address-misaligned exception before we ever saw this trap. hlvx.hu is then a naturally aligned halfword access, and advancing by 2 preserves that.For the data read there is deliberately no constraint: HLV behaves as the guest's own access would, so on a hart which handles misaligned accesses it simply works, and on one which doesn't it raises load-address-misaligned, which the exception table turns into trap->scause for the caller to redirect. But then it will be need to: --- a/xen/arch/riscv/traps.c +++ b/xen/arch/riscv/traps.c@@ -565,51 +565,73 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
void do_trap(struct cpu_user_regs *cpu_regs)
{
register_t pc = cpu_regs->sepc;
unsigned long cause = csr_read(CSR_SCAUSE);
+ /*
+ * A synchronous trap taken while Xen itself was running may come
from an
+ * access done on a vCPU's behalf, e.g. the hlv/hlvx sequences in+ * riscv_vcpu_unpriv_read(). Those accesses are covered by exception table + * entries which record the fault details for the caller and resume + * execution past the faulting instruction. + *+ * Interrupts must be excluded here: one taken at an address which happens + * to be listed in the exception table would otherwise be "fixed up" as if
+ * the access itself had faulted, silently skipping it.
+ *
+ * Returning early skips check_for_pcpu_work() below, which is correct:
+ * that only runs for traps taken from the guest.
+ */
+ if ( !(cause & CAUSE_IRQ_FLAG) && !(cpu_regs->hstatus & HSTATUS_SPV) &&
+ fixup_exception(cpu_regs) )
+ return;
+
switch ( cause )
{
case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
/* CAUSE_VIRTUAL_SUPERVISOR_ECALL should come from VS-mode */
BUG_ON(!(cpu_regs->hstatus & HSTATUS_SPV));
vsbi_handle_ecall(cpu_regs);
break;
case CAUSE_LOAD_GUEST_PAGE_FAULT:
case CAUSE_STORE_GUEST_PAGE_FAULT:
+ /*
+ * Anything not recovered by the exception table above must
have come
+ * from the guest: a G-stage fault taken in Xen context, e.g. by an
+ * hlv/hlvx not covered by an entry, is a bug.
+ */
+ BUG_ON(!(cpu_regs->hstatus & HSTATUS_SPV));
+
handle_guest_page_fault(cause, cpu_regs);
break;
case CAUSE_VIRTUAL_INST_FAULT:
{
int ret;
BUG_ON(!(cpu_regs->hstatus & HSTATUS_SPV));
ret = handle_virt_instruction_fault(current);
if ( ret < 0 )
/* TODO: crash only domain instead of Xen? */
/* domain_crash(current->domain); */
panic("couldn't handle CAUSE_VIRTUAL_INST_FAULT: %d\n", ret);
break;
}
case CAUSE_ILLEGAL_INSTRUCTION:
if ( do_bug_frame(cpu_regs, pc) >= 0 )
{
if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) )
{
printk("Something wrong with PC: %#lx\n", pc);
die();
}
cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
break;
}
- if ( fixup_exception(cpu_regs) )
- break;
-
fallthrough;
default:
if ( cause & CAUSE_IRQ_FLAG )
{
/* Handle interrupt */
Does it make sense?
Thanks.
~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |