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

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

On 14/01/2026 12:07, Luca Fancellu wrote:
> Hi Michal,
> 
>>>
>>> +/*
>>> + * 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     MPUMAP_REGION_* value.
>>> + * @return                      0 on success, otherwise negative on error.
>>> + */
>>> +static int xen_mpumap_free_entry(uint8_t idx, int region_found_type)
>>> +{
>>> +    ASSERT(spin_is_locked(&xen_mpumap_lock));
>>> +    ASSERT(idx != INVALID_REGION_IDX);
>>> +    ASSERT(MPUMAP_REGION_OVERLAP != region_found_type);
>>> +
>>> +    if ( MPUMAP_REGION_NOTFOUND == region_found_type )
>>> +    {
>>> +        printk(XENLOG_ERR "Cannot remove entry that does not exist\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if ( xen_mpumap[idx].refcount )
>>> +    {
>>> +        xen_mpumap[idx].refcount -= 1;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if ( MPUMAP_REGION_FOUND == region_found_type )
>>> +        disable_mpu_region_from_index(idx);
>>> +    else
>>> +    {
>>> +        printk(XENLOG_ERR "Cannot remove a partial region\n");
>>> +        return -EINVAL;
>>> +    }
>> NIT: Try to limit the number of if/else blocks to make the code read better.
>> Here you could do the following to remove one else:
>>    if ( MPUMAP_REGION_FOUND != region_found_type )
>>    {
>>        printk(XENLOG_ERR "Cannot remove a partial region\n");
>>        return -EINVAL;
>>    }
>>
>>    disable_mpu_region_from_index(idx);
>>
>>    return 0;
> 
> Makes sense, while we are there, shall we have also a comment above that 
> check, something like this:
> 
> /*
>  * If we reached this point, the region is due to be destroyed, as a safety
>  * check we need to ensure the API is called with the exact region, to prevent
>  * destroying a region using a partial memory range.
>  */
> 
> What do you think? Otherwise someone in the future might think it’s ok to 
> move this check
> together with the other on top.
I'm not sure. I can read this from code but if you think it will be beneficial,
I have no objections.

> 
>>>
>>>
>>> void vunmap(const void *va)
>>> {
>>> -    BUG_ON("unimplemented");
>>> +    paddr_t base = virt_to_maddr(va);
>>> +
>>> +    if ( destroy_xen_mapping_containing(base) )
>>> +        panic("Failed to vunmap region\n");
>>> }
>> Looking at this series as a whole, at the end we will end up with
>> vmap_contig/vunmap to contain the same implementation as map_domain_page and
>> alike from the last patch. Shouldn't we define ones using the others?
> 
> We can do it, the only thing is that if vunmap fails, we will get a less 
> specific error message,
> if you are ok with that we will do the change.
In the MMU case you won't get any info, so I think it's ok.

~Michal