Re: [PATCH v2 2/7] arm/mpu: Provide access to the MPU region from the C code

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Ayan,

> On 9 Apr 2025, at 11:07, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> 
> 
> On 09/04/2025 09:26, Luca Fancellu wrote:
>> Hi Ayan,
> Hi Luca,
>> 
>>>>> The point of the code was to don’t issue an isb() every time we change 
>>>>> the selector,
>>>>> of course the code would be easier otherwise, but do we want to do that?
>>>> Not sure if it is beneficial as you would need to use isb() from region16 
>>>> onwards.
>>> The isb() is issued only when changing the selector, so when you go from 
>>> reading/writing regions
>>> from 0-15 to 16-31 for example, of course there is a case that if you 
>>> continuously write on region 15
>>> and 16 you would have to always change the selector, but it’s the less 
>>> impact we could have.
>>> 
>>> armv8-R is even better since it’s able to address regions from 0 to 23 
>>> without flushing the pipeline,
>>                  ^— aarch32 :)
> Can you point me to the document where you got this info ?

was referring to the r52, of course you are more knowledgeable on the armv8-r 
aarch32 part, so ...

>> 
>>> so I would say we should exploit this big advantage.
>>> 
>>> I’ll send shortly in this thread the code I would use and the code I was 
>>> thinking you could use.
>> Here the code I have in mind:
>> 
>> static void prepare_selector(uint8_t *sel)
>> {
>>     uint8_t cur_sel = *sel;
>>     /*
>>      * {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.
>>      */
>>     cur_sel &= 0xF0U;
>>     if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
>>     {
>>         WRITE_SYSREG(cur_sel, PRSELR_EL2);
>>         isb();
>>     }
>>     *sel = *sel & 0xFU;
>> }
>> 
>> void read_protection_region(pr_t *pr_read, uint8_t sel)
>> {
>>     /*
>>      * Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
>>      * make sure PRSELR_EL2 is set, as it determines which MPU region
>>      * is selected.
>>      */
>>     prepare_selector(&sel);
>> 
>>     switch ( sel )
>>     {
>>         GENERATE_READ_PR_REG_CASE(0, pr_read);
>>         GENERATE_READ_PR_REG_CASE(1, pr_read);
>>         GENERATE_READ_PR_REG_CASE(2, pr_read);
>>         GENERATE_READ_PR_REG_CASE(3, pr_read);
>>         GENERATE_READ_PR_REG_CASE(4, pr_read);
>>         GENERATE_READ_PR_REG_CASE(5, pr_read);
>>         GENERATE_READ_PR_REG_CASE(6, pr_read);
>>         GENERATE_READ_PR_REG_CASE(7, pr_read);
>>         GENERATE_READ_PR_REG_CASE(8, pr_read);
>>         GENERATE_READ_PR_REG_CASE(9, pr_read);
>>         GENERATE_READ_PR_REG_CASE(10, pr_read);
>>         GENERATE_READ_PR_REG_CASE(11, pr_read);
>>         GENERATE_READ_PR_REG_CASE(12, pr_read);
>>         GENERATE_READ_PR_REG_CASE(13, pr_read);
>>         GENERATE_READ_PR_REG_CASE(14, pr_read);
>>         GENERATE_READ_PR_REG_CASE(15, pr_read);
>>     default:
>>         BUG(); /* Can't happen */
>>     }
>> }
>> 
>> void write_protection_region(const pr_t *pr_write, uint8_t sel)
>> {
>>     /*
>>      * Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
>>      * make sure PRSELR_EL2 is set, as it determines which MPU region
>>      * is selected.
>>      */
>>     prepare_selector(&sel);
>> 
>>     switch ( sel )
>>     {
>>         GENERATE_WRITE_PR_REG_CASE(0, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(1, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(2, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(3, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(4, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(5, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(6, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(7, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(8, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(9, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(10, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(11, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(12, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(13, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(14, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(15, pr_write);
>>     default:
>>         BUG(); /* Can't happen */
>>     }
>> }
> Till here I am fine if you think this is the correct approach for arm64.

Ok

>> 
>> Here the code I thought you could add for arm32:
>> 
>> static void prepare_selector(uint8_t *sel)
>> {
>>     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.
>>      */
>>     cur_sel &= 0xF0U;
>>     if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
>>     {
>>         WRITE_SYSREG(cur_sel, PRSELR_EL2);
>>         isb();
>>     }
>>     *sel = *sel & 0xFU;
>> #else
>>     if ( cur_sel > 23 )
>>         panic("Armv8-R AArch32 region selector exceeds maximum allowed 
>> range!");
> I am not sure about this. See my question before. However ...
>> #endif
>> }
> 
> From ARM DDI 0568A.c ID110520
> 
> E2.2.3 HPRBAR<n> - Provides access to the base addresses for the first 32 
> defined EL2 MPU regions.
> 
> E2.2.6 HPRLAR<n> - Provides access to the limit addresses for the first 32 
> defined EL2 MPU regions.
> 
> I understand that HPRSELR is not used when directly accessing the above two 
> registers. Thus, this function will be a nop for Arm32. Please let me know if 
> I am mistaken.

yes you are right, you can decide if doing something or not in that function, 
you don’t need to change the selector.


> 
>> 
>> void read_protection_region(pr_t *pr_read, uint8_t sel)
>> {
>>     /*
>>      * Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
>>      * make sure PRSELR_EL2 is set, as it determines which MPU region
>>      * is selected.
>>      */
>>     prepare_selector(&sel);
>> 
>>     switch ( sel )
>>     {
>>         GENERATE_READ_PR_REG_CASE(0, pr_read);
>>         GENERATE_READ_PR_REG_CASE(1, pr_read);
>>         GENERATE_READ_PR_REG_CASE(2, pr_read);
>>         GENERATE_READ_PR_REG_CASE(3, pr_read);
>>         GENERATE_READ_PR_REG_CASE(4, pr_read);
>>         GENERATE_READ_PR_REG_CASE(5, pr_read);
>>         GENERATE_READ_PR_REG_CASE(6, pr_read);
>>         GENERATE_READ_PR_REG_CASE(7, pr_read);
>>         GENERATE_READ_PR_REG_CASE(8, pr_read);
>>         GENERATE_READ_PR_REG_CASE(9, pr_read);
>>         GENERATE_READ_PR_REG_CASE(10, pr_read);
>>         GENERATE_READ_PR_REG_CASE(11, pr_read);
>>         GENERATE_READ_PR_REG_CASE(12, pr_read);
>>         GENERATE_READ_PR_REG_CASE(13, pr_read);
>>         GENERATE_READ_PR_REG_CASE(14, pr_read);
>>         GENERATE_READ_PR_REG_CASE(15, pr_read);
>> #ifdef CONFIG_ARM_32
>>         GENERATE_READ_PR_REG_CASE(16, pr_read);
>>         GENERATE_READ_PR_REG_CASE(17, pr_read);
>>         GENERATE_READ_PR_REG_CASE(18, pr_read);
>>         GENERATE_READ_PR_REG_CASE(19, pr_read);
>>         GENERATE_READ_PR_REG_CASE(20, pr_read);
>>         GENERATE_READ_PR_REG_CASE(21, pr_read);
>>         GENERATE_READ_PR_REG_CASE(22, pr_read);
>>         GENERATE_READ_PR_REG_CASE(23, pr_read);
> At least 32 regions are directly accessible, thus thisn should go till 31 
> (0-31). Same ..

yeah you can add until the 31st here and ...

>> #endif
>>     default:
>>         BUG(); /* Can't happen */
>>     }
>> }
>> 
>> void write_protection_region(const pr_t *pr_write, uint8_t sel)
>> {
>>     /*
>>      * Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
>>      * make sure PRSELR_EL2 is set, as it determines which MPU region
>>      * is selected.
>>      */
>>     prepare_selector(&sel);
>> 
>>     switch ( sel )
>>     {
>>         GENERATE_WRITE_PR_REG_CASE(0, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(1, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(2, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(3, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(4, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(5, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(6, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(7, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(8, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(9, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(10, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(11, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(12, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(13, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(14, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(15, pr_write);
>> #ifdef CONFIG_ARM_32
>>         GENERATE_WRITE_PR_REG_CASE(16, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(17, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(18, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(19, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(20, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(21, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(22, pr_write);
>>         GENERATE_WRITE_PR_REG_CASE(23, pr_write);
> for here.

also here.

So I guess we are aligned for this patch right? I will update this patch as the 
code above and you
will modify the code for arm32 to support the direct access up to 32 region. Ok?

Cheers,
Luca