Re: [PATCH v6 05/11] xen/arm: define Xen start address for FVP BaseR platform

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 9 Nov 2022, Julien Grall wrote:
> > > -----Original Message-----
> > > From: Julien Grall <julien@xxxxxxx>
> > > Sent: 2022年11月7日 3:20
> > > To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> > > Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand
> > > Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> > > <Volodymyr_Babchuk@xxxxxxxx>; Jiamei Xie <Jiamei.Xie@xxxxxxx>
> > > Subject: Re: [PATCH v6 05/11] xen/arm: define Xen start address for FVP
> > > BaseR platform
> > > 
> > > 
> > > 
> > > On 04/11/2022 10:07, Wei Chen wrote:
> > > > 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.
> > > > 
> > > > So in this patch, we reuse the existing arm/platforms to store
> > > > Armv8-R platforms' parameters. And `XEN_START_ADDRESS` is one
> > > > kind of FVP BaseR platform's parameters. So we define default
> > > > `XEN_START_ADDRESS` for FVP BaseR in its platform file.
> > > > 
> > > > We also introduce one Kconfig option for users to override the
> > > > default Xen start address of selected platform, if they think
> > > > the default address doesn't suit their scenarios. For this
> > > > Kconfig option, we use an unaligned address "0xffffffff" as the
> > > > default value to indicate that users haven't used a customized
> > > > Xen start address.
> > > > 
> > > > 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@xxxxxxx>
> > > > Signed-off-by: Jiamei.Xie <jiamei.xie@xxxxxxx>
> > > > ---
> > > >    xen/arch/arm/Kconfig                           | 11 +++++++++++
> > > >    xen/arch/arm/include/asm/platforms/fvp_baser.h | 14 ++++++++++++++
> > > 
> > > I looked at the content of fvp_baser.h after this series is applied.
> > > There are a bit of boiler plate that I expect to be part for other
> > > platforms. In particular...
> > > 
> > > >    xen/arch/arm/platforms/Kconfig                 | 16 +++++++++++++---
> > > >    3 files changed, 38 insertions(+), 3 deletions(-)
> > > >    create mode 100644 xen/arch/arm/include/asm/platforms/fvp_baser.h
> > > > 
> > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > > index ad592367bd..ac276307d6 100644
> > > > --- a/xen/arch/arm/Kconfig
> > > > +++ b/xen/arch/arm/Kconfig
> > > > @@ -138,6 +138,17 @@ 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 0xFFFFFFFF
> > > 
> > > ... this default value will need to be tested everywhere. At least for
> > > now, I think you can avoid the per platform header by using the Kconfig
> > > to select the proper address (see the config for selecting early printk
> > > address).
> > > 
> > > This will also avoids to use an invalid value here.
> > > 
> > 
> > We had considered to use Kconfig to define the start addresses of v8R64
> > platforms (prompt users to input the address). But we also want to provide
> > a default start address for each platform (Discussed in [1], header for
> > default value, Kconfig option for customized address).
> Why do you want to provide a default value? And how it is guaranteed that it
> will work for most of the users?
> 
> > 
> > We also had thought to use Kconfig to define a default start address
> > for each platform like what we had done for early printk in RFC[2].
> > But this method has been deprecated.
> 
> Most of the current Xen is board agnostic except the UART. We push back the
> addition of new one because the address can be found in the firmware table and
> I wanted to avoid increase the number of option (there are dozens of platform
> out...).
> 
> > 
> > So if we don’t use header files, just use the Kconfig, we can't
> > provide a default start address for platforms, and have to force users
> > to enter the start address.
> 
> I am not sure I see the problem to force the user to enter the start address.
> My worry with per-platform default value is we end up to force each vendor to
> provide an header in order to boot Xen.
> 
> I think it would be better to provide a config tailored for that platform
> (whether it is part of Xen can be debatable). This would allow a user to try a
> release Xen on their platform with zero changes in the code.

I agree with Julien, especially on this last point.

Of course we need a default configuration for a given platform, we don't
want every user of the same platform to have to go and look at the
manual to find the right address to use.

The question is where to put the per-platform default value. The kconfig
"default" keyword is not great for that and it is not realistic to have
a single address that works everywhere.

Instead, we could have a prepopulated kconfig under
xen/arch/arm/configs, or something under ImageBuilder, or maybe expand
the existing "Platform Support" kconfig menu.

If this was just XEN_START_ADDRESS, I would suggest to keep it in
xen.git somewhere. But given that there are a few addresses and sizes to
provide/calculate for Xen on MPU to work, using ImageBuilder could be a
good idea.