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
