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
