Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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