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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> +    /* 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?

Jan