Re: [PATCH v1 2/4] xen/riscv: add csr_allowed_read() helper

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.03.2026 17:44, Oleksii Kurochko wrote:
> Accessing some CSRs may trap when the corresponding extension is not
> implemented or enabled. Introduce csr_allowed_read() which attempts to
> read a CSR and relies on the exception table mechanism to safely recover
> if the access faults.
> 
> This helper allows Xen to probe CSR availability without taking a fatal
> trap and will be used for feature detection during early boot as we
> can't always rely on what is in riscv,isa string in DTS.
> 
> While touching the header, reorder the include directives to follow the
> usual Xen style.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>  xen/arch/riscv/include/asm/csr.h | 34 +++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/riscv/include/asm/csr.h 
> b/xen/arch/riscv/include/asm/csr.h
> index 01876f828981..b9bee3d25d21 100644
> --- a/xen/arch/riscv/include/asm/csr.h
> +++ b/xen/arch/riscv/include/asm/csr.h
> @@ -6,8 +6,10 @@
>  #ifndef ASM__RISCV__CSR_H
>  #define ASM__RISCV__CSR_H
>  
> -#include <asm/asm.h>
>  #include <xen/const.h>
> +
> +#include <asm/asm.h>
> +#include <asm/extables.h>
>  #include <asm/riscv_encoding.h>
>  
>  #ifndef __ASSEMBLER__
> @@ -78,6 +80,36 @@
>                             : "memory" );                        \
>  })
>  
> +static always_inline bool csr_allowed_read(unsigned long csr,
> +                                           unsigned long *val)
> +{
> +    bool error = false;

Wrong polarity or wrong name? You set this ...

> +    /*
> +     * Use "+" as a constraint instead of "=" to ensure the compiler passes 
> the
> +     * initial value into the asm volatile block. Otherwise, if the 
> instruction
> +     * (at label 1) faults, the variable 'error' may contain an undefined 
> value
> +     * instead of 0.
> +     * If reading of CSR register was failed, we don't care about val, so "="
> +     * constraint could be used in asm volatile block to not force always 
> init.
> +     * val argument before being passed to csr_allowed_read() functions.
> +     *
> +     * This avoids the need for an additional instruction inside the asm 
> block
> +     * to explicitly initialize 'error' to 0 before executing the potentially
> +     * faulting instruction.
> +     */
> +    asm volatile (
> +        "1: csrr %[val], %[csr]\n"
> +        "   li %[err], 1\n"

... to true in the success case.

Please can all of the commentary be dropped? You're re-stating what the compiler
doc has, and (imo) such doesn't belong here. If you want to comment on anything,
then the (again imo) less obvious need to use always_inline.

> +        "2:\n"
> +        ASM_EXTABLE(1b, 2b)
> +        : [val] "=&r" (*val), [err] "+&r" (error)

You don't clobber [err] early, so & isn't needed here.

> +        : [csr] "i" (csr)
> +        : "memory" );

The memory clobber, if you think you really need it, wants to come with
a comment.

Jan