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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Stefano, Julien,

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Stefano Stabellini
> Sent: 2022年11月11日 6:13
> To: Julien Grall <julien@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; 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 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

Do you mean we can keep a config like armv8r_fvp_baser_config in
xen/arch/arm/configs for users to generate a default config?
If yes I think this method might be better for now. And about ImageBuilder
solution we can do it after MPU support be merged?

Cheers,
Wei Chen

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