Re: [PATCH v3 3/7] arm/mpu: Introduce utility functions for the pr_t type

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 16/04/2025 14:57, Luca Fancellu wrote:
> Hi Michal,
> 
>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +/* Set limit address of MPU protection region(pr_t). */
>>>>> +static inline void pr_set_limit(pr_t *pr, paddr_t limit)
>>>>> +{
>>>>> +    pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT);
>>>> Why -1? AFAIR these registers take inclusive addresses, so is it because 
>>>> you
>>>> want caller to pass limit as exclusive and you convert it to inclusive? I 
>>>> think
>>>> it's quite error prone.
>>>
>>> Yes it’s meant to be used with exclusive range, shall we document it or use
>>> Inclusive range instead?
>> What's the expected behavior of callers? Are we going to set up protection
>> region based on regular start+size pair (like with MMU) or start+end? If the
>> latter for some reason (with size there are less issues), then end usually is
>> inclusive and you would not need conversion.
> 
> I think we have a mix because for example destroy_xen_mappings and 
> modify_xen_mappings
> are start and end, map_pages_to_xen instead is kind of start+size?
> 
> I moved the -1 inside pr_set_limit because it was open-coded in all the 
> places, for example when
> referencing linker symbols or output of mfn_to_maddr(mfn_add(smfn, nr_mfn)), 
> for this reason I
> thought: let’s call this one with exclusive ranges and modify internally for 
> inclusive.
Hmm, yes. Indeed we have a mix of places where end is inclusive or exclusive. I
think we can stick with exclusive address being passed to this helper unless
others have a different opinion. I know we tried to convert some places to
start+size but I don't remember the future plans.

~Michal