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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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