Re: [PATCH v3 4/6] arm/mpu: Move the functions to arm64 specific files

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 13/06/2025 16:08, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
> > On 11 Jun 2025, at 15:35, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> >
> > prepare_selector(), read_protection_region() and write_protection_region()
> > differ significantly between arm32 and arm64. Thus, move these functions
> > to their specific folders.
>     ^— NIT: “to sub-arch specific folder”? What do you think?
yes
> > GENERATE_{WRITE/READ}_PR_REG_CASE are duplicated for arm32 and arm64 so
> > as to improve the code readability.
> It reads a bit hard in this way, what about:
>
> “Also the macro GENERATE_{WRITE/READ}_PR_REG_CASE are moved, in order to
> keep them in the same file of their usage and improve readability"
yes, this reads better.
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > Changes from -
> >
> > v1..v2 - New patch introduced in v3.
> >
> > xen/arch/arm/mpu/Makefile       |   1 +
> > xen/arch/arm/mpu/arm64/Makefile |   1 +
> > xen/arch/arm/mpu/arm64/mm.c     | 130 ++++++++++++++++++++++++++++++++
> > xen/arch/arm/mpu/mm.c           | 117 ----------------------------
> > 4 files changed, 132 insertions(+), 117 deletions(-)
> > create mode 100644 xen/arch/arm/mpu/arm64/Makefile
> > create mode 100644 xen/arch/arm/mpu/arm64/mm.c
> >
> > diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
> > index 9359d79332..4963c8b550 100644
> > --- a/xen/arch/arm/mpu/Makefile
> > +++ b/xen/arch/arm/mpu/Makefile
> > @@ -1,4 +1,5 @@
> > obj-$(CONFIG_ARM_32) += arm32/
> > +obj-$(CONFIG_ARM_64) += arm64/
> > obj-y += mm.o
> > obj-y += p2m.o
> > obj-y += setup.init.o
> > diff --git a/xen/arch/arm/mpu/arm64/Makefile b/xen/arch/arm/mpu/arm64/Makefile
> > new file mode 100644
> > index 0000000000..b18cec4836
> > --- /dev/null
> > +++ b/xen/arch/arm/mpu/arm64/Makefile
> > @@ -0,0 +1 @@
> > +obj-y += mm.o
> > diff --git a/xen/arch/arm/mpu/arm64/mm.c b/xen/arch/arm/mpu/arm64/mm.c
> > new file mode 100644
> > index 0000000000..a978c1fc6e
> > --- /dev/null
> > +++ b/xen/arch/arm/mpu/arm64/mm.c
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/bug.h>
> > +#include <xen/types.h>
> > +#include <asm/mpu.h>
> > +#include <asm/sysregs.h>
> > +#include <asm/system.h>
> > +
> > +/*
> > + * The following are needed for the cases: GENERATE_WRITE_PR_REG_CASE
> > + * and GENERATE_READ_PR_REG_CASE with num==0
> > + */
> > +#define PRBAR0_EL2 PRBAR_EL2
> > +#define PRLAR0_EL2 PRLAR_EL2
> > +
> > +#define PRBAR_EL2_(n)   PRBAR##n##_EL2
> > +#define PRLAR_EL2_(n)   PRLAR##n##_EL2
> > +
> > +#define GENERATE_WRITE_PR_REG_CASE(num, pr)                                 \
> > +    case num:                                                               \
> > +    {                                                                       \
> > +        WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2_(num));   \
> > +        WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2_(num));   \
> > +        break;                                                              \
> > +    }
> > +
> > +#define GENERATE_READ_PR_REG_CASE(num, pr)                      \
> > +    case num:                                                   \
> > +    {                                                           \
> > +        pr->prbar.bits = READ_SYSREG(PRBAR_EL2_(num));          \
> > +        pr->prlar.bits = READ_SYSREG(PRLAR_EL2_(num));          \
> > +        break;                                                  \
> > +    }
> > +
> > +/*
> > + * Armv8-R supports direct access and indirect access to the MPU regions 
> > through
> > + * registers:
> > + *  - indirect access involves changing the MPU region selector, issuing an isb
> > + *    barrier and accessing the selected region through specific registers
> > + *  - direct access involves accessing specific registers that point to
> > + *    specific MPU regions, without changing the selector, avoiding the use of
> > + *    a barrier.
> > + * For Arm64 the PR{B,L}AR_ELx (for n=0) and PR{B,L}AR<n>_ELx (for n=1..15) are
> > + * used for the direct access to the regions selected by
> > + * PRSELR_EL2.REGION<7:4>:n, so 16 regions can be directly accessed when the
> > + * selector is a multiple of 16, giving access to all the supported memory
> > + * regions.
> > + */
> > +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;
> This one is different in the original file (*sel &= 0xFU;)
Agree with all of the suggested changes.
> The rest looks good to me!
> With the above fixed:
>
> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
- Ayan