RE: [PATCH v2 13/40] xen/mpu: introduce unified function setup_early_uart to map early UART

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, January 31, 2023 5:42 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 13/40] xen/mpu: introduce unified function
> setup_early_uart to map early UART
> 
> Hi Penny,
> 
> On 31/01/2023 05:38, Penny Zheng wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: Monday, January 30, 2023 6:00 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 13/40] xen/mpu: introduce unified function
> >> setup_early_uart to map early UART
> >>
> >>
> >>
> >> On 30/01/2023 06:24, Penny Zheng wrote:
> >>> Hi, Julien
> >>
> >> Hi Penny,
> >>
> >>>> -----Original Message-----
> >>>> From: Julien Grall <julien@xxxxxxx>
> >>>> Sent: Sunday, January 29, 2023 3:43 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 13/40] xen/mpu: introduce unified function
> >>>> setup_early_uart to map early UART
> >>>>
> >>>> Hi Penny,
> >>>>
> >>>> On 29/01/2023 06:17, Penny Zheng wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Julien Grall <julien@xxxxxxx>
> >>>>>> Sent: Wednesday, January 25, 2023 3:09 AM
> >>>>>> 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 13/40] xen/mpu: introduce unified function
> >>>>>> setup_early_uart to map early UART
> >>>>>>
> >>>>>> Hi Peny,
> >>>>>
> >>>>> Hi Julien,
> >>>>>
> >>>>>>
> >>>>>> On 13/01/2023 05:28, Penny Zheng wrote:
> >>>>>>> In MMU system, we map the UART in the fixmap (when earlyprintk
> >>>>>>> is
> >>>> used).
> >>>>>>> However in MPU system, we map the UART with a transient MPU
> >>>> memory
> >>>>>>> region.
> >>>>>>>
> >>>>>>> So we introduce a new unified function setup_early_uart to
> >>>>>>> replace the previous setup_fixmap.
> >>>>>>>
> >>>>>>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> >>>>>>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> >>>>>>> ---
> >>>>>>>      xen/arch/arm/arm64/head.S               |  2 +-
> >>>>>>>      xen/arch/arm/arm64/head_mmu.S           |  4 +-
> >>>>>>>      xen/arch/arm/arm64/head_mpu.S           | 52
> >>>>>> +++++++++++++++++++++++++
> >>>>>>>      xen/arch/arm/include/asm/early_printk.h |  1 +
> >>>>>>>      4 files changed, 56 insertions(+), 3 deletions(-)
> >>>>>>>
> > Yes, I'll draw the layout for you:
> 
> Thanks!
> 
> > '''
> >   Xen MPU Map before reorg:
> >
> > 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[5] : Xen static
> > heap ......
> > xen_mpumap[max_xen_mpumap - 7]: Static shared memory section
> > xen_mpumap[max_xen_mpumap - 6]: Boot Module memory
> section(kernel,
> > initramfs, etc) xen_mpumap[max_xen_mpumap - 5]: Device memory
> section
> > xen_mpumap[max_xen_mpumap - 4]: Guest memory section
> > xen_mpumap[max_xen_mpumap - 3]: Early FDT
> xen_mpumap[max_xen_mpumap -
> > 2]: Xen init data xen_mpumap[max_xen_mpumap - 1]: Xen init text
> >
> > In the end of boot, function init_done will do the reorg and boot-only
> region clean-up:
> >
> > Xen MPU Map after reorg(idle vcpu):
> >
> > xen_mpumap[0] : Xen text
> > xen_mpumap[1] : Xen read-only data
> > xen_mpumap[2] : Xen read-only after init data
> 
> In theory 1 and 2 could be merged after boot. But I guess it might be
> complicated?
> 

In theory, if in C merging codes, we do not use any read-only data or 
read-only-after-init
data, then, ig, it will be ok.
Since, In MPU system, when we implement C merging codes, we need to disable 
region 1 and 2
firstly, and enable the merged region after. The reason is that two MPU regions 
with address overlapping
is not allowed when MPU on.
 
> > xen_mpumap[3] : Xen read-write data
> > xen_mpumap[4] : Xen BSS
> > xen_mpumap[5] : Xen static heap
> > xen_mpumap[6] : Guest memory section
> 
> Why do you need to map the "Guest memory section" for the idle vCPU?
> 

Hmmm, "Guest memory section" here refers to *ALL* guest RAM address range with 
only EL2 read/write access.

For guest vcpu, this section will be replaced by guest itself own RAM with both 
EL1/EL2 access.


> > xen_mpumap[7] : Device memory section
> 
> I might be missing some context here. But why this section is not also
> mapped in the context of the guest vCPU?
> 
> For instance, how would you write to the serial console when the context is
> the guest vCPU?
> 

I think, as Xen itself, it shall have access to all system device memory on EL2.
Ik, it is not accurate in current MMU implementation, only devices with 
supported driver
will get ioremap.

But like we discussed before, if following the same strategy as MMU does, with 
limited
MPU regions, we could not afford mapping a MPU region for each device.
For example, On FVPv8R model, we have four uarts, and a GICv3. At most, we may 
provide
four MPU regions for uarts, and two MPU regions for Distributor and one 
Redistributor region.
So, I thought up this new device tree property “mpu,device-memory-section = 
<0x0 0x80000000 0x0 0x7ffff000>;“
to roughly map all system device memory for Xen itself.

For guest, it shall only see vgic, vpl011, and its own passthrough device. And 
here, to maintain safe and
isolation, we will be mapping a MPU region for each device for guest vcpu.
For example, vgic and vpl011 are emulated and direct-map in MPU. Relevant device
mapping(GFN == MFN with only EL2 access)will be added to its *P2M mapping 
table*, in vgic_v3_domain_init [1].

Later, on vcpu context switching, when switching from idle vcpu, device memory 
section gets disabled
and switched out in ctxt_switch_from [2], later when switching into guest vcpu, 
vgic and vpl011 device mapping
will be switched in along with the whole P2M mapping table [3]. 

Words might be ambiguous, but all related code implementation is on MPU patch 
serie part II - guest initialization, you may
have to check the gitlab link:
[1] 
https://gitlab.com/xen-project/people/weic/xen/-/commit/a51d5b25eb17a50a36b27987a2f48e14793ac585
 
[2] 
https://gitlab.com/xen-project/people/weic/xen/-/commit/c6a069d777d9407aeda42b7e5b08a086a1c15976
 
[3] 
https://gitlab.com/xen-project/people/weic/xen/-/commit/d8c6408b6eef1190d75c9bd4e58557d34fc8b4df
 

> > xen_mpumap[6] : Static shared memory section
> >
> > Xen MPU Map on runtime(guest vcpu):
> >
> > 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[5] : Xen static
> > heap xen_mpumap[6] : Guest memory xen_mpumap[7] : vGIC map
> > xen_mpumap[8] : vPL011 map
> 
> I was expected the PL011 to be fully emulated. So why is this necessary?
> 
> > xen_mpumap[9] : Passthrough device map(UART, etc) xen_mpumap[10] :
> > Static shared memory section
> >
> >>>
> >>> That will be already at least 13 MPU regions ;\.
> >>
> >> The section I am the most concern of is mpu,device-memory-section
> >> because it would likely mean that all the devices will be mapped in Xen.
> >> Is there any risk that the guest may use different memory attribute?
> >>
> >
> > Yes, on current implementation, per-domain vgic, vpl011, and
> > passthrough device map will be individually added into per-domain P2M
> > mapping, then when switching into guest vcpu from xen idle vcpu,
> > device memory section will be replaced by vgic, vpl011, passthrough
> device map.
> 
> Per above, I am not entirely sure how you could remove the device memory
> section when using the guest vCPU.
> 
> Now about the layout between init and runtime. From previous discussion,
> you said you didn't want to have init section to be fixed because of the
> section "Xen static heap".
> 
> Furthermore, you also mention that you didn't want a bitmap. So how
> about the following for the assembly part:
> 
> 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[5]: Early FDT
> xen_mpumap[6]: Xen init data
> xen_mpumap[7]: Xen init text
> xen_mpumap[8]: Early UART (optional)
> 
> Then when you switch to C, you could have:
> 
> 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[5]: Early FDT
> xen_mpumap[6]: Xen init data
> xen_mpumap[7]: Xen init text
> 
> xen_mpumap[max_xen_mpumap - 4]: Device memory section
> xen_mpumap[max_xen_mpumap - 3]: Guest memory section
> xen_mpumap[max_xen_mpumap - 2]: Static shared memory section
> xen_mpumap[max_xen_mpumap - 1] : Xen static heap
> 
> And at runtime, you would keep the "Xen static heap" right at the end of
> the MPU and keep the middle entries as the switchable one.
> 
> There would be not bitmap with this solution and all the entries for the
> assembly code would be fixed.
> 

That's really smart! Thanks!

I've done a little twist on your design: I've put all switchable ones in the 
middle
after regions defined in assembly. If near Xen heap region, I'm afraid if one 
day in the future,
someone is introducing another fixed region in C, we will leave holes there.

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
( Fixed MPU region defined in assembly )
--------------------------------------------------------------------------
xen_mpumap[5]: Xen init data
xen_mpumap[6]: Xen init text
xen_mpumap[7]: Early FDT
xen_mpumap[8]: Guest memory section
xen_mpumap[9]: Device memory section
xen_mpumap[10]: Static shared memory section
( boot-only and switching regions defined in C )
--------------------------------------------------------------------------
...
xen_mpumap[max_xen_mpumap - 1] : Xen static heap
( Fixed MPU region defined in C )
--------------------------------------------------------------------------

After re-org:
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
( Fixed MPU region defined in assembly )
--------------------------------------------------------------------------
xen_mpumap[8]: Guest memory section
xen_mpumap[9]: Device memory section
xen_mpumap[10]: Static shared memory section
( Switching region )
--------------------------------------------------------------------------
...
xen_mpumap[max_xen_mpumap - 1] : Xen static heap
( Fixed MPU region defined in C )

If you're fine with it, then next serie, I'll use this layout, to keep both
simple assembly and re-org process.

> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng