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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 11 Nov 2022, Wei Chen wrote:
> 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?

Yes


> If yes I think this method might be better for now. And about ImageBuilder
> solution we can do it after MPU support be merged?

That's fine