|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 09/10] arm: add QEMU, Rcar3 and MPSoC configs
On Wed, 30 May 2018, Julien Grall wrote:
> On 30/05/2018 22:39, Stefano Stabellini wrote:
> > On Tue, 29 May 2018, Julien Grall wrote:
> > > Hi Stefano,
> > >
> > > On 23/05/18 01:25, Stefano Stabellini wrote:
> > > > Add a "Platform Support" menu with three umbrella kconfig options: QEMU,
> > > > RCAR3 and MPSOC. They enable the required options for their hardware
> > > > platform.
> > > >
> > > > They are introduced for convience: the user will be able to simply open
> > > > the menu and enable the right config for her platform.
> > > >
> > > > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > CC: artem_mygaiev@xxxxxxxx
> > > > CC: volodymyr_babchuk@xxxxxxxx
> > > >
> > > > ---
> > > > Note that this approach has a limitation: it is not possible to "select
> > > > a range". In other words, using tiny.config NR_CPUS is set to 4. It is
> > > > not possible to increase it to 8 from config RCAR3.
> > >
> > > What you can do is:
> > >
> > > config NR_CPUS
> > > range ...
> > > default "8" if (RCAR3)
> > > default "x" if (QEMU)
> > > default 64
> > >
> > > This would imply to move NR_CPUS in arch/{arm,x86}/Kconfig.
> > >
> > > This solution is not very nice, but at least would provide a better
> > > experience
> > > to the user.
> >
> > Unfortunately, make olddefconfig is executed automatically when make is
> > called, adding CONFIG_NR_CPUS=128. Thus, unless tiny.config has already
> > CONFIG_RCAR3 in it, the correct default won't be applied.
> >
> > This suggestions only make sense if we introduce per-platform configs,
> > such as xen/arch/arm/configs/tiny-rcar3.config.
>
> The other solution is to introduce a new command (or script) that will select
> the platform at the same time as olddefconfig.
>
> This would avoid to create a config per board and still keeping only one tiny
> config.
I am not looking forward to making changes to the kconfig commands, but
fortunately I was wrong on my previous reply: the issue was the order of
the defaults in the range! To fix the problem I just had to:
default "8" if ARM && RCAR3
default "4" if ARM && QEMU
default "4" if ARM && MPSOC
default "256" if X86
default "128" if ARM
> > > > Suggestions are welcome.
> > > > ---
> > > > xen/arch/arm/Kconfig | 2 ++
> > > > xen/arch/arm/platforms/Kconfig | 30 ++++++++++++++++++++++++++++++
> > > > 2 files changed, 32 insertions(+)
> > > > create mode 100644 xen/arch/arm/platforms/Kconfig
> > > >
> > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > > index a5a6943..b5ddd12 100644
> > > > --- a/xen/arch/arm/Kconfig
> > > > +++ b/xen/arch/arm/Kconfig
> > > > @@ -245,6 +245,8 @@ config ARM64_HARDEN_BRANCH_PREDICTOR
> > > > config ARM32_HARDEN_BRANCH_PREDICTOR
> > > > def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
> > > > +source "arch/arm/platforms/Kconfig"
> > > > +
> > > > source "common/Kconfig"
> > > > source "drivers/Kconfig"
> > > > diff --git a/xen/arch/arm/platforms/Kconfig
> > > > b/xen/arch/arm/platforms/Kconfig
> > > > new file mode 100644
> > > > index 0000000..0eafbef
> > > > --- /dev/null
> > > > +++ b/xen/arch/arm/platforms/Kconfig
> > > > @@ -0,0 +1,30 @@
> > > > +menu "Platform Support"
> > > > +
> > > > +config QEMU
> > > > + bool "QEMU aarch virt machine support"
> > > > + default n
> > > The default value is confusing here. The default .config will support QEMU
> > > but
> > > not select that.
> > >
> > > While I don't yet buy the argument, some users will also want to remove
> > > platform specific code (Andrii suggest that). This would means by default
> > > support for a specific platform will not be in Xen.
> > >
> > > Furthermore, very likely, the end user will select either one board (e.g
> > > automotive) or all of them (e.g distribution).
> > >
> > > So I think it would be better to do a choice list:
> > > - All -> Board support for all board added. Drivers selected by the
> > > user
> > > - MPSOC -> Select board support for Xilinx + appropriate drivers
> > > - RCAR3 -> Select board support for RCAR3 + appropriate drivers
> > >
> > > The tiny.config would select ALL. This could then be refined by selecting
> > > a
> > > specific platform.
> >
> > The idea of an "ALL" configuration is interesting, however, all the
> > options we would select under "ALL" already default to "Y". Effectively,
> > if we remove the following lines from tiny.config:
> >
> > # CONFIG_GICV3 is not set
> > # CONFIG_MEM_ACCESS is not set
> > # CONFIG_SBSA_VUART_CONSOLE is not set
> > # CONFIG_HAS_NS16550 is not set
> > # CONFIG_HAS_CADENCE_UART is not set
> > # CONFIG_HAS_MVEBU is not set
> > # CONFIG_HAS_PL011 is not set
> > # CONFIG_HAS_SCIF is not set
> > # CONFIG_ARM_SMMU is not set
> >
> > then, it would be as if tiny.config had CONFIG_ALL=y, because after
> > running `make olddefconfig' it would get all these options set to "Y".
> >
> > Given that make olddefconfig is always executed automatically by make, I
> > cannot even remove the "is not set" options above from tiny.config
> > because otherwise they will all be automatically enabled always unless I
> > change all the defaults from Y to N.
> >
> > In fact, the main issue is that it is not possible to deselect Kconfig
> > options using the Kconfig infrastructure. So, if a user has a
> > config with CONFIG_ALL in it, then she executes `make menuconfig'
> > to select RCAR3 and reduce the config size, the menu won't actually be
> > able to deselect any other option automatically. This is very
> > unfortunate. For instance, if the config has CADENCE_UART, and the user
> > selects CONFIG_RCAR3 from the menu, the resulting config will still have
> > CADENCE_UART, unless she goes to remove it by hand.
> >
> > Given all this, I don't know if it is worth introducing CONFIG_ALL. I
> > could add something like:
> >
> > +config ALL
> > + bool "Support for all platforms"
> > + default y
> > + select GICv3
> > + select HAS_NS16550
> > + select HAS_CADENCE_UART
> > + select HAS_PL011
> > + select HAS_EXYNOS4210
> > + select HAS_MVEBU
> > + select HAS_OMAP
> > + select HAS_SCIF
> > + select ARM_SMMU
> > + ---help---
> > + Enable support for all platforms. Triggers the build of a larger Xen
> > + binary but with more drivers.
> > +
> > + If unsure, say Y.
> >
> > but my preference would be to avoid it because it just duplicates the
> > default Y/N settings elsewhere.
>
> This is not what I suggested for all. What I suggested is the option All will
> select all platforms/*.c file to build and does not select any drivers. The
> user will have to chose the drivers. You can see it as a "custom" option.
>
> Also, by a list I meant:
>
> config PLATFORM_RCAR3
> ...
>
> choice
> prompt "Machine"
> default ....
>
> config ALL
> select PLATFORM_RCAR3
> select PLATFORM_XILINX
>
> config RCAR3
> prompt "RCAR 3 support"
> select PLATFORM_RCAR3
> select HAS_PL011
> select ...
>
> endchoice.
>
> The config PLATFORM_* would then be used in the Makefile
> xen-$(CONFIG_PLATFORM_XILINX) += xilinx.o
> ...
>
> I can also understand that there might be issue with the "All" option hence
> why I suggested the "NONE" platform. This would select none of the PLATFORM_*.
Unfortunately it doesn't seem possible to have an option under a choice
menu in kconfig that enables the other options. I get this error:
arch/arm/platforms/Kconfig:1:error: recursive dependency detected!
arch/arm/platforms/Kconfig:1: choice <choice> contains symbol ALL
arch/arm/platforms/Kconfig:9: symbol ALL depends on QEMU
arch/arm/platforms/Kconfig:18: symbol QEMU is part of choice <choice>
Given the lack of better alternatives, I'll stick with what I had
before: all platforms can be enabled manually by ticking all the three
boxes, however, I changed the default to y, so that they will all be
selected in the menu by default. If you have a better suggestion please
reply to the new patch series I'll send shortly.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |