Re: [PATCH v1 03/17] xen/riscv: add missing APLIC register offsets, masks to asm/aplic.h

On 8/10/26 3:45 PM, Baptiste Le Duc wrote:
>> These definitions are required for correct decoding of APLIC MMIO
>> accesses and target configuration, and will be used by both the
>> physical and virtual APLIC implementations.
>>
>> No functional change is intended by this patch; it only centralises
>> hardware definitions that were previously missing.
>>
>> Co-developed-by: Romain Caritey <[email protected]>
>> Signed-off-by: Oleksii Kurochko <[email protected]>
>>
>> diff --git a/xen/arch/riscv/include/asm/aplic.h b/xen/arch/riscv/include/asm/aplic.h
>> index 07318aaac2..f22622b9a2 100644
>> --- a/xen/arch/riscv/include/asm/aplic.h
>> +++ b/xen/arch/riscv/include/asm/aplic.h
>> @@ -15,6 +15,8 @@
>>   
>>   #include <asm/imsic.h>
>>   
>> +#define APLIC_REG_OFFSET_MASK   0x3fff
> 
> _REG stands for memory-mapped control "region" as explained in spec? If
> yes, it'd be better to add a comment.
> 

Yes, it is a mask that allows us to get the offsets for the registers of 
an interrupt domain’s memory-mapped control region.

IMO, if the problem is with the name of the macro, it would be better to 
use a clearer name instead of adding a comment. For example, 
`APLIC_CTRL_REGION_OFFSET_MASK` sounds self-explanatory to me.

If you’re still not happy with the suggested name and it isn't 
self-explanatory, then:

/*
  * Offsets of the registers of an interrupt domain's memory-mapped control
  * region, which is APLIC_MIN_SIZE bytes large.
  */
#define APLIC_CTRL_REGION_OFFSET_MASK   0x3fff

Alternatively, I could keep the old name (APLIC_REG_OFFSET_MASK) and use 
the suggested comment.

Which option do you prefer?

~ Oleksii