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