Re: [PATCH 2/6] arm/mpu: Implement vmap functions for MPU

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

On 08/12/2025 11:20, Luca Fancellu wrote:
> Hi Michal,
> 
> thanks for your review, I’ll answer only few of your points and I’ll let 
> Harry take the rest.
> 
>> On 8 Dec 2025, at 09:53, Orzel, Michal <Michal.Orzel@xxxxxxx> wrote:
>>
>>
>>
>> On 28/11/2025 10:58, Harry Ramsey wrote:
>>> From: Luca Fancellu <luca.fancellu@xxxxxxx>
>>>
>>> HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
>> Just as a reminder, we don't intend to support VMAP on MPU? Asking because it
>> would otherwise be a duplicate effort to implement only these two helpers.
> 
> Yes we are not going to support VMAP on MPU, here we want only to provide the
> implementation of these two helper so that we don’t touch the common code 
> using these.
> Are you suggesting some rewording or was it only a curiosity on your side?
It was a question to check whether we are aligned.

> 
>>>
>>> +/*
>>> + * Free a xen_mpumap entry given the index. A mpu region is actually 
>>> disabled
>>> + * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
>>> + *
>>> + * @param idx                   Index of the mpumap entry.
>>> + * @param region_found_type     Either MPUMAP_REGION_FOUND or 
>>> MPUMAP_REGION_INCLUSIVE.
>> Both of these are unsigned, so why the parameter is int?
> 
> Uhm, good catch, I think this is a documentation issue because it might be 
> also MPUMAP_REGION_OVERLAP which is
> -1, we should not mandate which value might be here, we should only say 
> MPUMAP_REGION_* values.
> 
> 
>>>
>>>
>>> void vunmap(const void *va)
>>> {
>>> -    BUG_ON("unimplemented");
>>> +    paddr_t base = virt_to_maddr(va);
>>> +
>>> +    if ( destroy_entire_xen_mapping(base) )
>>> +        panic("Failed to vunmap region\n");
>> Looking at common vunmap() we ignore the return code from
>> destroy_xen_mappings(). Is it ok to diverge?
> 
> In our tests it’s ok, I asked Harry to have this so that we can catch any 
> vunmap that is not balanced
> with a prior mapping. To be fair I’m not really sure why in the vmap.c 
> implementation we are not
> reading the error codes from destroy_xen_mappings.
I'm ok with that. I don't know why we don't check the rc there. If you look at
x86 destroy_xen_mappings calls, none seem to be checked against rc. It can be
that it's impossible scenario there.

~Michal

> 
> Cheers,
> Luca
>