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 <[email protected]> wrote:
>>
>>
>>
>> On 28/11/2025 10:58, Harry Ramsey wrote:
>>> From: Luca Fancellu <[email protected]>
>>>
>>> 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
>
