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.hal...@amd.com> 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.hal...@amd.com> --- 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.fance...@arm.com>
- Ayan
