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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Michal,

>> 
>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>> index 1368b2eb990f..40a86140b6cc 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -17,6 +17,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   (0xFFFFULL << 48)
> This does not look like a common macro. It's arm64 specific.
> Also, it looks like you use it in macros that are common too.

Yes right, I’ll move that into asm/arm64/mpu.h

>> 
>> +/*
>> + * Reads the MPU region with index 'sel' from the HW.
> If you use @foo style, you should use @sel here.
> But IMO this comment does not bring any usefulness.
> The name of the helper and parameter description is enough.
> 
>> + *
>> + * @pr_read: mpu protection region returned by read op.
>> + * @sel: mpu protection region selector
>> + */
>> +extern void read_protection_region(pr_t *pr_read, uint8_t sel);
>> +
>> +/*
>> + * Writes the MPU region with index 'sel' to the HW.
>> + *
>> + * @pr_write: const mpu protection region passed through write op.
> No need to say const in parameter description
> 
>> + * @sel: mpu protection region selector
> Same here.

I’ll modify these above

>> 
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 9eab09ff2044..40ccf99adc94 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -8,6 +8,8 @@
>> #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;
>> 
>> @@ -26,6 +28,35 @@ DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
>> /* EL2 Xen MPU memory region mapping table. */
>> pr_t __section(".data.page_aligned") xen_mpumap[MAX_MPU_REGION_NR];
>> 
>> +#ifdef CONFIG_ARM_64
>> +/*
>> + * The following are needed for the case generators 
>> GENERATE_WRITE_PR_REG_CASE
> It's read a bit odd. Perhaps remove 'generators' word and use 'cases:'

ok

>> 
>> 
>> +#ifdef CONFIG_ARM_64
>> +/*
>> + * Armv8-R supports direct access and indirect access to the MPU regions 
>> through
>> + * registers, indirect access involves changing the MPU region selector, 
>> issuing
> s/registers,/registers:/ and perhaps use bullet points
> 
>> + * an isb barrier and accessing the selected region through specific 
>> registers;
>> + * instead direct access involves accessing specific registers that points 
>> to
>> + * a specific MPU region, without changing the selector (in some cases) and
> What do you mean by "in some cases"?

what I had in mind was that eventually you’ll need to change the selector at 
some point,
like arm64 every 16 regions or on arm32 from region 32 onwards, but maybe I can 
simplify
and remove this part.

> 
>> + * issuing barriers because of that.
>> + * For Arm64 the PR{B,L}AR_ELx (for n=0) and PR{B,L}AR<n>_ELx, n=1..15, are 
>> used
> If for n==0 you used (), why not following the same style for 1..15?
> It all improves readability of such long comments.
> 
>> + * for the direct access to the regions selected by 
>> PRSELR_EL2.REGION<7:4>:n, so
>> + * 16 regions can be directly access when the selector is multiple of 16, 
>> giving
> s/access/accessed/
> s/is multiple/is a multiple/

ok all the above

Cheers,
Luca