Re: [PATCH v2 01/16] xen/riscv: introduce struct arch_vcpu

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/26/26 12:41 PM, Jan Beulich wrote:
> On 22.01.2026 17:47, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/domain.h
> > +++ b/xen/arch/riscv/include/asm/domain.h
> > @@ -22,9 +22,62 @@ struct hvm_domain
> >   struct arch_vcpu_io {
> >   };
> >   
> > -struct arch_vcpu {
> > +struct arch_vcpu
> > +{
> >       struct vcpu_vmid vmid;
> > -};
> > +
> > +    /*
> > +     * Callee saved registers for Xen's state deep in the callframe used to
> > +     * switch from prev's stack to the next's stack during context switch.
> > +     */
> What is "deep in the callframe" intended to convey? I'm in particular wondering
> about ...
>
> > +    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;
> > +        register_t ra;
> ... sp and ra, which presumably don't live anywhere "deep"?
context_switch() is invoked relatively deep in the call stack, so the stack
pointer in use when context_switch() executes can also be considered to be
deep in the call frame. The same applies to RA: after the first
__context_switch() call, RA will point to the next instruction within
context_switch().

I can update the comment and drop the wording about being “deep in the call
frame” to avoid confusion. In that case it would simply read:

+    /*
+     * Callee saved registers for Xen's state used to
+     * switch from prev's stack to the next's stack during context switch.
+     */

> Also, what about tp? The 't' in there isn't the same as that in "t0", "t1", etc.
tp stores pcpu_info[] and it isn't expected to be changed during (or between) 
function
calls.
In this structure we are dealing only with registers which should be saved 
according
to RISC-V ABI convention:
 [1] 
https://riscv-non-isa.github.io/riscv-elf-psabi-doc/#_integer_register_convention
The exception is for RA (as it is also used to jump to continue_to_new_vcpu() 
when vcpu is scheduled
first time). During a review of the [1], I think that GP could be dropped as it 
shouldn't
be preserved across calls.

> > +    } xen_saved_context;
> > +
> > +    /* CSRs */
> > +    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 */
> Why the V? These are normal CSRs dedicated to VS use, aren't they?
I meant VSCSRs, yes, it is normal CSRs dedicated to VS use.
I'll drop the comment as from the name it is clear that VS-mode CSRs.

> > +    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;
> I continue to question the presence of this attribute.
I will drop it until I could really measure that it boosts performance.
At the moment, it is just an assumption.

~ Oleksii