|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 38/39] xen/riscv: implement continue_new_vcpu()
On 9/21/26 2:12 PM, Jan Beulich wrote: On 27.08.2026 17:21, Oleksii Kurochko wrote: It is requirement for boot one. For secondary processors it is HSM boot data which is passed to sbi_hsm_hart_start() and then intercpeted by Xen. At the moment of writing of this commit message we have only boot CPU and so only DTB could be passed. I can update the commit message and the comment in return_to_new_vcpu() to tell that it could be DTB address for boot cpu and/or for secondary CPUs HSM boot data or it will be better to add info about HSM boot data during and an introduction of secondary CPUs support? Interrupts have to stay disabled across the restore. The trap entry logic implicitly clears hstatus.SPV, so an interrupt taken between the write of hstatus and sret would make sret return to HS-mode instead of VS-mode, and restoring SPV afterwards is non-trivial. Instead interrupts are simply kept off and sstatus.SPIE is set, so that SIE is restored from SPIE once sret has been executed.As written this reads as if the guest would be responsible for doing this. Isn't it rather SRET itself which does this? IIUC to which part you refer then yes, it is SRET itself which does this. So some re-wording should be done ... Also, it follows what hardware will do with real CPU which is also started with interrupts disabled.Further up, aiui, you talk about the host's interrupt state. How vCPU-s are started, however, is virtual interrupt state. Mixing both isn't very helpful. ...: Interrupts have to stay disabled across the restore. The trap entry logic implicitly clears hstatus.SPV, so an interrupt taken between the write of hstatus and sret would make sret return to HS-mode instead of VS-mode, and restoring SPV afterwards is non-trivial. Hence interruptsare kept disabled, and sstatus.SPIE is set so that sret itself re-enables them (SIE := SPIE) as part of entering the guest. Then it will be also need to update the comment inside continue_new_vcpu() to:
- * To avoid this, interrupts are kept disabled during the restore.
- * Additionally, setting sstatus.SPIE=1 ensures that after sret is
- * executed (as sstatus.SIE will be loaded from SPIE), HS-mode will
- * continue to receive interrupts normally.
+ * To avoid this, interrupts are kept disabled during the restore,
+ * and sstatus.SPIE is set so that sret itself re-enables them
+ * (SIE := SPIE) as part of entering the guest.
*/
Would it be the wording okay for you now?
Introduce get_cpu_info() and reset_stack_and_jump() in asm/current.h, needed by the above. get_cpu_info() is a macro rather than a static inline because asm/current.h is pulled in by <xen/percpu.h> before this_cpu() is defined and before <xen/sched.h> completes struct vcpu.This is odd, given the similarity to Arm. They get away without using "current", and hence without using this_cpu().
Arm calculates struct cpu_info * based on sp register + STACK_SIZE:
static inline struct cpu_info *get_cpu_info(void)
{
#ifdef __clang__
unsigned long sp;
asm ("mov %0, sp" : "=r" (sp));
#else
register unsigned long sp asm ("sp");
#endif
return (struct cpu_info *)((sp & ~(STACK_SIZE - 1)) +
STACK_SIZE - sizeof(struct cpu_info));
}
what is equal to current->arch.cpu_info as I can see based on how both
arch-es are initializing v->arch.cpu_info what is used in RISC-V.
--- a/xen/arch/riscv/domain.c +++ b/xen/arch/riscv/domain.c @@ -8,10 +8,13 @@ #include <xen/smp.h> #include <xen/vmap.h>+#include <asm/aia.h>+#include <asm/aplic.h> #include <asm/bitops.h> #include <asm/cpufeature.h> #include <asm/csr.h> #include <asm/current.h> +#include <asm/imsic.h> #include <asm/intc.h> #include <asm/mmio.h> #include <asm/riscv_encoding.h>What makes these additions necessary here? None, they are leftovers from an earlier version of this patch. I'll drop them; <asm/imsic.h> will be added by the patch which starts using imsic_vsfile_attach().
Will add.
Agreed, the "else" isn't needed here. I kept it only because it looked more symmetric to me, but I'll drop it and move its body out to the same level as the if().
There was no real criterion (and SPIE is set here only for better explanation of the comment). I'll move all the CSR setup sret depends on (hstatus, sepc, sstatus.SPP and sstatus.SPIE) into return_to_new_vcpu(),like the regular return-to-guest path in entry.S already does, and set SPP and SPIE with a single CSR access there. Only local_irq_disable() will stay in continue_new_vcpu(). I will do the following: + * return_to_new_vcpu() sets up hstatus.SPV, sstatus.SPP and sepc so + * that sret enters the guest in VS-mode. A trap taken in HS-mode+ * overwrites all of them (trap entry clears hstatus.SPV in particular), + * so interrupts have to stay disabled until sret. They are re-enabled by
+ * sret itself, as return_to_new_vcpu() also sets sstatus.SPIE.
*/
local_irq_disable();
- csr_set(CSR_SSTATUS, SSTATUS_SPIE);
-
- csr_write(CSR_HSTATUS, vcpu_guest_cpu_user_regs(current)->hstatus);
reset_stack_and_jump(return_to_new_vcpu);
and then:
-/* t0 is used as a temporary reg and is clobbered to oblivion */
+/*
+ * Enter a vCPU for the first time. Must be called with interrupts
disabled,
+ * see continue_new_vcpu().
+ *
+ * t0 is used as a temporary reg and is clobbered to oblivion.
+ */
FUNC(return_to_new_vcpu)
/* Swap tp with sscratch */
csrrw tp, CSR_SSCRATCH, tp
/* Set vCPU registers */
+ REG_L t0, CPU_USER_REGS_HSTATUS(sp)
+ csrw CSR_HSTATUS, t0
+
REG_L t0, CPU_USER_REGS_SEPC(sp)
- csrw sepc, t0
+ csrw CSR_SEPC, t0
/* Hartid goes to a0 */
REG_L a0, CPU_USER_REGS_A0(sp)
/* DTB goes to a1 */
REG_L a1, CPU_USER_REGS_A1(sp)
- /* Set guest mode to supervisor */
- li t0, SSTATUS_SPP
+ /* Return to (V)S-mode, with interrupts re-enabled by sret */
+ li t0, SSTATUS_SPP | SSTATUS_SPIE
csrs CSR_SSTATUS, t0
/* Enter guest */
sret
END(return_to_new_vcpu)
--- a/xen/arch/riscv/entry.S +++ b/xen/arch/riscv/entry.S @@ -143,3 +143,26 @@ FUNC(__context_switch)retEND(__context_switch) + +/* t0 is used as a temporary reg and is clobbered to oblivion */ +FUNC(return_to_new_vcpu) + /* Swap tp with sscratch */ + csrrw tp, CSR_SSCRATCH, tpWhat is this about? I'm not aware of any counterpart code, yet all on its own this I can't see it being overly useful. It will be needed later when a guest will be able to launch to distinguish in handle_trap() [1] if a trap is from guest or not. I can drop it for now and re-introduce it with the code of handle_trap() or as an option I could update the comment above to:
/*
* SSCRATCH holds this hart's struct pcpu_info while a guest
runs and
* is zero while Xen runs, so that a trap handler can tell the two
* apart: tp is Xen's pointer to pcpu_info in Xen context, but
belongs
* to the guest once sret has been executed. Establish that by
swapping
* the two here; the trap path swaps them back.
*/
csrrw tp, CSR_SSCRATCH, tp
(but then the last part will point to the part which isn't yet
introduced so probably it will be better to drop this line for now)
[1] https://gitlab.com/xen-project/people/olkur/xen/-/blob/riscv-next-upstreaming/xen/arch/riscv/entry.S?ref_type=heads&blame=1#L15 + /* Set vCPU registers */ + REG_L t0, CPU_USER_REGS_SEPC(sp) + csrw sepc, t0 + + /* Hartid goes to a0 */ + REG_L a0, CPU_USER_REGS_A0(sp) + + /* DTB goes to a1 */ + REG_L a1, CPU_USER_REGS_A1(sp)The fields loaded are merely .a0 and .a1 of the register struct. There's nothing here making sure (all on its own) that what is loaded is what is said by the comments. If e.g. the first comment was /* .a0 holds the hart id */ or some such to remind readers what is being loaded without giving the impression that the correct value is _established_ here, that may be better.
I will re-word the comments in suggested way:
- /* Hartid goes to a0 */
+ /* .a0 holds the hart id */
REG_L a0, CPU_USER_REGS_A0(sp)
- /* DTB goes to a1 */
+ /* .a1 holds the address of the DTB */
REG_L a1, CPU_USER_REGS_A1(sp)
+ /* Set guest mode to supervisor */ + li t0, SSTATUS_SPP + csrs CSR_SSTATUS, t0 + + /* Enter guest */ + sret +END(return_to_new_vcpu)Aiui SRET does not switch stacks. Shouldn't you therefore clear sp here? And perhaps also other GPRs, not the least ra? Exposing hypervisor register values to guests is, well, a bit of a problem. Good point, sret leaves all GPRs as they are, so the guest would indeed see Xen's sp, ra and friends. Only a0 and a1 are architecturally meaningful for a booting hart, so I'll clear every other GPR right before sret.
I will add the following before sret:
/*
* sret doesn't switch stacks and leaves the GPRs alone, so every
* register which isn't meaningful to the vCPU being started
has to be
* cleared here: otherwise the guest would see Xen's values, sp
(this
* vCPU's Xen stack) and ra among them.
*/
.irp reg, ra, sp, gp, tp, t0, t1, t2, s0, s1, a2, a3, a4, a5,
a6, a7, \
s2, s3, s4, s5, s6, s7, s8, s9, s10, s11, t3, t4, t5, t6
mv \reg, zero
.endr
Thanks.
~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |