Hi 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,
^— aarch32 :)
> 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 */
}
}
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!");
#endif
}
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);
#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);
#endif
default:
BUG(); /* Can't happen */
}
}
Please let me know your thoughts.
Cheers,
Luca
