On 06.01.2026 15:19, Oleksii Kurochko wrote: > On 1/5/26 5:58 PM, Jan Beulich wrote: >> On 24.12.2025 18:03, Oleksii Kurochko wrote: >>> Introduce structure with VCPU's registers which describes its state. >>> >>> Signed-off-by: Oleksii Kurochko <[email protected]> >> Since none of this is being used for the time being, I think the description >> wants to be a little less terse. Coming from the x86 (rather then the Arm) >> side, I find the arrangements irritating. And even when comparing to Arm, ... >> >>> --- a/xen/arch/riscv/include/asm/domain.h >>> +++ b/xen/arch/riscv/include/asm/domain.h >>> @@ -22,9 +22,63 @@ struct hvm_domain >>> struct arch_vcpu_io { >>> }; >>> >>> -struct arch_vcpu { >>> +struct arch_vcpu >>> +{ >>> struct vcpu_vmid vmid; >>> -}; >>> + >>> + /* Xen's state: Callee-saved registers and tp, gp, ra */ >> ... I don't think the following structure describes "Xen's state". On Arm >> it's guest controlled register values which are being saved afaict. I >> would then expect the same to become the case for RISC-V. > > I think this is not fully correct, because guest-controlled registers on > Arm are allocated on the stack [1][2].
I'll admit that I should have said "possibly guest-controlled". Callee- saved registers may or may not be used in functions, and if one isn't used throughout the call-stack reaching __context_switch(), it would still hold whatever the guest had put there. > Regarding|xen_saved_context| (or|saved_context| on Arm, which I used as a > base), > I think|xen_saved_context| is a slightly better name. Looking at how the > |saved_context| structure is used on Arm [3], it can be concluded that > |__context_switch()| switches only Xen’s internal context. What actually > happens is > that|__context_switch()| is called while running on the previous vCPU’s stack > and returns on the next vCPU’s stack. Therefore, it is necessary to have > the correct register values stored in the|saved_context| structure in order > to continue Xen’s execution when it later returns to the previous stack. For this and ... > Probably I need to introduce|__context_switch()| in this patch series for > RISC-V > now; I hope this will clarify things better. At the moment, it looks like [4]. > > [1] > https://elixir.bootlin.com/xen/v4.21.0/source/xen/arch/arm/include/asm/arm64/processor.h#L14 > [2] https://elixir.bootlin.com/xen/v4.21.0/source/xen/arch/arm/domain.c#L547 > > [3] > https://elixir.bootlin.com/xen/v4.21.0/source/xen/arch/arm/arm64/entry.S#L650 > > [4] > https://gitlab.com/xen-project/people/olkur/xen/-/blob/riscv-next-upstreaming/xen/arch/riscv/entry.S?ref_type=heads#L153 > >> >>> + struct >>> + { >>> + register_t s0; >>> + register_t s1; >>> + register_t s2; >>> + register_t s3; >>> + register_t s4; >>> + register_t s5; >>> + register_t s6; >>> + register_t s7; >>> + register_t s8; >>> + register_t s9; >>> + register_t s10; >>> + register_t s11; >>> + >>> + register_t sp; >>> + register_t gp; >>> + >>> + /* ra is used to jump to guest when creating new vcpu */ >>> + register_t ra; >>> + } xen_saved_context; >> The xen_ prefix here also doesn't exist in Arm code. > > I think it should be added for Arm too. I can send a patch. ... this, to reword my comment: What value does the xen_ prefix add? >> Nor is there a >> similar, partly potentially misleading comment on "pc" there >> comparable to the one that you added for "ra". ("Potentially >> misleading" because what is being described is, aiui, not the only >> and not even the main purpose of the field.) > > Yes, the purpose of|ra| here is not just to jump to the new vCPU code > (|continue_new_vcpu()|). It is used that way only the first time; > afterwards,|ra| will simply point to the next instruction after the > call to|__context_switch()| in|context_switch()| [5]. > > [5] > https://gitlab.com/xen-project/people/olkur/xen/-/blob/riscv-next-upstreaming/xen/arch/riscv/domain.c?ref_type=heads#L463 > >> >>> + /* CSRs */ >>> + register_t hstatus; >>> + register_t hedeleg; >>> + register_t hideleg; >>> + register_t hvip; >>> + register_t hip; >>> + register_t hie; >>> + register_t hgeie; >>> + register_t henvcfg; >>> + register_t hcounteren; >>> + register_t htimedelta; >>> + register_t htval; >>> + register_t htinst; >>> + register_t hstateen0; >>> +#ifdef CONFIG_RISCV_32 >>> + register_t henvcfgh; >>> + register_t htimedeltah; >>> +#endif >>> + >>> + /* VCSRs */ >>> + register_t vsstatus; >>> + register_t vsip; >>> + register_t vsie; >>> + register_t vstvec; >>> + register_t vsscratch; >>> + register_t vscause; >>> + register_t vstval; >>> + register_t vsatp; >>> + register_t vsepc; >>> +} __cacheline_aligned; >> Why this attribute? > > As arch_vcpu structure is accessed pretty often I thought it would > be nice to have it cache-aligned so some accesses would be faster > and something like false sharing won't happen. I think you would want to prove that this actually makes a difference. I notice Arm has such an attribute (and maybe indeed you merely copied it), but x86 doesn't. Jan
