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

[Julien Grall <julien@xxxxxxx>]

On 31/01/2023 04:11, Penny Zheng wrote:
> 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.
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