Re: [PATCH v2 3/6] arm/mpu: Implement free_init_memory for MPU systems

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Michal,

>> 
>> -static bool is_mm_attr_match(pr_t *region, unsigned int attributes)
>> +static int is_mm_attr_match(pr_t *region, unsigned int attributes)
>> {
>>     if ( region->prbar.reg.ro != PAGE_RO_MASK(attributes) )
>> -    {
>> -        printk(XENLOG_WARNING
>> -               "Mismatched Access Permission attributes (%#x instead of 
>> %#x)\n",
>> -               region->prbar.reg.ro, PAGE_RO_MASK(attributes));
>> -        return false;
>> -    }
>> +        return MPU_ATTR_RO_MISMATCH;
>> 
>>     if ( region->prbar.reg.xn != PAGE_XN_MASK(attributes) )
>> -    {
>> -        printk(XENLOG_WARNING
>> -               "Mismatched Execute Never attributes (%#x instead of %#x)\n",
>> -               region->prbar.reg.xn, PAGE_XN_MASK(attributes));
>> -        return false;
>> -    }
>> +        return MPU_ATTR_XN_MISMATCH;
> Later below you don't seem to differentiate between MPU_ATTR_RO_MISMATCH and
> MPU_ATTR_XN_MISMATCH. Therefore I would suggest to keep them as one to 
> simplify
> the code.
> 
>> 
>>     if ( region->prlar.reg.ai != PAGE_AI_MASK(attributes) )
>> -    {
>> -        printk(XENLOG_WARNING
>> -               "Mismatched Memory Attribute Index (%#x instead of %#x)\n",
>> -               region->prlar.reg.ai, PAGE_AI_MASK(attributes));
>> -        return false;
>> -    }
>> +        return MPU_ATTR_AI_MISMATCH;
>> 
>> -    return true;
>> +    return 0;
>> }
>> 
>> /* Map a frame table to cover physical addresses ps through pe */
>> @@ -357,12 +342,45 @@ static int xen_mpumap_update_entry(paddr_t base, 
>> paddr_t limit,
>>     */
>>     if ( flags_has_page_present && (rc >= MPUMAP_REGION_FOUND) )
>>     {
>> -        if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
>> +        int attr_match = is_mm_attr_match(&xen_mpumap[idx], flags);
>> +
>> +        /* We do not support modifying AI attribute. */
>> +        if ( MPU_ATTR_AI_MISMATCH == attr_match )
>>         {
>> -            printk("Modifying an existing entry is not supported\n");
>> +            printk(XENLOG_ERR
>> +                   "Modifying memory attribute is not supported\n");
>>             return -EINVAL;
>>         }
>> 
>> +        /*
>> +         * Permissions RO and XN can be changed only by the full region.
>> +         * Permissions that match can continue and just increment refcount.
>> +         */
>> +        if ( MPU_ATTR_RO_MISMATCH == attr_match ||
> Enlcose in brackets () || ()
> 
>> +             MPU_ATTR_XN_MISMATCH == attr_match )
>> +        {
>> +            if ( rc == MPUMAP_REGION_INCLUSIVE )
>> +            {
>> +                printk(XENLOG_ERR
>> +                       "Cannot modify partial region permissions\n");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if ( xen_mpumap[idx].refcount != 0 )
>> +            {
>> +                printk(XENLOG_ERR
>> +                       "Cannot modify memory permissions for a region 
>> mapped multiple times\n");
> Memory permission? Here you are checking for XN/RO.

Right, is it better “memory attributes”?

> 
>> +                return -EINVAL;
>> +            }
>> +
>> +            /* Set new permission */
>> +            xen_mpumap[idx].prbar.reg.ro = PAGE_RO_MASK(flags);
>> +            xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
> Here you always change both, that's why there is no need to provide two 
> separate
> macros as I mentioned above.

good point.

Cheers,
Luca