Re: [PATCH v1 03/15] xen/riscv: implement vcpu_csr_init()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.12.2025 18:03, Oleksii Kurochko wrote:
> Implement function to initialize VCPU's CSR registers to delegate handling
> of some traps to VS-mode ( guest ), enable vstimecmp for VS-mode, and
> allow some AIA-related register (thier vs* copies ) for VS-mode.

The henvcfg setting isn't covered here at all, unless I'm failing to make the
respective association. Nor is the setting of SMSTATEEN0_HSENVCFG in hstateen0.

Overall it feels like the description here is too terse anyway, as the bits
set (or not) are a pretty crucial thing for running guests. Then again maybe
this is just me, for not being a RISC-V person ...

> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -3,6 +3,67 @@
>  #include <xen/mm.h>
>  #include <xen/sched.h>
>  
> +#include <asm/cpufeature.h>
> +#include <asm/csr.h>
> +#include <asm/riscv_encoding.h>
> +
> +static void vcpu_csr_init(struct vcpu *v)
> +{
> +    unsigned long hedeleg, hideleg, hstatus;
> +
> +    hedeleg = 0;
> +    hedeleg |= (1U << CAUSE_MISALIGNED_FETCH);
> +    hedeleg |= (1U << CAUSE_FETCH_ACCESS);
> +    hedeleg |= (1U << CAUSE_ILLEGAL_INSTRUCTION);
> +    hedeleg |= (1U << CAUSE_MISALIGNED_LOAD);
> +    hedeleg |= (1U << CAUSE_LOAD_ACCESS);
> +    hedeleg |= (1U << CAUSE_MISALIGNED_STORE);
> +    hedeleg |= (1U << CAUSE_STORE_ACCESS);
> +    hedeleg |= (1U << CAUSE_BREAKPOINT);
> +    hedeleg |= (1U << CAUSE_USER_ECALL);
> +    hedeleg |= (1U << CAUSE_FETCH_PAGE_FAULT);
> +    hedeleg |= (1U << CAUSE_LOAD_PAGE_FAULT);
> +    hedeleg |= (1U << CAUSE_STORE_PAGE_FAULT);
> +    v->arch.hedeleg = hedeleg;

Wouldn't you better start from setting all of the non-reserved bits, to then
clear the few that you mean to not delegate? Then again I'm not quite sure
whether the set of CAUSE_* in the header file is actually complete: MCAUSE
also can hold the values 16, 18, and 19. (Otoh you have CAUSE_MACHINE_ECALL,
which I don't think can ever be observed outside of M-mode.)

Also, while it may seem to not matter much, sorting the above by their numeric
values would ease comparison against the full set.

> +    hstatus = HSTATUS_SPV | HSTATUS_SPVP;
> +    v->arch.hstatus = hstatus;

Why would these (or in fact any) bits need setting here? Isn't hstatus written
upon exit from guest context?

> +    hideleg = MIP_VSTIP |  MIP_VSEIP | MIP_VSSIP;
> +    v->arch.hideleg = hideleg;

Again I think having MIP_VSTIP in the middle (to establish numeric sorting)
would be slightly better.

Also there's a stray blank after the first |.

> +    /*
> +     * VS should access only the time counter directly.
> +     * Everything else should trap.
> +     */
> +    v->arch.hcounteren |= HCOUNTEREN_TM;

Why are this and ...

> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) )
> +        v->arch.henvcfg |= ENVCFG_PBMTE;

... this using |= but the earlier ones simply = ? Unless there is a specific
reason, consistency is likely preferable.

> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> +    {
> +        /*
> +         * If the hypervisor extension is implemented, the same three bitsare
> +         * defined also in hypervisor CSR hstateen0 but concern only the 
> state
> +         * potentially accessible to a virtual machine executing in privilege
> +         * modes VS and VU:
> +         *      bit 60 CSRs siselect and sireg (really vsiselect and vsireg)
> +         *      bit 59 CSRs siph and sieh (RV32 only) and stopi (really 
> vsiph,
> +         *             vsieh, and vstopi)
> +         *      bit 58 all state of IMSIC guest interrupt files, including 
> CSR
> +         *             stopei (really vstopei)
> +         * If one of these bits is zero in hstateen0, and the same bit is one
> +         * in mstateen0, then an attempt to access the corresponding state 
> from
> +         * VS or VU-mode raises a virtual instruction exception.
> +        */
> +        v->arch.hstateen0 = SMSTATEEN0_AIA | SMSTATEEN0_IMSIC | 
> SMSTATEEN0_SVSLCT;

What is SVSLCT? Bit 60 is named CSRIND in the spec I'm looking at, and the
commentary above looks to confirm this.

Also, wouldn't you better keep internal state in line with what hardware
actually supports? CSRIND may be read-only-zero in the real register, in
which case having the bit set in the "cached" copy can be misleading.
(This may similarly apply to at least hedeleg and hideleg, btw.)

As to consistency: Further up you use local helper variables (for imo no real
reason), when here you don't. Instead this line ends up being too long.

Jan