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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 1/6/26 3:26 PM, Jan Beulich wrote:
> 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 <oleksii.kurochko@gmail.com>
>>> 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.

But the guest doesn't put there nothing, only Xen does that and it is a reason
why I am trying to call it Xen state. Guest works only with what is stored in
struct cpu_info->guest_cpu_user_regs.* ...


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

... because guest doesn't access saved_context and as I mentioned above
guest has "access" only to struct cpu_info->guest_cpu_user_regs.*.

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

I haven't measured, but I saw that Arm has and it was my explanation to
myself to put it for RISC-V too.

~ Oleksii