Re: [PATCH v3 6/6] arm/mpu: Enable read/write to protection regions for arm32

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 12/06/2025 10:35, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
> > On 11 Jun 2025, at 15:35, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> >
> > Define prepare_selector(), read_protection_region() and
> > write_protection_region() for arm32. Also, define
> > GENERATE_{READ/WRITE}_PR_REG_OTHERS to access MPU regions from 32 to 255.
> >
> > Enable pr_{get/set}_{base/limit}(), region_is_valid() for arm32.
> > Enable pr_of_addr() for arm32.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> Based on your v2 
> (https://patchwork.kernel.org/project/xen-devel/patch/20250606164854.1551148-4-ayan.kumar.halder@xxxxxxx/)
>  I was imaging something like this:
>
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 74e96ca57137..5d324b2d4ca5 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -87,20 +87,28 @@ static void __init __maybe_unused build_assertions(void)
>    */
>   static void prepare_selector(uint8_t *sel)
>   {
> -#ifdef CONFIG_ARM_64
>       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.
> +     * {read,write}_protection_region works using the Arm64 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;
> +#else
> +    /* Arm32 MPU can use direct access for 0-31 */
> +    if ( cur_sel > 31 )
> +        cur_sel = 0;
> +#endif
>       if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
>       {
>           WRITE_SYSREG(cur_sel, PRSELR_EL2);
>           isb();
>       }
> +
> +#ifdef CONFIG_ARM_64
>       *sel = *sel & 0xFU;
>   #endif
>   }
> @@ -144,6 +152,12 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
>           GENERATE_READ_PR_REG_CASE(29, pr_read);
>           GENERATE_READ_PR_REG_CASE(30, pr_read);
>           GENERATE_READ_PR_REG_CASE(31, pr_read);
> +        case 32 ... 255:
> +        {
> +            pr->prbar.bits = READ_SYSREG(PRBAR_EL2);
> +            pr->prlar.bits = READ_SYSREG(PRLAR_EL2);
> +            break;
> +        }
>   #endif
>       default:
>           BUG(); /* Can't happen */
> @@ -190,6 +204,12 @@ void write_protection_region(const pr_t *pr_write, uint8_t 
> sel)
>           GENERATE_WRITE_PR_REG_CASE(29, pr_write);
>           GENERATE_WRITE_PR_REG_CASE(30, pr_write);
>           GENERATE_WRITE_PR_REG_CASE(31, pr_write);
> +        case 32 ... 255:
> +        {
> +            WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2);
> +            WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2);
> +            break;
> +        }
>   #endif
>       default:
>           BUG(); /* Can't happen */
>
>
> Is it using too ifdefs in your opinion that would benefit the split you do in 
> v3?
Yes. However, I understand that this is subjective. I need your and 
Michal/Julien to have an opinion here whether to go with the split 
(which means some amount of code duplication) or introduce if-defs. I 
will be happy to proceed as per your opinions.
- Ayan

>