Re: [PATCH v6 01/14] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.03.2026 15:54, Oleksii Kurochko wrote:
> On 3/3/26 1:23 PM, Jan Beulich wrote:
>> On 26.02.2026 12:51, Oleksii Kurochko wrote:
>>> +void __init init_csr_masks(void)
>>> +{
>>> +    /*
>>> +     * The mask specifies the bits that may be safely modified without
>>> +     * causing side effects.
>>> +     *
>>> +     * For example, registers such as henvcfg or hstateen0 contain WPRI
>>> +     * fields that must be preserved. Any write to the full register must
>>> +     * therefore retain the original values of those fields.
>>> +     */
>>> +#define INIT_CSR_MASK(csr, field, mask) do { \
>>> +        old = csr_read(CSR_##csr); \
>>> +        csr_write(CSR_##csr, (old & ~(mask)) | (mask)); \
>> I (now) agree csr_swap() can't be used here, but isn't the above
>>
>>      old = csr_read_set(CSR_##csr, mask);
>>
>> ?
> 
> Agree, csr_read_set() could be used.
> 
>>> +        csr_masks.field = csr_swap(CSR_##csr, old); \
>>> +    } while (0)
>>> +
>>> +    register_t old;
>> Since the macro uses the variable, this decl may better move up.
>>
>>> +    INIT_CSR_MASK(HEDELEG, hedeleg, ULONG_MAX);
>>> +    INIT_CSR_MASK(HIDELEG, hideleg, ULONG_MAX);
>>> +
>>> +    INIT_CSR_MASK(HENVCFG, henvcfg, _UL(0xE0000003000000FF));
>> This raw hex number (also the other one below) isn't quite nice. Can we have
>> a #define for this, please? It doesn't need to live in a header file if it's
>> not going to be used anywhere else.
> 
> Sure, would it be okay to define them inside this init_csr_masks() or at the 
> top
> of the file would be better?

Either would work imo, as long as you don't expect other uses of these
constants. Personally I'd likely put them not at the top of the file, but
immediately ahead of the function.

Jan