Re: [PATCH v3 09/10] arm: add QEMU, Rcar3 and MPSoC configs

[Julien Grall <julien.grall@arm.com>]

On 05/31/2018 10:38 PM, Stefano Stabellini wrote:
> 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@kernel.org>
>>>>> CC: artem_mygaiev@epam.com
>>>>> CC: volodymyr_babchuk@epam.com
>>>>>
>>>>> ---
>>>>> 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.

Well, what you've implemented is not my suggestion. I never suggested 
that all select RCAR3. Instead it selects PLATFORM_RCAR3.

RCAR3 would select all the drivers + enable PLATFORM_RCAR3.
PLATFORM_RCAR3 would only be used to gate any potential code in platforms.

I would appreciate if you have a look again at my suggestion and explain 
why it would not work for you.

Cheers,

> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel