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
