Re: [PATCH v2 11/40] xen/mpu: build up start-of-day Xen MPU memory region map

[Julien Grall]

On 31/01/2023 04:11, Penny Zheng wrote:
Hi Julien

-----Original Message-----
From: Julien Grall <jul...@xen.org>
Sent: Monday, January 30, 2023 5:40 PM
To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
Cc: Wei Chen <wei.c...@arm.com>; Stefano Stabellini
<sstabell...@kernel.org>; Bertrand Marquis <bertrand.marq...@arm.com>;
Volodymyr Babchuk <volodymyr_babc...@epam.com>
Subject: Re: [PATCH v2 11/40] xen/mpu: build up start-of-day Xen MPU
memory region map

Hi Penny,

On 30/01/2023 05:45, Penny Zheng wrote:
   There are three types of MPU regions during boot-time:
1. Fixed MPU region
Regions like Xen text section, Xen heap section, etc.
2. Boot-only MPU region
Regions like Xen init sections, etc. It will be removed at the end of booting.
3.   Regions need switching in/out during vcpu context switch
Region like system device memory map.
For example, for FVP_BaseR_AEMv8R, we have [0x80000000, 0xfffff000) as
the whole system device memory map for Xen(idle vcpu) in EL2,  when
context switching to guest vcpu, it shall be replaced with
guest-specific device mapping, like vgic, vpl011, passthrough device, etc.

We don't have two mappings for different stage translations in MPU, like
we had in MMU.
Xen stage 1 EL2 mapping and stage 2 mapping are both sharing one MPU
memory mapping(xen_mpumap) So to save the trouble of hunting down
each
switching regions in time-sensitive context switch, we must re-order
xen_mpumap to keep fixed regions in the front, and switching ones in the
heels of them.

  From my understanding, hunting down each switching regions would be a
matter to loop over a bitmap. There will be a slight increase in the number
of instructions executed, but I don't believe it will be noticeable.


In Patch Serie v1, I was adding MPU regions in sequence,  and I
introduced a set of bitmaps to record the location of same type
regions. At the end of booting, I need to *disable* MPU to do the
reshuffling, as I can't move regions like xen heap while MPU on.

And we discussed that it is too risky to disable MPU, and you
suggested [1] "
You should not need any reorg if you map the boot-only section
towards in the higher slot index (or just after the fixed ones).
"

Right, looking at the new code. I realize that this was probably a bad idea
because we are adding complexity in the assembly code.


Maybe in assembly, we know exactly how many fixed regions are,
boot-only regions are, but in C codes, we parse FDT to get static
configuration, like we don't know how many fixed regions for xen static
heap is enough.
Approximation is not suggested in MPU system with limited MPU regions,
some platform may only have 16 MPU regions, IMHO, it is not worthy
wasting in approximation.

I haven't suggested to use approximation anywhere here. I will answer
about the limited number of entries in the other thread.


So I take the suggestion of putting regions in the higher slot index.
Putting fixed regions in the front, and putting boot-only and
switching ones at tail. Then, at the end of booting, when we reorg the
mpu mapping, we remove all boot-only regions, and for switching ones, we
disable-relocate(after fixed ones)-enable them. Specific codes in [2].

  From this discussion, it feels to me that you are trying to make the code
more complicated just to keep the split and save a few cycles (see above).

I would suggest to investigate the cost of "hunting down each section".
Depending on the result, we can discuss what the best approach.


Correct me if I'm wrong, the complicated things in assembly you are worried 
about
is that we couldn't define the index for initial sections, no hardcoded to keep 
simple.

Correct.

And function write_pr, ik, is really a big chunk of codes, however the logic is 
simple there,
just a bunch of "switch-cases".

I agree that write_pr() is a bunch of switch-cases. But there are a lot 
of duplication in it and the interface to use it is, IMHO, not intuitive.


If we are adding MPU regions in sequence as you suggested, while using bitmap 
at the
same time to record used entry.
TBH, this is how I designed at the very beginning internally. We found that if 
we don't
do reorg late-boot to keep fixed in front and switching ones after, each time 
when we
do vcpu context switch, not only we need to hunt down switching ones to disable,
while we add new switch-in regions, using bitmap to find free entry is saying 
that the
process is unpredictable. Uncertainty is what we want to avoid in Armv8-R 
architecture.

I don't understand why it would be unpredictable. For a given 
combination of platform/device-tree, the bitmap will always look the 
same. So the number of cycles/instructions will always be the same.

This is not very different from the case where you split the MPU in two 
because


Hmmm, TBH, I really really like your suggestion to put boot-only/switching 
regions into
higher slot. It really saved a lot trouble in late-init reorg and also avoids 
disabling MPU
at the same time. The split is a simple and easy-to-understand construction 
compared
with bitmap too.

I would like to propose another split. I will reply to that in the 
thread where you provided the MPU layout.

Cheers,

--
Julien Grall