Hi Michal,
>>
>> +/*
>> + * Excute never.
>> + * Stage 1 EL2 translation regime.
>> + * XN[1] determines whether execution of the instruction fetched from the
>> MPU
>> + * memory region is permitted.
>> + * Stage 2 EL1/EL0 translation regime.
>> + * XN[0] determines whether execution of the instruction fetched from the
>> MPU
>> + * memory region is permitted.
>> + */
> Why do we need this comment? If any, why do we need EL1 description if the
> macro
> is EL2?
Uhm yes I think I can remove altogether this comment
>
>> +#define PRBAR_EL2_XN_ENABLED 0x2
>> +
>> #ifndef __ASSEMBLY__
>>
>> /* Protection Region Base Address Register */
>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>> index 0e0a7f05ade9..7b82f10d336b 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -24,6 +24,10 @@
>> #define NUM_MPU_REGIONS_MASK (NUM_MPU_REGIONS - 1)
>> #define MAX_MPU_REGION_NR 255
>>
>> +/* Access permission attributes. */
>> +/* Read/Write at EL2, No Access at EL1/EL0. */
>> +#define AP_RW_EL2 0x0
> This macro and the previous one are used only once (I also checked your full
> tree) and cannot be set by the caller. What's the purpose of the macros then?
> Why can't we set these values in pr_of_xenaddr() and add comment next to value
> there?
Sure, I’ll do that
>
>> +
>> #ifndef __ASSEMBLY__
>>
>> #ifdef CONFIG_ARM_64
>> diff --git a/xen/arch/arm/include/asm/mpu/mm.h
>> b/xen/arch/arm/include/asm/mpu/mm.h
>> index e2235e568e81..296fe74c5d61 100644
>> --- a/xen/arch/arm/include/asm/mpu/mm.h
>> +++ b/xen/arch/arm/include/asm/mpu/mm.h
>> @@ -75,6 +75,16 @@ extern void read_protection_region(pr_t *pr_read, uint8_t
>> sel);
>> */
>> extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
> Here and ...
>
>>
>> +/*
>> + * Creates a pr_t structure describing a protection region.
>> + *
>> + * @base: base address as base of the protection region.
>> + * @limit: exclusive address as limit of the protection region.
>> + * @attr: attribute index for the memory type.
>> + * @return: pr_t structure describing a protection region.
>> + */
>> +extern pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int
>> attr_idx);
> here. Please don't use extern in prototypes. It's not needed.
I see we have a mixed usage of this in arch/arm and it’s not documented to do
otherwise
in the code style, in this case I would prefer to be explicit unless it’s a
strong objection on your side,
let me know.
>>
>> +pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx)
>> +{
>> + prbar_t prbar;
>> + prlar_t prlar;
>> + pr_t region;
>> +
>> + /* Build up value for PRBAR_EL2. */
>> + prbar = (prbar_t) {
>> + .reg = {
>> + .ap = AP_RW_EL2, /* Read/Write at EL2, no access at
>> EL1/EL0. */
>> + .xn = PRBAR_EL2_XN_ENABLED, /* No need to execute outside
>> .text */
>> + }};
>> +
>> + switch ( attr_idx )
>> + {
>> + case MT_NORMAL_NC:
>> + /*
>> + * ARM ARM: Overlaying the shareability attribute (DDI
>> + * 0406C.b B3-1376 to 1377)
> It's a bit odd to provide here the manual for Armv7.
> Also, our general advice is to use the latest revision.
I suspect this was copied from mfn_to_xen_entry, I’ll try to find the latest
reference to this.
>
>> + *
>> + * A memory region with a resultant memory type attribute of normal,
>> + * and a resultant cacheability attribute of Inner non-cacheable,
>> + * outer non-cacheable, must have a resultant shareability attribute
>> + * of outer shareable, otherwise shareability is UNPREDICTABLE.
>> + *
>> + * On ARMv8 sharability is ignored and explicitly treated as outer
>> + * shareable for normal inner non-cacheable, outer non-cacheable.
>> + */
>> + prbar.reg.sh = LPAE_SH_OUTER;
>> + break;
>> + case MT_DEVICE_nGnRnE:
>> + case MT_DEVICE_nGnRE:
>> + /*
>> + * Shareability is ignored for non-normal memory, Outer is as
>> + * good as anything.
>> + *
>> + * On ARMv8 sharability is ignored and explicitly treated as outer
> Does this Armv8 comments make sense? We don't support Armv7 MPU.
>
>> + * shareable for any device memory type.
>> + */
>> + prbar.reg.sh = LPAE_SH_OUTER;
>> + break;
>> + default:
>> + /* Xen mappings are SMP coherent */
>> + prbar.reg.sh = LPAE_SH_INNER;
> AFAIR MISRA C requires every clause to be terminated with break.
> That said I don't remember if we accepted this rule...
I’ll add a break, it doesn’t hurt.
Cheers,
Luca
