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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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
>