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.
>>
>>
>> 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.
Cheers,
Luca
