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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> 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.
> 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)));
     gic_preinit();
 
     arm_uart_init();

> 
> ~Michal