Re: [PATCH v3 6/6] arm/mpu: Enable read/write to protection regions for arm32

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

On 12/06/2025 12:37, Ayan Kumar Halder wrote:
> 
> On 12/06/2025 10:35, Luca Fancellu wrote:
>> Hi Ayan,
> Hi Luca,
>>
>>> On 11 Jun 2025, at 15:35, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> 
>>> wrote:
>>>
>>> Define prepare_selector(), read_protection_region() and
>>> write_protection_region() for arm32. Also, define
>>> GENERATE_{READ/WRITE}_PR_REG_OTHERS to access MPU regions from 32 to 255.
>>>
>>> Enable pr_{get/set}_{base/limit}(), region_is_valid() for arm32.
>>> Enable pr_of_addr() for arm32.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>>> ---
>> Based on your v2 
>> (https://patchwork.kernel.org/project/xen-devel/patch/20250606164854.1551148-4-ayan.kumar.halder@xxxxxxx/)
>>  I was imaging something like this:
>>
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 74e96ca57137..5d324b2d4ca5 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -87,20 +87,28 @@ static void __init __maybe_unused build_assertions(void)
>>    */
>>   static void prepare_selector(uint8_t *sel)
>>   {
>> -#ifdef CONFIG_ARM_64
>>       uint8_t cur_sel = *sel;
>>   
>> +#ifdef CONFIG_ARM_64
>>       /*
>> -     * {read,write}_protection_region works using the direct access to the 
>> 0..15
>> -     * regions, so in order to save the isb() overhead, change the 
>> PRSELR_EL2
>> -     * only when needed, so when the upper 4 bits of the selector will 
>> change.
>> +     * {read,write}_protection_region works using the Arm64 direct access 
>> to the
>> +     * 0..15 regions, so in order to save the isb() overhead, change the
>> +     * PRSELR_EL2 only when needed, so when the upper 4 bits of the selector
>> +     * will change.
>>        */
>>       cur_sel &= 0xF0U;
>> +#else
>> +    /* Arm32 MPU can use direct access for 0-31 */
>> +    if ( cur_sel > 31 )
>> +        cur_sel = 0;
>> +#endif
>>       if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
>>       {
>>           WRITE_SYSREG(cur_sel, PRSELR_EL2);
>>           isb();
>>       }
>> +
>> +#ifdef CONFIG_ARM_64
>>       *sel = *sel & 0xFU;
>>   #endif
>>   }
>> @@ -144,6 +152,12 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
>>           GENERATE_READ_PR_REG_CASE(29, pr_read);
>>           GENERATE_READ_PR_REG_CASE(30, pr_read);
>>           GENERATE_READ_PR_REG_CASE(31, pr_read);
>> +        case 32 ... 255:
>> +        {
>> +            pr->prbar.bits = READ_SYSREG(PRBAR_EL2);
>> +            pr->prlar.bits = READ_SYSREG(PRLAR_EL2);
>> +            break;
>> +        }
>>   #endif
>>       default:
>>           BUG(); /* Can't happen */
>> @@ -190,6 +204,12 @@ void write_protection_region(const pr_t *pr_write, 
>> uint8_t sel)
>>           GENERATE_WRITE_PR_REG_CASE(29, pr_write);
>>           GENERATE_WRITE_PR_REG_CASE(30, pr_write);
>>           GENERATE_WRITE_PR_REG_CASE(31, pr_write);
>> +        case 32 ... 255:
>> +        {
>> +            WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2);
>> +            WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2);
>> +            break;
>> +        }
>>   #endif
>>       default:
>>           BUG(); /* Can't happen */
>>
>>
>> Is it using too ifdefs in your opinion that would benefit the split you do 
>> in v3?
> 
> Yes. However, I understand that this is subjective. I need your and 
> Michal/Julien to have an opinion here whether to go with the split 
> (which means some amount of code duplication) or introduce if-defs. I 
> will be happy to proceed as per your opinions.
I don't have a strong opinion here. Maybe I slightly prefer the split to avoid
ifdefery.

~Michal