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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 15/04/2025 13:10, Luca Fancellu wrote:
> Hi Julien,
> 
>> On 15 Apr 2025, at 11:20, Julien Grall <julien@xxxxxxx> wrote:
>>
>> Hi Luca,
>>
>> On 15/04/2025 16:55, Luca Fancellu wrote:
>>> 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.
>>
>> It doesn't really matter whether the region is temporary or not. My concern 
>> is you are making assumption that are difficult to track (they are not 
>> documented where a developper would most likely look at).
>>
>> So if we still want to hardcode the value, then this should be documented in 
>> head.S and probably in a layout.h (or similar) so there is a single place 
>> where the MPU layout is described.
> 
> Sure, I’m fine with documenting everything, let’s see ...
> 
>>
>>>>
>>>>> 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).
>>
>> I understand the changes and risks, but I still think this is the right 
>> approach. Let see what the other maintainers think.
> 
> what the other maintainers thinks about this one. 
I read the thread and my opinion is that we should do that in ASM even it if
means more code and possible duplication. From maintainer perspective this is
easier to handle than assumptions which are more error prone and difficult to
change in the future.

> 
>>
>>>>
>>>> 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.
>>
>> This is only telling me the state PRLAR.EN when the CPU is initially turn 
>> on. This doesn't tell me the value of the bit when jumping in Xen.
>>
>> I am making the difference because there might be another software running 
>> at EL2 before jumping into Xen (e.g. bootloader, or even a previous Xen if 
>> we were using Kexec) which could use the MPU.
>>
>> So I am looking for some details on how the expected state of the system 
>> when jumping to an OS/hypervisor. For a comparison, on the MMU side, we have 
>> the Linux arm64 Image protocol that will specific how
>> a bootloader needs to configure the system.
> 
> Ok I now understand the question, so I think we still could use the Linux 
> arm64 Image protocol, but we will need to define what we expect for the MPU, 
> is docs/misc/arm/booting.txt
> the right place for it? Shall we start a different thread?
Please start a new thread for documenting booting protocol for ARMv8-R.

~Michal