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
