> On 16 Apr 2025, at 14:10, Orzel, Michal <michal.or...@amd.com> wrote:
>
>
>
> 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.
Ok, I'll document on top of the helper that @limit needs to be exclusive just
to be sure it will be used
in this way.
