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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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. 

> 
>>> 
>>> 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?

So I could use your help to define it in the best way possible, since 
unfortunately we don’t have anything available.
The only implementation of a bootloader is the boot-wrapper-aarch64 which keeps 
the MPU off, I/D cache off.

So shall we add in the "Firmware/bootloader requirements” that on Armv8-R we 
should enter Xen with MPU off and D cache off,
Xen shall use spin-table as enable method for the CPUs?

We should clear all the available MPU region before enabling the MPU then.

Please let me know your thoughts.

Cheers,
Luca