Re: [RFC PATCH] arm/gicv2: make GICv2 driver and vGICv2 optional

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 2 Aug 2023, at 19:39, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 02/08/2023 18:54, Luca Fancellu wrote:
>>> On 2 Aug 2023, at 18:39, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> Hi Luca,
>>> 
>>> On 02/08/2023 16:05, Luca Fancellu wrote:
>>>>> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> On 02/08/2023 16:42, Luca Fancellu wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>>>>>> 
>>>>>>> Hi Luca,
>>>>>>> 
>>>>>>> On 02/08/2023 15:53, Luca Fancellu wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Introduce Kconfig GICV2 to be able to compile the GICv2 driver only
>>>>>>>> when needed, the option is active by default.
>>>>>>>> 
>>>>>>>> Introduce Kconfig VGICV2 that depends on GICV2 or GICV3 and compiles
>>>>>>>> the GICv2 emulation for guests, it is required only when using GICV2
>>>>>>>> driver, otherwise using GICV3 it is optional and can be deselected
>>>>>>>> if the user doesn't want to offer the vGICv2 interface to guests or
>>>>>>>> maybe its GICv3 hardware can't offer the GICv2 compatible mode.
>>>>>>>> 
>>>>>>>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>>>>>>> ---
>>>>>>>> xen/arch/arm/Kconfig        | 13 +++++++++++++
>>>>>>>> xen/arch/arm/Makefile       |  4 ++--
>>>>>>>> xen/arch/arm/domain_build.c |  4 ++++
>>>>>>>> xen/arch/arm/gic-v3.c       |  4 ++++
>>>>>>>> xen/arch/arm/vgic.c         |  2 ++
>>>>>>>> 5 files changed, 25 insertions(+), 2 deletions(-)
>>>>>>>> 
>>>>>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>>>>>> index fd57a82dd284..dc702f08ace7 100644
>>>>>>>> --- a/xen/arch/arm/Kconfig
>>>>>>>> +++ b/xen/arch/arm/Kconfig
>>>>>>>> @@ -78,6 +78,14 @@ config ARM_EFI
>>>>>>>>         UEFI firmware. A UEFI stub is provided to allow Xen to
>>>>>>>>         be booted as an EFI application.
>>>>>>>> 
>>>>>>>> +config GICV2
>>>>>>> So, now it would be possible to deselect both GIC drivers and Xen would 
>>>>>>> not complain when building.
>>>>>>> This means that Xen would fail on boot without any message as it 
>>>>>>> happens before serial driver initialization.
>>>>>>> Since having GIC driver built in is a must-have I think we need to make 
>>>>>>> sure that at least one is enabled.
>>>>>> 
>>>>>> Hi Michal,
>>>>>> 
>>>>>> I tried and I had:
>>>>>> 
>>>>>> Starting kernel ...
>>>>>> 
>>>>>> - UART enabled -
>>>>>> - Boot CPU booting -
>>>>>> - Current EL 0000000000000008 -
>>>>>> - Initialize CPU -
>>>>>> - Turning on paging -
>>>>>> - Zero BSS -
>>>>>> - Ready -
>>>>>> (XEN) Checking for initrd in /chosen
>>>>>> (XEN) RAM: 0000000080000000 - 00000000feffffff
>>>>>> (XEN) RAM: 0000000880000000 - 00000008ffffffff
>>>>>> (XEN)
>>>>>> (XEN) MODULE[0]: 0000000084000000 - 000000008415d000 Xen
>>>>>> (XEN) MODULE[1]: 00000000fd6c5000 - 00000000fd6c8000 Device Tree
>>>>>> (XEN) MODULE[2]: 0000000080080000 - 00000000814f1a00 Kernel
>>>>>> (XEN)  RESVD[0]: 0000000080000000 - 0000000080010000
>>>>>> (XEN)  RESVD[1]: 0000000018000000 - 00000000187fffff
>>>>>> (XEN)
>>>>>> (XEN)
>>>>>> (XEN) Command line: noreboot dom0_mem=1024M console=dtuart 
>>>>>> dtuart=serial0 bootscrub=0
>>>>>> (XEN) PFN compression on bits 20...22
>>>>>> (XEN) Domain heap initialised
>>>>>> (XEN) Booting using Device Tree
>>>>>> (XEN) Platform: Generic System
>>>>>> (XEN)
>>>>>> (XEN) ****************************************
>>>>>> (XEN) Panic on CPU 0:
>>>>>> (XEN) Unable to find compatible GIC in the device tree
>>>>>> (XEN) ****************************************
>>>>>> (XEN)
>>>>>> (XEN) Manual reset required ('noreboot' specified)
>>>>> Having early printk enabled all the time is not common and not enabled in 
>>>>> release builds FWIK.
>>> 
>>> There are a lot of information printed before the console is properly 
>>> brought up. I wonder if we should look at adding early console like Linux 
>>> does?
>>> 
>>>>> So in general, user would just see "Starting kernel" from u-boot and had 
>>>>> to debug what's going on.
>>>>> 
>>>>>> 
>>>>>> Wouldn’t be enough to suggest the user that at least one GIC needs to be 
>>>>>> selected? In the help it
>>>>>> also states “if unsure, say Y"
>>>>> I always think it is better to meet the users needs by preventing unwise 
>>>>> mistakes like unselecting both drivers.
>>>>> As always, it is up to maintainers.
>>>> Anyway I understand your point, do you think something like that could be 
>>>> ok? I’ve checked and it works, it
>>>> compile only if at least one GIC driver is enabled
>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index 264d2f2d4b09..85b4a7f08932 100644
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -1096,6 +1096,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>>>>        preinit_xen_time();
>>>>  +    /* Don't build if at least one GIC driver is enabled */
>>>> +    BUILD_BUG_ON(!(IS_ENABLED(CONFIG_GICV3) || IS_ENABLED(CONFIG_GICV2)
>>>> +                 || IS_ENABLED(CONFIG_NEW_VGIC)));
>>> randconfig in gitlab will now randomly fail compilation. If we want to 
>>> encode the dependency then it should be done in Kconfig. But I haven't 
>>> looked at how to do that.
>> Ok I’ll investigate it, apart from that, if I find a possible way to have 
>> that in Kconfig, do you have any objection on what this patch is doing and 
>> the approach taken?
> 
> Even if it is not possible, I wouldn't worry too much about it. While I agree 
> with Michal that it is not great to have nothing printed, Kconfig can only 
> reject configuration that are properly broken. But there are plenty that are 
> sound but would still not boot on some platform.
> 
> A pretty good example is someone may decide to disable GICv3 and try to boot 
> on a GICv3 platform... Another one is not enabling the UART driver for your 
> platform :).
> 
> The latter there is nothing we can do without earlyprintk. But for the 
> former, then we can try to enable the console earlier and/or delay when the 
> GIC is initialized.

:) Just found a way

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 5cdba07df964..93309cd49144 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -18,6 +18,7 @@ config ARM
        select HAS_PMAP
        select HAS_UBSAN
        select IOMMU_FORCE_PT_SHARE
+       select GICV2 if !GICV3 && !NEW_VGIC
 
 config ARCH_DEFCONFIG
        string

If I’ve played a bit with the menuconfig and it selects GICv2 if no other gic 
driver is selected, so basically
as before when gicv2 was always enabled.
If everyone agrees I can use this solution and include it in the next push


> 
> Cheers,
> 
> -- 
> Julien Grall