Re: [PATCH v1 01/15] xen/riscv: introduce struct arch_vcpu

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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 <oleksii.kurochko@xxxxxxxxx>
> 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].

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.

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.

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

~ Oleksii