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

[Julien Grall <julien@xxxxxxx>]

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

Cheers,

--
Julien Grall