On 13/01/2026 12:13, Luca Fancellu wrote:
> 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”?
Yet in a if() block for MPU_ATTR_AI_MISMATCH == attr_match you already have the
same message.
~Michal
>
>>
>>> + 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
>
