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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Sunday, January 29, 2023 3:37 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,
> 

Hi Julien,

> On 29/01/2023 05:39, Penny Zheng wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: Thursday, January 19, 2023 11:04 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,
> >>
> >
> > Hi Julien
> >
> > Sorry for the late response, just come back from Chinese Spring
> > Festival Holiday~
> >
> >> On 13/01/2023 05:28, Penny Zheng wrote:
> >>> From: Penny Zheng <penny.zheng@xxxxxxx>
> >>>
> >>> The start-of-day Xen MPU memory region layout shall be like as follows:
> >>>
> >>> xen_mpumap[0] : Xen text
> >>> xen_mpumap[1] : Xen read-only data
> >>> xen_mpumap[2] : Xen read-only after init data xen_mpumap[3] : Xen
> >>> read-write data xen_mpumap[4] : Xen BSS ......
> >>> xen_mpumap[max_xen_mpumap - 2]: Xen init data
> >>> xen_mpumap[max_xen_mpumap - 1]: Xen init text
> >>
> >> Can you explain why the init region should be at the end of the MPU?
> >>
> >
> > As discussed in the v1 Serie, I'd like to put all transient MPU
> > regions, like boot-only region, at the end of the MPU.
> 
> I vaguely recall the discussion but can't seem to find the thread. Do you have
> a link? (A summary in the patch would have been nice)
> 
> > Since they will get removed at the end of the boot, I am trying not to
> > leave holes in the MPU map by putting all transient MPU regions at rear.
> 
> I understand the principle, but I am not convinced this is worth it because of
> the increase complexity in the assembly code.
> 
> What would be the problem with reshuffling partially the MPU once we
> booted?

 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.

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

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. 

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

[1] https://lists.xenproject.org/archives/html/xen-devel/2022-11/msg00457.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2023-01/msg00795.html

> 
[...]
> >>> +/*
> >>> + * ENTRY to configure a EL2 MPU memory region
> >>> + * ARMv8-R AArch64 at most supports 255 MPU protection regions.
> >>> + * See section G1.3.18 of the reference manual for ARMv8-R AArch64,
> >>> + * PRBAR<n>_EL2 and PRLAR<n>_EL2 provides access to the EL2 MPU
> >>> +region
> >>> + * determined by the value of 'n' and PRSELR_EL2.REGION as
> >>> + * PRSELR_EL2.REGION<7:4>:n.(n = 0, 1, 2, ... , 15)
> >>> + * For example to access regions from 16 to 31 (0b10000 to 0b11111):
> >>> + * - Set PRSELR_EL2 to 0b1xxxx
> >>> + * - Region 16 configuration is accessible through PRBAR0_EL2 and
> >>> +PRLAR0_EL2
> >>> + * - Region 17 configuration is accessible through PRBAR1_EL2 and
> >>> +PRLAR1_EL2
> >>> + * - Region 18 configuration is accessible through PRBAR2_EL2 and
> >>> +PRLAR2_EL2
> >>> + * - ...
> >>> + * - Region 31 configuration is accessible through PRBAR15_EL2 and
> >>> +PRLAR15_EL2
> >>> + *
> >>> + * Inputs:
> >>> + * x27: region selector
> >>> + * x28: preserve value for PRBAR_EL2
> >>> + * x29: preserve value for PRLAR_EL2
> >>> + *
> >>> + */
> >>> +ENTRY(write_pr)
> >>
> >> AFAICT, this function would not be necessary if the index for the
> >> init sections were hardcoded.
> >>
> >> So I would like to understand why the index cannot be hardcoded.
> >>
> >
> > The reason is that we are putting init sections at the *end* of the
> > MPU map, and the length of the whole MPU map is platform-specific. We
> read it from MPUIR_EL2.
> 
> Right, I got that bit from the code. What I would like to understand is why 
> all
> the initial address cannot be hardocoded?
> 
>  From a brief look, this would simplify a lot the assembly code.
> 

Like I said before,  "map towards higher slot", if it is not the tail, it is 
hard to decide another
number to meet different platforms and various FDT static configuration.

If we, in assembly, put fixed regions in front and boot-only regions after, 
then, when we
enter C world, we immediately do a simple reshuffle, which means that we need 
to relocate
these init sections to tail, it is workable only when MPU is disabled, unless 
we're sure that
"reshuffling part" is not using any init codes or data.
  
> >
> >>> +    msr   PRSELR_EL2, x27
> >>> +    dsb   sy
> >>
> >> [...]
> >>
> >>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index
> >>> bc45ea2c65..79965a3c17 100644
> >>> --- a/xen/arch/arm/xen.lds.S
> >>> +++ b/xen/arch/arm/xen.lds.S
> >>> @@ -91,6 +91,8 @@ SECTIONS
> >>>          __ro_after_init_end = .;
> >>>      } : text
> >>>
> >>> +  . = ALIGN(PAGE_SIZE);
> >>
> >> Why do you need this ALIGN?
> >>
> >
> > I need a symbol as the start of the data section, so I introduce
> > "__data_begin = .;".
> > If I use "__ro_after_init_end = .;" instead, I'm afraid in the future,
> > if someone introduces a new section after ro-after-init section, this
> > part also needs modification too.
> 
> I haven't suggested there is a problem to define a new symbol. I am merely
> asking about the align.
> 
> >
> > When we define MPU regions for each section in xen.lds.S, we always
> > treat these sections page-aligned.
> > I checked each section in xen.lds.S, and ". = ALIGN(PAGE_SIZE);" is
> > either added at section head, or at the tail of the previous section,
> > to make sure starting address symbol page-aligned.
> >
> > And if we don't put this ALIGN, if "__ro_after_init_end " symbol
> > itself is not page-aligned, the two adjacent sections will overlap in MPU.
> 
> __ro_after_init_end *has* to be page aligned because the permissions are
> different than for __data_begin.
> 
> If we were going to add a new section, then either it has the same permission
> as .data.read.mostly and we will bundle them or it doesn't and we would
> need a .align.
> 
> But today, the extra .ALIGN seems unnecessary (at least in the context of this
> patch).
> 

Understood, I'll remove

> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng

--
Penny Zheng