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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/26/26 1:53 PM, Jan Beulich wrote:
> On 26.01.2026 13:30, Oleksii Kurochko wrote:
> > 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().
> While writing, did you maybe notice that "deep" can have two entirely distinct
> meanings here? It could be "far from where the stack starts when we enter the
> hypervisor" or "far from present top of stack".
Yeah, but at time when I was writing the commit I thought only about one meaning
"far from where the stack starts when we enter the hypervisor".


> > 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.
> > +     */
> Yes please.
>
> > > 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.
> Oh, right, I forgot about that aspect. However, the more that you reference ...
>
> > 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.
> ... this - why would gp then need saving? That ought to be stable across Xen as
> well (or not be used at all)?
Totally agree, that why I mentioned in reply that it could (it would be better 
if
"must/should" were used) be dropped as it shouldn't be preserved across calls 
and
as you also notice that it ought to be stable across Xen as well (or not be used
at all).

~ Oleksii