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
