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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/03/2026 4:44 pm, Oleksii Kurochko wrote:
> 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
> @@ -78,6 +80,36 @@
>                             : "memory" );                        \
>  })
>  
> +static always_inline bool csr_allowed_read(unsigned long csr,
> +                                           unsigned long *val)

$FOO_safe() is the common nomenclature.  So csr_read_safe() in this case.

csr_allowed_read() reads as if it's a predicate, which this is not.

> +{
> +    bool error = false;
> +
> +    /*
> +     * 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.

Honestly, this is mostly tutorial level "how to asm", and isn't really
appropriate.

> +     */
> +    asm volatile (

asm_inline.  Especially important for inlining decisions when using
ASM_EXTABLE().

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

err needs & dropping.

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

You should write a second form with CONFIG_CC_HAS_ASM_GOTO_OUTPUT right
away.  See x86's rdmsr_safe() for the equivalent of this function.

Code generation with ASM_EXTABLE() is far better when the fixup label
can be used.

~Andrew