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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Michal,

>> +
>> +pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
>> +{
>> +    unsigned int attr_idx = PAGE_AI_MASK(flags);
>> +    prbar_t prbar;
>> +    prlar_t prlar;
>> +    pr_t region;
>> +
>> +    /* Build up value for PRBAR_EL2. */
>> +    prbar = (prbar_t) {
>> +        .reg = {
>> +            .ro = PAGE_RO_MASK(flags),
>> +            .xn = PAGE_XN_MASK(flags),
> Shouldn't you also initialize .xn_0 and .ap_0 or you rely on compound literal
> implicit zero initialization of unspecified fields? But then you do initialize
> .ns to 0 fror prlar...

Yes, because there I would like to specify that value 0 means Hyp in secure 
world,
but of course if you want I can explicitly initialise also these two

> 
>> +        }};
>> +
>> +    switch ( attr_idx )
>> +    {
>> +    /*
>> +     * ARM ARM: Shareable, Inner Shareable, and Outer Shareable Normal 
>> memory
>> +     * (DDI 0487L.a B2.10.1.1.1 Note section):
>> +     *
>> +     * Because all data accesses to Non-cacheable locations are data 
>> coherent
>> +     * to all observers, Non-cacheable locations are always treated as Outer
>> +     * Shareable
>> +     *
>> +     * ARM ARM: Device memory (DDI 0487L.a B2.10.2)
>> +     *
>> +     * All of these memory types have the following properties:
>> +     * [...]
>> +     *  - Data accesses to memory locations are coherent for all observers 
>> in
>> +     *    the system, and correspondingly are treated as being Outer 
>> Shareable
>> +     */
>> +    case MT_NORMAL_NC:
>> +        /* Fall through */
>> +    case MT_DEVICE_nGnRnE:
>> +        /* Fall through */
>> +    case MT_DEVICE_nGnRE:
>> +        prbar.reg.sh = LPAE_SH_OUTER;
>> +        break;
>> +    default:
>> +        /* Xen mappings are SMP coherent */
>> +        prbar.reg.sh = LPAE_SH_INNER;
>> +        break;
>> +    }
>> +
>> +    /* Build up value for PRLAR_EL2. */
>> +    prlar = (prlar_t) {
>> +        .reg = {
>> +            .ns = 0,        /* Hyp mode is in secure world */
>> +            .ai = attr_idx,
>> +            .en = 1,        /* Region enabled */
>> +        }};
>> +
>> +    /* Build up MPU memory region. */
>> +    region = (pr_t) {
>> +        .prbar = prbar,
>> +        .prlar = prlar,
>> +    };
>> +
>> +    /* Set base address and limit address. */
>> +    pr_set_base(&region, base);
>> +    pr_set_limit(&region, limit);
>> +
>> +    return region;
>> +}
>> #endif
>> 
>> void __init setup_mm(void)
> 
> Other than that:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> ~Michal
> 

Cheers,
Luca