HI Julien,
> On 14 Apr 2025, at 12:41, Julien Grall <jul...@xen.org> wrote:
>
> Hi Luca,
>
> On 11/04/2025 23:56, Luca Fancellu wrote:
>> Implement some utility function in order to access the MPU regions
>> from the C world.
>> Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
>> ---
>> v3 changes:
>> - Moved PRBAR0_EL2/PRLAR0_EL2 to arm64 specific
>> - Modified prepare_selector() to be easily made a NOP
>> for Arm32, which can address up to 32 region without
>> changing selector and it is also its maximum amount
>> of MPU regions.
>> ---
>> ---
>> xen/arch/arm/include/asm/arm64/mpu.h | 7 ++
>> xen/arch/arm/include/asm/mpu.h | 1 +
>> xen/arch/arm/include/asm/mpu/mm.h | 24 +++++
>> xen/arch/arm/mpu/mm.c | 125 +++++++++++++++++++++++++++
>> 4 files changed, 157 insertions(+)
>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h
>> b/xen/arch/arm/include/asm/arm64/mpu.h
>> index 4d2bd7d7877f..b4e1ecdf741d 100644
>> --- a/xen/arch/arm/include/asm/arm64/mpu.h
>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
>> @@ -8,6 +8,13 @@
>> #ifndef __ASSEMBLY__
>> +/*
>> + * The following are needed for the case generators
>> GENERATE_WRITE_PR_REG_CASE
>> + * and GENERATE_READ_PR_REG_CASE with num==0
>> + */
>> +#define PRBAR0_EL2 PRBAR_EL2
>> +#define PRLAR0_EL2 PRLAR_EL2
>
> Rather than aliasing, shouldn't we just rename PR{B,L}AR_EL2 to
> PR{B,L}AR0_EL2? This would the code mixing between the two.
PR{B,L}AR0_ELx does not exists really, the PR{B,L}AR<n>_ELx exists for n=1..15,
here I’m only using this “alias” for the generator,
but PR{B,L}AR_EL2 are the real register.
>
>> +
>> /* Protection Region Base Address Register */
>> typedef union {
>> struct __packed {
>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>> index e148c705b82c..59ff22c804c1 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -13,6 +13,7 @@
>> #define MPU_REGION_SHIFT 6
>> #define MPU_REGION_ALIGN (_AC(1, UL) << MPU_REGION_SHIFT)
>> #define MPU_REGION_MASK (~(MPU_REGION_ALIGN - 1))
>> +#define MPU_REGION_RES0 (0xFFFULL << 52)
>> #define NUM_MPU_REGIONS_SHIFT 8
>> #define NUM_MPU_REGIONS (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
>> diff --git a/xen/arch/arm/include/asm/mpu/mm.h
>> b/xen/arch/arm/include/asm/mpu/mm.h
>> index 86f33d9836b7..5cabe9d111ce 100644
>> --- a/xen/arch/arm/include/asm/mpu/mm.h
>> +++ b/xen/arch/arm/include/asm/mpu/mm.h
>> @@ -8,6 +8,7 @@
>> #include <xen/page-size.h>
>> #include <xen/types.h>
>> #include <asm/mm.h>
>> +#include <asm/mpu.h>
>> extern struct page_info *frame_table;
>> @@ -29,6 +30,29 @@ static inline struct page_info *virt_to_page(const void
>> *v)
>> return mfn_to_page(mfn);
>> }
>> +/* Utility function to be used whenever MPU regions are modified */
>> +static inline void context_sync_mpu(void)
>> +{
>> + /*
>> + * ARM DDI 0600B.a, C1.7.1
>> + * Writes to MPU registers are only guaranteed to be visible following a
>> + * Context synchronization event and DSB operation.
>
> I know we discussed about this before. I find odd that the specification says
> "context synchronization event and DSB operation". At least to me, it implies
> "isb + dsb" not the other way around. Has this been clarified in newer
> version of the specification?
unfortunately no, I’m looking into the latest one (Arm® Architecture Reference
Manual Supplement Armv8, for R-profile AArch64 architecture 0600B.a) but it has
the same wording, however
I spoke internally with Cortex-R architects and they told me to use DSB+ISB
>
>> + */
>> + dsb(sy);
>> + isb();
>> +}
>> +
>> +/*
>> + * The following API require context_sync_mpu() after being used to modifiy
>> MPU
>
> typo: s/require/requires/ and s/modifiy/modify/
>
>> + * regions:
>> + * - write_protection_region
>> + */
>> +
>> +/* Reads the MPU region with index 'sel' from the HW */
>> +extern void read_protection_region(pr_t *pr_read, uint8_t sel);
>
> I am probably missing something. But don't you have a copy of pr_t in
> xen_mpumap? If so, can't we use the cached version to avoid accessing the
> system registers?
This API is meant to read/write registers, last patch uses it to populate
xen_mpumap, along the tree it is also used in dump_hyp_walk, probably given
your comment to the
last patch, if we need to update the xen_mpumap from the asm code, this could
change.
>
>> +/* Writes the MPU region with index 'sel' to the HW */
>> +extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
>> +
>> #endif /* __ARM_MPU_MM_H__ */
>> /*
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index f83ce04fef8a..e522ce53c357 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -8,12 +8,30 @@
>> #include <xen/sizes.h>
>> #include <xen/types.h>
>> #include <asm/mpu.h>
>> +#include <asm/mpu/mm.h>
>> +#include <asm/sysregs.h>
>> struct page_info *frame_table;
>> /* EL2 Xen MPU memory region mapping table. */
>> pr_t xen_mpumap[MAX_MPU_REGIONS];
>> +#define GENERATE_WRITE_PR_REG_CASE(num, pr)
>> \
>> + case num:
>> \
>> + {
>> \
>> + WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR##num##_EL2);
>> \
>> + WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR##num##_EL2);
>> \
>> + break;
>> \
>> + }
>> +
>> +#define GENERATE_READ_PR_REG_CASE(num, pr) \
>> + case num: \
>> + { \
>> + pr->prbar.bits = READ_SYSREG(PRBAR##num##_EL2); \
>> + pr->prlar.bits = READ_SYSREG(PRLAR##num##_EL2); \
>> + break; \
>> + }
>> +
>> static void __init __maybe_unused build_assertions(void)
>> {
>> /*
>> @@ -24,6 +42,113 @@ static void __init __maybe_unused build_assertions(void)
>> BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
>> }
>> +static void prepare_selector(uint8_t *sel)
>> +{
>> + uint8_t cur_sel = *sel;
>
> Coding style: Missing newline.
will fix
>
>> + /*
>> + * {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;
>> +}
>> +
>> +/*
>> + * Armv8-R AArch64 at most supports 255 MPU protection regions.
>> + * See section G1.3.18 of the reference manual for Armv8-R AArch64,
>> + * PRBAR<n>_EL2 and PRLAR<n>_EL2 provide access to the EL2 MPU region
>> + * determined by the value of 'n' and PRSELR_EL2.REGION as
>> + * PRSELR_EL2.REGION<7:4>:n(n = 0, 1, 2, ... , 15)
>> + * For example to access regions from 16 to 31 (0b10000 to 0b11111):
>> + * - Set PRSELR_EL2 to 0b1xxxx
>> + * - Region 16 configuration is accessible through PRBAR_EL2 and PRLAR_EL2
>> + * - Region 17 configuration is accessible through PRBAR1_EL2 and PRLAR1_EL2
>> + * - Region 18 configuration is accessible through PRBAR2_EL2 and PRLAR2_EL2
>> + * - ...
>> + * - Region 31 configuration is accessible through PRBAR15_EL2 and
>> PRLAR15_EL2
>> + */
>
> I am a bit confused. This function is implemented in the common MPU code.
> Yet, then comment only refer to 64-bit. Is the code the same on 32-bit? If
> not, then I think this function wants to be moved in arm64/mpu/
I’ll try to reword the comment removing the arm64-only parts
>
>> +/*
>> + * Read EL2 MPU Protection Region.
>> + *
>> + * @pr_read: mpu protection region returned by read op.
>> + * @sel: mpu protection region selector
>> + */
>
> NIT: Usually we add documentation on the prototype in the header and not in
> the definition.
I’ll change
>
>> +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 */
>> + }
>> +}
>> +
>> +/*
>> + * Write EL2 MPU Protection Region.
>> + *
>> + * @pr_write: const mpu protection region passed through write op.
>> + * @sel: mpu protection region selector
>> + */
>> +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 */
>> + }
>> +}
>> +
>> void __init setup_mm(void)
>> {
>> BUG_ON("unimplemented");
>
> Cheers,
>
> --
> Julien Grall
>
