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 8 Apr 2025, at 18:02, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> 
> Hi,
> 
> On 08/04/2025 17:48, Luca Fancellu wrote:
>> 
>>> On 8 Apr 2025, at 17:33, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
>>> 
>>> 
>>> On 08/04/2025 15:29, Luca Fancellu wrote:
>>>> Hi Ayan,
>>> Hi Luca,
>>>>> On 8 Apr 2025, at 15:02, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
>>>>> 
>>>>> Hi Luca,
>>>>> 
>>>>>> +static void prepare_selector(uint8_t 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.
>>>>>> +     */
>>>>>> +    sel &= 0xF0U;
>>>>>> +    if ( READ_SYSREG(PRSELR_EL2) != sel )
>>>>>> +    {
>>>>>> +        WRITE_SYSREG(sel, PRSELR_EL2);
>>>>>> +        isb();
>>>>>> +    }
>>>>> This needs to be arm64 specific. Refer ARM DDI 0600A.d ID120821
>>>>> 
>>>>> G1.3.19  PRBAR<n>_EL2, /* I guess this is what you are following */
>>>>> 
>>>>> Provides access to the base address for the MPU region determined by the 
>>>>> value of 'n' and
>>>>> PRSELR_EL2.REGION as PRSELR_EL2.REGION<7:4>:n.
>>>>> 
>>>>> 
>>>>> Whereas for arm32 . Refer ARM DDI 0568A.c ID110520
>>>>> 
>>>>> E2.2.3 HPRBAR<n>,
>>>>> 
>>>>> Provides access to the base addresses for the first 32 defined EL2 MPU 
>>>>> regions.
>>>>> 
>>>>> /* Here we do not need to use HPRSELR for region selection */
>>>>> 
>>>>> 
>>>>> If you want to make the code similar between arm32 and arm64, then I can 
>>>>> suggest you can use these registers.
>>>>> 
>>>>> G1.3.17 PRBAR_EL2
>>>>> 
>>>>> Provides access to the base addresses for the EL2 MPU region. 
>>>>> PRSELR_EL2.REGION determines
>>>>> which MPU region is selected.
>>>>> 
>>>>> E2.2.2 HPRBAR
>>>>> 
>>>>> Provides indirect access to the base address of the EL2 MPU region 
>>>>> currently defined by
>>>>> HPRSELR.
>>>>> 
>>>>> Let me know if it makes sense.
>>>>> 
>>>>> - Ayan
>>>> Ok I didin’t get that before, what do you think if I modify the code as in 
>>>> this diff (not tested):
>>>> 
>>>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>>>> index fe05c8097155..1bc6d7a77296 100644
>>>> --- a/xen/arch/arm/mpu/mm.c
>>>> +++ b/xen/arch/arm/mpu/mm.c
>>>> @@ -58,19 +58,21 @@ static void __init __maybe_unused 
>>>> build_assertions(void)
>>>>      BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
>>>>  }
>>>>  -static void prepare_selector(uint8_t sel)
>>>> +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.
>>>>       */
>>>> -    sel &= 0xF0U;
>>>> -    if ( READ_SYSREG(PRSELR_EL2) != sel )
>>>> +    cur_sel &= 0xF0U;
>>>> +    if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
>>>>      {
>>>> -        WRITE_SYSREG(sel, PRSELR_EL2);
>>>> +        WRITE_SYSREG(cur_sel, PRSELR_EL2);
>>>>          isb();
>>>>      }
>>>> +    *sel = *sel & 0xFU;
>>>>  }
>>>>    /*
>>>> @@ -102,7 +104,7 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
>>>>       */
>>>>      prepare_selector(sel);
>>>>  -    switch ( sel & 0xFU )
>>>> +    switch ( sel )
>>>>      {
>>>>          GENERATE_READ_PR_REG_CASE(0, pr_read);
>>>>          GENERATE_READ_PR_REG_CASE(1, pr_read);
>>>> @@ -140,7 +142,7 @@ void write_protection_region(const pr_t *pr_write, 
>>>> uint8_t sel)
>>>>       */
>>>>      prepare_selector(sel);
>>>>  -    switch ( sel & 0xFU )
>>>> +    switch ( sel )
>>>>      {
>>>>          GENERATE_WRITE_PR_REG_CASE(0, pr_write);
>>>>          GENERATE_WRITE_PR_REG_CASE(1, pr_write);
>>>> 
>>>> And later you will use some #ifdef CONFIG_ARM_32 inside prepare_selector() 
>>>> to check
>>>> that the code is passing up to the max supported region or panic.
>>>> And in {read,write}_protection_region() you could add some #ifdef 
>>>> CONFIG_ARM_32 to add
>>>> the case generators for regions from 16 to 23 since R52 can address them 
>>>> directly.
>>>> 
>>>> What do you think?
>>> I got this diff working as it is for R82. This looks much lesser code
>>> 
>>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>>> index fe05c80971..63627c85dc 100644
>>> --- a/xen/arch/arm/mpu/mm.c
>>> +++ b/xen/arch/arm/mpu/mm.c
>>> @@ -28,23 +28,19 @@ DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGIONS);
>>>  /* EL2 Xen MPU memory region mapping table. */
>>>  pr_t xen_mpumap[MAX_MPU_REGIONS];
>>> 
>>> -/* The following are needed for the case generator with num==0 */
>>> -#define PRBAR0_EL2 PRBAR_EL2
>>> -#define PRLAR0_EL2 PRLAR_EL2
>>> -
>>>  #define GENERATE_WRITE_PR_REG_CASE(num, pr)                                
>>>  \
>>>      case num: \
>>> { \
>>> -        WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR##num##_EL2); 
>>>  \
>>> -        WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR##num##_EL2); 
>>>  \
>>> +        WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2);  \
>>> +        WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2);  \
>>> break; \
>>>      }
>>> 
>>>  #define GENERATE_READ_PR_REG_CASE(num, pr)                      \
>>>      case num:                                                   \
>>>      {                                                           \
>>> -        pr->prbar.bits = READ_SYSREG(PRBAR##num##_EL2);         \
>>> -        pr->prlar.bits = READ_SYSREG(PRLAR##num##_EL2);         \
>>> +        pr->prbar.bits = READ_SYSREG(PRBAR_EL2);         \
>>> +        pr->prlar.bits = READ_SYSREG(PRLAR_EL2);         \
>>>          break;                                                  \
>>>      }
>>> 
>>> @@ -65,7 +61,6 @@ static void prepare_selector(uint8_t sel)
>>>       * 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.
>>>       */
>>> -    sel &= 0xF0U;
>>>      if ( READ_SYSREG(PRSELR_EL2) != sel )
>>>      {
>>>          WRITE_SYSREG(sel, PRSELR_EL2);
>>> 
>>> Please give it a try to see if it works at your end.
>>> 
>>> 
>>> And then, the code can be reduced further as
>>> 
>>> 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);
>>> 
>>>     pr_read->prbar.bits = READ_SYSREG(PRBAR_EL2);
>>> 
>>>     pr_read->prlar.bits = READ_SYSREG(PRLAR_EL2);
>>> 
>>> }
>>> 
>>> The same can be done for write_protection_region(...)
>>> 
>>> - Ayan
>> 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,
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.

> 
> If you are going to keep the code as it is, then the following needs to be 
> moved to arm64 specific header as well
> 
> #define PRBAR0_EL2 PRBAR_EL2
> #define PRLAR0_EL2 PRLAR_EL2

ok I’ll move them.

Cheers,
Luca