Re: [PATCH v2 38/39] xen/riscv: implement continue_new_vcpu()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2026 17:21, Oleksii Kurochko wrote:
> continue_new_vcpu() is the arch hook invoked the first time a freshly
> created vCPU is scheduled. Implement both cases it has to cover:
>  - for the idle vCPU, switch to its own stack and jump to idle_loop();
>  - for a guest vCPU, restore hstatus and enter the guest through the new
>    return_to_new_vcpu() path in entry.S, which loads sepc, passes the
>    hart id in a0 and the DTB address in a1 as expected by the RISC-V
>    boot protocol, sets sstatus.SPP and executes sret.

Is this a requirement for all CPUs, or just for the boot one? (I can't
quite see why secondary processors would need passing a DTB address.)

> 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?

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

> 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().

> --- 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?

> @@ -140,9 +143,43 @@ static void vcpu_csr_init(struct vcpu *v)
>      v->arch.hie = MIP_SGEIP;
>  }
>  
> +static void schedule_tail(struct vcpu *prev);
> +static void noreturn idle_loop(void);
> +void noreturn return_to_new_vcpu(void);

For this last one: asmlinkage?

>  static void continue_new_vcpu(struct vcpu *prev)
>  {
> -    BUG_ON("unimplemented\n");
> +    schedule_tail(prev);
> +
> +    if ( is_idle_vcpu(current) )
> +        reset_stack_and_jump(idle_loop);
> +    else

This is the kind of "else" which I consider particularly confusing: It
suggests that the if() body can actually be exited at the bottom, when
(by the name "reset_stack_and_jump") it hopefully cannot.

> +    {
> +        /*
> +         * During a context switch to a new vCPU, interrupts must be disabled
> +         * to guarantee that the vCPU's CSR state can be safely restored into
> +         * the hart without being clobbered by an interrupt trap.
> +         *
> +         * For example, when return_to_new_vcpu() finishes, it executes sret.
> +         * At that point, the hart checks hstatus.SPV=1 and sstatus.SPP=1 in
> +         * order to return from HS-mode into VS-mode. If an interrupt were to
> +         * arrive before sret, the trap entry logic would implicitly clear
> +         * hstatus.SPV to 0. Correctly restoring it afterwards is 
> non-trivial,
> +         * and if left as 0, sret would incorrectly return to HS-mode instead
> +         * of VS-mode.
> +         *
> +         * 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.
> +         */
> +        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);

What are the criteria by which you split CSR accesses between doing some here
and some in return_to_new_vcpu()? In particular you set sstatus.SPIE here but
sstatus.SPP there, when both could - I think - be done with a single CSR
access.

> --- a/xen/arch/riscv/entry.S
> +++ b/xen/arch/riscv/entry.S
> @@ -143,3 +143,26 @@ FUNC(__context_switch)
>  
>          ret
>  END(__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, tp

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

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

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

Jan