> On 8 Apr 2025, at 17:33, Ayan Kumar Halder <ayank...@amd.com> wrote:
>
>
> On 08/04/2025 15:29, Luca Fancellu wrote:
>> Hi Ayan,
> Hi Luca,
>>
>>> On 8 Apr 2025, at 15:02, Ayan Kumar Halder <ayank...@amd.com> 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?
>
>>
>> Cheers,
>> Luca
>>
>>
>>
>>
