Re: [PATCH v1 12/15] xen/riscv: introduce sbi_set_timer()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.12.2025 18:03, Oleksii Kurochko wrote:
> Introduce pointer to function which points to a specific sbi_set_timer()
> implementation. It is done in this way as different OpenSBI version can
> have different Extenion ID and/or funcion ID for TIME extension.
> 
> sbi_set_time() programs the clock for next event after stime_value
> time. This function also clears the pending timer interrupt bit.
> 
> Introduce extension ID and SBI function ID for TIME extension.
> 
> Implement only sbi_set_timer_v02() as there is not to much sense
> to support earlier version and, at the moment, Xen supports only v02.

Besides this somewhat contradicting the use of a function pointer: What
about the legacy extension's equivalent?

> --- a/xen/arch/riscv/include/asm/sbi.h
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -33,6 +33,7 @@
>  
>  #define SBI_EXT_BASE                    0x10
>  #define SBI_EXT_RFENCE                  0x52464E43
> +#define SBI_EXT_TIME                    0x54494D45
>  
>  /* SBI function IDs for BASE extension */
>  #define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
> @@ -65,6 +66,9 @@
>  
>  #define SBI_SPEC_VERSION_DEFAULT 0x1
>  
> +/* SBI function IDs for TIME extension */
> +#define SBI_EXT_TIME_SET_TIMER  0x0

Move up besides the other SBI_EXT_* and use the same amount of padding?

> @@ -138,6 +142,19 @@ int sbi_remote_hfence_gvma(const cpumask_t *cpu_mask, 
> vaddr_t start,
>  int sbi_remote_hfence_gvma_vmid(const cpumask_t *cpu_mask, vaddr_t start,
>                                  size_t size, unsigned long vmid);
>  
> +/*
> + * Programs the clock for next event after stime_value time. This function 
> also
> + * clears the pending timer interrupt bit.
> + * If the supervisor wishes to clear the timer interrupt without scheduling 
> the
> + * next timer event, it can either request a timer interrupt infinitely far
> + * into the future (i.e., (uint64_t)-1), or it can instead mask the timer
> + * interrupt by clearing sie.STIE CSR bit.
> + *
> + * This SBI call returns 0 upon success or an implementation specific 
> negative
> + * error code.
> + */
> +extern int (*sbi_set_timer)(uint64_t stime_value);

Despite the pretty extensive comment, the granularity of the value to be passed
isn't mentioned.

> --- a/xen/arch/riscv/sbi.c
> +++ b/xen/arch/riscv/sbi.c
> @@ -249,6 +249,26 @@ static int (* __ro_after_init sbi_rfence)(unsigned long 
> fid,
>                                            unsigned long arg4,
>                                            unsigned long arg5);
>  
> +static int cf_check sbi_set_timer_v02(uint64_t stime_value)
> +{
> +    struct sbiret ret;
> +
> +#ifdef CONFIG_RISCV_64
> +    ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0,
> +                    0, 0, 0, 0);
> +#else
> +    ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
> +                    stime_value >> 32, 0, 0, 0, 0);
> +#endif

How about

    ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
#ifdef CONFIG_RISCV_64
                    0,
#else
                    stime_value >> 32,
#endif
                    0, 0, 0, 0);

? Granted some may say this looks a little m ore clumsy, but it's surely
less redundancy.

Also I'd suggest to use CONFIG_RISCV_32 with the #ifdef, as then the "else"
covers both RV64 and RV128.

> +    if ( ret.error )
> +        return sbi_err_map_xen_errno(ret.error);
> +    else
> +        return 0;
> +}

While I understand this is being debated, I continue to think that this
kind of use of if/else isn't very helpful. Function's main return
statements imo benefit from being unconditional.

Jan