Re: [PATCH v3 7/7] arm/mpu: Implement setup_mpu for MPU system

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Julien,

>>>> +static void __init setup_mpu(void)
>>>> +{
>>>> +    register_t prenr;
>>>> +    unsigned int i = 0;
>>>> +
>>>> +    /*
>>>> +     * MPUIR_EL2.Region[0:7] identifies the number of regions supported by
>>>> +     * the EL2 MPU.
>>>> +     */
>>>> +    max_xen_mpumap = (uint8_t)(READ_SYSREG(MPUIR_EL2) & 
>>>> NUM_MPU_REGIONS_MASK);
>>>> +
>>>> +    /* PRENR_EL2 has the N bit set if the N region is enabled, N < 32 */
>>>> +    prenr = (READ_SYSREG(PRENR_EL2) & PRENR_MASK);
>>>> +
>>>> +    /*
>>>> +     * Set the bitfield for regions enabled in assembly boot-time.
>>>> +     * This code works under the assumption that the code in head.S has
>>>> +     * allocated and enabled regions below 32 (N < 32).
>>>> +
>>> This is a bit fragile. I think it would be better if the bitmap is set by 
>>> head.S as we add the regions. Same for ...
>> So, I was trying to avoid that because in that case we need to place 
>> xen_mpumap out of the BSS and start
>> manipulating the bitmap from asm, instead I was hoping to use the C code, I 
>> understand that if someone
>> wants to have more than 31 region as boot region this might break, but it’s 
>> also a bit unlikely?
> 
> The 31 is only part of the problem. The other problem is there is at least 
> one other places in Xen (i.e. as early_fdt_map()) which will also create an 
> entry in the MPU before setup_mm()/setup_mpu() is called. I am a bit 
> concerned that the assumption is going to spread and yet we have no way to 
> programmatically check if this will be broken.

If we would like to find ways, we could check eventually for clashes on enabled 
MPU regions;
so the only part where a region is created in the C world without the 
assistance of xen_mpumap is early_fdt_map(),
asserts etc could be used in one or both setup_mpu and early_fdt_map to prevent 
breakage.

> 
> Furthermore, right now, you are hardcoding the slot used and updating the 
> MPU. But if you had the bitmap updated, you could just look up for a free 
> slot.

of course, but still the early DTB map is temporary and it will be gone after 
boot, so it won’t impact much unless I’m
missing something. 

> 
>> So I was balancing the pros to manipulate everything from the C world 
>> against the cons (boot region > 31).
>> Is it still your preferred way to handle everything from asm?
> 
> Yes. I don't think the change in asm will be large and this would allow to 
> remove other assumptions (like in the FDT mapping code).

not large, but still something to be maintained, we will need arm64/arm32 code 
to set/clear bits on the bitmap
(causing duplication with bitops.c), code to save things on the xen_mpumap, 
code to clean/invalidate dcache for the entries in xen_mpumap and finally we 
will need to keep the code aligned to the implementation of the bitmap
(which is fairly stable, but still needs to be taken into account).

> 
> As a side note, I noticed that the MPU entries are not cleared before we 
> enable the MPU. Is there anything in the boot protocol that guarantee all the 
> entries will be invalid? If not, then I think we need to clear the entries.
> 
> Otherwise, your current logic doesn't work. That said, I think it would still 
> be necessary even if we get rid of your logic because we don't know the 
> content of the MPU entries.

The PRLAR.EN bit resets to zero on a warm reset, so any region will be always 
disabled unless programmed, I thought it is enough.

Cheers,
Luca