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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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