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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.09.2026 10:23, Oleksii Kurochko wrote:
> On 9/21/26 2:12 PM, Jan Beulich wrote:
>> 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.)
> 
> 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.

That you're talking about Xen. Despite Xen being UP only right now, guests
still can have more than one vCPU, can't they?

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

As per above you want to deal with multi-vCPU guests right now.

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

I think so, yes.

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

Yes, introducing it together with the other related pieces is going to be
more consistent and easier to follow.

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

At which point discussing the clobbering of t0 in the comment ahead of
the function also isn't needed anymore.

Jan