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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, January 30, 2023 5:40 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> 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.
And function write_pr, ik, is really a big chunk of codes, however the logic is 
simple there,
just a bunch of "switch-cases".

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. 

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.

IMHO, reorg is really worth doing. We put all complicated things in boot-time to
make runtime context-switch simple and fast, even for a few cycles.
As the Armv8-R architecture profile from the beginning is designed to support 
use
cases that have a high sensitivity to deterministic execution. (e.g. Fuel 
Injection,
Brake control, Drive trains, Motor control etc).
However, when talking about architecture thing, I need more professional 
opinions, 
@Wei Chen @Bertrand Marquis
Also, Will R52 implementation encounter the same issue. 
@ayan.kumar.halder@xxxxxxxxxx
@Stefano Stabellini

> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng