RE: [PATCH v2 04/40] xen/arm: add an option to define Xen start address for Armv8-R

[Wei Chen <Wei.Chen@arm.com>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2023年1月18日 17:44
> To: Wei Chen <Wei.Chen@arm.com>; Penny Zheng <Penny.Zheng@arm.com>; xen-
> devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>;
> Jiamei Xie <Jiamei.Xie@arm.com>
> Subject: Re: [PATCH v2 04/40] xen/arm: add an option to define Xen start
> address for Armv8-R
> 
> 
> 
> On 18/01/2023 03:00, Wei Chen wrote:
> > Hi Julien,
> 
> Hi Wei,
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: 2023年1月18日 7:24
> >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Jiamei Xie
> >> <Jiamei.Xie@arm.com>
> >> Subject: Re: [PATCH v2 04/40] xen/arm: add an option to define Xen
> start
> >> address for Armv8-R
> >>
> >> Hi Penny,
> >>
> >> On 13/01/2023 05:28, Penny Zheng wrote:
> >>> From: Wei Chen <wei.chen@arm.com>
> >>>
> >>> On Armv8-A, Xen has a fixed virtual start address (link address
> >>> too) for all Armv8-A platforms. In an MMU based system, Xen can
> >>> map its loaded address to this virtual start address. So, on
> >>> Armv8-A platforms, the Xen start address does not need to be
> >>> configurable. But on Armv8-R platforms, there is no MMU to map
> >>> loaded address to a fixed virtual address and different platforms
> >>> will have very different address space layout. So Xen cannot use
> >>> a fixed physical address on MPU based system and need to have it
> >>> configurable.
> >>>
> >>> In this patch we introduce one Kconfig option for users to define
> >>> the default Xen start address for Armv8-R. Users can enter the
> >>> address in config time, or select the tailored platform config
> >>> file from arch/arm/configs.
> >>>
> >>> And as we introduced Armv8-R platforms to Xen, that means the
> >>> existed Arm64 platforms should not be listed in Armv8-R platform
> >>> list, so we add !ARM_V8R dependency for these platforms.
> >>>
> >>> Signed-off-by: Wei Chen <wei.chen@arm.com>
> >>> Signed-off-by: Jiamei.Xie <jiamei.xie@arm.com>
> >>
> >> Your signed-off-by is missing.
> >>
> >>> ---
> >>> v1 -> v2:
> >>> 1. Remove the platform header fvp_baser.h.
> >>> 2. Remove the default start address for fvp_baser64.
> >>> 3. Remove the description of default address from commit log.
> >>> 4. Change HAS_MPU to ARM_V8R for Xen start address dependency.
> >>>      No matter Arm-v8r board has MPU or not, it always need to
> >>>      specify the start address.
> >>
> >> I don't quite understand the last sentence. Are you saying that it is
> >> possible to have an ARMv8-R system with an MPU nor a page-table?
> >>
> >
> > Yes, from the Cortex-R82 page [1], you can see the MPU is optional in
> EL1
> > and EL2:
> > "Two optional and programmable MPUs controlled from EL1 and EL2
> respectively."
> Would this mean a vendor may provide their custom solution to protect
> the memory?
> 

Ah, you gave me a new idea, yes in the "ARM DDI 0600A.c G1.3.7" MSA_frac
of ID_AA64MMFR0_EL1 says:
0b0000 PMSAv8-64 not supported in any translation regime.
0b0000 is not permitted value.

So maybe you're right, on Armv8-R64, we always have MPU in EL1&EL2, the
optional is for MPU customization.

> >
> > Although it is unlikely that vendors using the Armv8-R IP will do so, it
> > is indeed an option. In the ID register, there are also related bits in
> > ID_AA64MMFR0_EL1 (MSA_frac) to indicate this.
> >
> >>> ---
> >>>    xen/arch/arm/Kconfig           |  8 ++++++++
> >>>    xen/arch/arm/platforms/Kconfig | 16 +++++++++++++---
> >>>    2 files changed, 21 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> >>> index ace7178c9a..c6b6b612d1 100644
> >>> --- a/xen/arch/arm/Kconfig
> >>> +++ b/xen/arch/arm/Kconfig
> >>> @@ -145,6 +145,14 @@ config TEE
> >>>    	  This option enables generic TEE mediators support. It allows
> >> guests
> >>>    	  to access real TEE via one of TEE mediators implemented in
> XEN.
> >>>
> >>> +config XEN_START_ADDRESS
> >>> +	hex "Xen start address: keep default to use platform defined
> >> address"
> >>> +	default 0
> >>> +	depends on ARM_V8R
> >>
> >> It is still pretty unclear to me what would be the difference between
> >> HAS_MPU and ARM_V8R.
> >>
> >
> > If we don't want to support non-MPU supported Armv8-R, I think they are
> the
> > same. IMO, non-MPU supported Armv8-R is meaningless to Xen.
> OOI, why do you think this is meaningless?

If there is Armv8-R board without EL2 MPU, how can we protect Xen? Of course,
if users don't care about security, Xen still can support it.

> 
> >
> >>> +	help
> >>> +	  This option allows to set the customized address at which Xen will
> >> be
> >>> +	  linked on MPU systems. This address must be aligned to a page size.
> >>> +
> >>>    source "arch/arm/tee/Kconfig"
> >>>
> >>>    config STATIC_SHM
> >>> diff --git a/xen/arch/arm/platforms/Kconfig
> >> b/xen/arch/arm/platforms/Kconfig
> >>> index c93a6b2756..0904793a0b 100644
> >>> --- a/xen/arch/arm/platforms/Kconfig
> >>> +++ b/xen/arch/arm/platforms/Kconfig
> >>> @@ -1,6 +1,7 @@
> >>>    choice
> >>>    	prompt "Platform Support"
> >>>    	default ALL_PLAT
> >>> +	default FVP_BASER if ARM_V8R
> >>>    	---help---
> >>>    	Choose which hardware platform to enable in Xen.
> >>>
> >>> @@ -8,13 +9,14 @@ choice
> >>>
> >>>    config ALL_PLAT
> >>>    	bool "All Platforms"
> >>> +	depends on !ARM_V8R
> >>>    	---help---
> >>>    	Enable support for all available hardware platforms. It
> doesn't
> >>>    	automatically select any of the related drivers.
> >>>
> >>>    config QEMU
> >>>    	bool "QEMU aarch virt machine support"
> >>> -	depends on ARM_64
> >>> +	depends on ARM_64 && !ARM_V8R
> >>>    	select GICV3
> >>>    	select HAS_PL011
> >>>    	---help---
> >>> @@ -23,7 +25,7 @@ config QEMU
> >>>
> >>>    config RCAR3
> >>>    	bool "Renesas RCar3 support"
> >>> -	depends on ARM_64
> >>> +	depends on ARM_64 && !ARM_V8R
> >>>    	select HAS_SCIF
> >>>    	select IPMMU_VMSA
> >>>    	---help---
> >>> @@ -31,14 +33,22 @@ config RCAR3
> >>>
> >>>    config MPSOC
> >>>    	bool "Xilinx Ultrascale+ MPSoC support"
> >>> -	depends on ARM_64
> >>> +	depends on ARM_64 && !ARM_V8R
> >>>    	select HAS_CADENCE_UART
> >>>    	select ARM_SMMU
> >>>    	---help---
> >>>    	Enable all the required drivers for Xilinx Ultrascale+ MPSoC
> >>>
> >>> +config FVP_BASER
> >>> +	bool "Fixed Virtual Platform BaseR support"
> >>> +	depends on ARM_V8R
> >>> +	help
> >>> +	  Enable platform specific configurations for Fixed Virtual
> >>> +	  Platform BaseR
> >>
> >> This seems unrelated to this patch.
> >>
> >
> > Can we add some descriptions in commit log for this change, or we
> > Should move it to a new patch?
> 
> New patch please or introduce it in the patch where you need it.
> 
> We had preferred to use separate
> > patches for this kind of changes, but we found the number of patches
> > would become more and more. This problem has been bothering us for
> > organizing patches.
> 
> I understand the concern of increasing the number of patches. However,
> this also needs to weight against the review.
> 

Understand.

> In this case, it is very difficult for me to understand why we need to
> introduce FVP_BASER.
> 
> In fact, on the previous version, we discussed to not introduce any new
> platform specific config. So I am a bit surprised this is actually needed.
> 

No, this is no true, it's my mistake, I forgot to remove FVP_BASER from
this Kconfig. Actually, we do not need this one. We also don't need a
new patch for it.

Cheers,
Wei Chen

> Cheers,
> 
> --
> Julien Grall