| Message ID | 20230802135350.745251-1-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] arm/gicv2: make GICv2 driver and vGICv2 optional | expand |
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@arm.com> > --- > 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. ~Michal
> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >> --- >> 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) 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" > > ~Michal
Hi, On 02/08/2023 16:42, Luca Fancellu wrote: > > >> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>> --- >>> 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. ~Michal
> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi, > > On 02/08/2023 16:42, Luca Fancellu wrote: >> >> >>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>> --- >>>> 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
Hi Luca, On 02/08/2023 16:05, Luca Fancellu wrote: >> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: >> >> Hi, >> >> On 02/08/2023 16:42, Luca Fancellu wrote: >>> >>> >>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>>> --- >>>>> 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. Cheers,
> On 2 Aug 2023, at 18:39, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 02/08/2023 16:05, Luca Fancellu wrote: >>> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: >>> >>> Hi, >>> >>> On 02/08/2023 16:42, Luca Fancellu wrote: >>>> >>>> >>>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>>>> --- >>>>>> 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? > > Cheers, > > -- > Julien Grall
Hi, On 02/08/2023 18:54, Luca Fancellu wrote: > > >> On 2 Aug 2023, at 18:39, Julien Grall <julien@xen.org> wrote: >> >> Hi Luca, >> >> On 02/08/2023 16:05, Luca Fancellu wrote: >>>> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: >>>> >>>> Hi, >>>> >>>> On 02/08/2023 16:42, Luca Fancellu wrote: >>>>> >>>>> >>>>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>>>>> --- >>>>>>> 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. Cheers,
> On 2 Aug 2023, at 19:39, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 02/08/2023 18:54, Luca Fancellu wrote: >>> On 2 Aug 2023, at 18:39, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Luca, >>> >>> On 02/08/2023 16:05, Luca Fancellu wrote: >>>>> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On 02/08/2023 16:42, Luca Fancellu wrote: >>>>>> >>>>>> >>>>>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>>>>>> --- >>>>>>>> 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
Hi Luca, On 02/08/2023 19:48, Luca Fancellu wrote: > 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 I am not thrilled with this approach. I understand that the default today is GICv2, but why should it continue to be like that? Even if we were switching to GICv3, it feels rather a odd approach because you would make it work for one set of the users but not the others. Cheers,
On 02/08/2023 19:39, Julien Grall wrote: > > > Hi Luca, > > On 02/08/2023 16:05, Luca Fancellu wrote: >>> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: >>> >>> Hi, >>> >>> On 02/08/2023 16:42, Luca Fancellu wrote: >>>> >>>> >>>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>>>> --- >>>>>> 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? I guess it is not a matter of "if" but "when" and I think it's just that no one has time to do that since it is more a like a feature for developers rather than for customers (they just report the issue and we need to fix it :)). There are so many things that can go wrong during early boot including the entire boofdt parsing and having earlycon would be so desirable. ~Michal
Hi, On 03/08/2023 08:15, Michal Orzel wrote: > On 02/08/2023 19:39, Julien Grall wrote: >> >> >> Hi Luca, >> >> On 02/08/2023 16:05, Luca Fancellu wrote: >>>> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: >>>> >>>> Hi, >>>> >>>> On 02/08/2023 16:42, Luca Fancellu wrote: >>>>> >>>>> >>>>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>>>>> --- >>>>>>> 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? > I guess it is not a matter of "if" but "when" and I think it's just that no one has time to do that > since it is more a like a feature for developers rather than for customers (they just report the issue > and we need to fix it :)). Sure. This is always the case, but it also means developpers will continue to waste time when investigating early boot issues. I wouldn't be surprised if the total time wasted is more than the actual effort to add support :). > There are so many things that can go wrong during early boot including the entire boofdt parsing > and having earlycon would be so desirable. It is not clear what to take from your reply. Earlier you were concerned about the error not showing up in the log if the .config is not correct. There is no really quick fix for that as a .config may work for platform A but not platform B. The only viable solution is having an early console. Anything else like forcing to always have one GICvX selected is just a hack that would work for one set of people but not the others. Cheers,
>>>>>> >>>>>> - 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) Hi Julien, Michal, >>>>> 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? >> I guess it is not a matter of "if" but "when" and I think it's just that no one has time to do that >> since it is more a like a feature for developers rather than for customers (they just report the issue >> and we need to fix it :)). > > Sure. This is always the case, but it also means developpers will continue to waste time when investigating early boot issues. I wouldn't be surprised if the total time wasted is more than the actual effort to add support :). I’ve not investigated on the amount of time needed to add that, I wouldn’t underestimate that and unfortunately people have different capacity and companies have different priorities, so a quick feature can translate on months when the time allocated for a particular job is, let’s say, one day a month. > >> There are so many things that can go wrong during early boot including the entire boofdt parsing >> and having earlycon would be so desirable. > It is not clear what to take from your reply. Earlier you were concerned about the error not showing up in the log if the .config is not correct. > > There is no really quick fix for that as a .config may work for platform A but not platform B. The only viable solution is having an early console. > > Anything else like forcing to always have one GICvX selected is just a hack that would work for one set of people but not the others. I agree with you in part, when you say that it would work for one set of people but not the others, isn’t it always the case? We are providing a defconfig that would fit the majority of the people, but it’s always a set of them. More specific to this patch, with the provided Kconfig “hack” as you say, it is not only providing the same user experience as the current state, it is doing more, it is providing a way to exclude from the build the GICv2 which is not possible currently, this work aims to provide a more fine granular configuration so that experienced users can remove entire modules that they don’t need in their platform so that they don’t have to take them into account when going to, for example, safety critical audits. I agree with you that earlycon would be perfect, so that we could make the user remove every module and let him know quickly the issue, but on the other hand it is a complete new feature that you are requesting (are you?) to make this patch go forward. My personal opinion is that, since this patch is not affecting the user experience, it is maintaining the current status (not ideal), but at least It is providing something more, it would be a shame to throw that in the bin because no one has time to work on earlycon. Cheers, Luca
On 03/08/2023 09:35, Julien Grall wrote: > > > Hi, > > On 03/08/2023 08:15, Michal Orzel wrote: >> On 02/08/2023 19:39, Julien Grall wrote: >>> >>> >>> Hi Luca, >>> >>> On 02/08/2023 16:05, Luca Fancellu wrote: >>>>> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On 02/08/2023 16:42, Luca Fancellu wrote: >>>>>> >>>>>> >>>>>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>>>>>> --- >>>>>>>> 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? >> I guess it is not a matter of "if" but "when" and I think it's just that no one has time to do that >> since it is more a like a feature for developers rather than for customers (they just report the issue >> and we need to fix it :)). > > Sure. This is always the case, but it also means developpers will > continue to waste time when investigating early boot issues. I wouldn't > be surprised if the total time wasted is more than the actual effort to > add support :). most probably > >> There are so many things that can go wrong during early boot including the entire boofdt parsing >> and having earlycon would be so desirable. > It is not clear what to take from your reply. Earlier you were concerned > about the error not showing up in the log if the .config is not correct. > > There is no really quick fix for that as a .config may work for platform > A but not platform B. The only viable solution is having an early console. > > Anything else like forcing to always have one GICvX selected is just a > hack that would work for one set of people but not the others. Yes, having early console would solve these issues and for now we might need to be on the mercy of users to do wise selection. That said, the scenario with GIC is not really the same as for something that can work on platform A and not on B. No GIC built-in cannot work on any platform, hence my concern (it is different than selecting wrong GIC). But because I do not see any simple solution other than building always both and you don't like what Luca just did (fallback to GICv2), I'm ok. Initializing UART prior to GIC would also require quite some work to defer the IRQ translation that depends on the GIC. ~Michal
Hi Michal, On 03/08/2023 09:07, Michal Orzel wrote: > On 03/08/2023 09:35, Julien Grall wrote: >> On 03/08/2023 08:15, Michal Orzel wrote: >>> On 02/08/2023 19:39, Julien Grall wrote: >>>> >>>> >>>> Hi Luca, >>>> >>>> On 02/08/2023 16:05, Luca Fancellu wrote: >>>>>> On 2 Aug 2023, at 15:48, Michal Orzel <michal.orzel@amd.com> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 02/08/2023 16:42, Luca Fancellu wrote: >>>>>>> >>>>>>> >>>>>>>> On 2 Aug 2023, at 15:26, Michal Orzel <michal.orzel@amd.com> 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@arm.com> >>>>>>>>> --- >>>>>>>>> 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? >>> I guess it is not a matter of "if" but "when" and I think it's just that no one has time to do that >>> since it is more a like a feature for developers rather than for customers (they just report the issue >>> and we need to fix it :)). >> >> Sure. This is always the case, but it also means developpers will >> continue to waste time when investigating early boot issues. I wouldn't >> be surprised if the total time wasted is more than the actual effort to >> add support :). > most probably > >> >>> There are so many things that can go wrong during early boot including the entire boofdt parsing >>> and having earlycon would be so desirable. >> It is not clear what to take from your reply. Earlier you were concerned >> about the error not showing up in the log if the .config is not correct. >> >> There is no really quick fix for that as a .config may work for platform >> A but not platform B. The only viable solution is having an early console. >> >> Anything else like forcing to always have one GICvX selected is just a >> hack that would work for one set of people but not the others. > Yes, having early console would solve these issues and for now we might need to be > on the mercy of users to do wise selection. That said, the scenario with GIC is not really > the same as for something that can work on platform A and not on B. No GIC built-in > cannot work on any platform, hence my concern (it is different than selecting wrong GIC). The assumption behind this is the user will always have a platform where it can boot Xen. I am not aware of many users with this opportunity of choice. So to me this is not very different from Xen can't boot on A but can on B. Cheers,
On 03/08/2023 09:04, Luca Fancellu wrote: >>> There are so many things that can go wrong during early boot including the entire boofdt parsing >>> and having earlycon would be so desirable. >> It is not clear what to take from your reply. Earlier you were concerned about the error not showing up in the log if the .config is not correct. >> >> There is no really quick fix for that as a .config may work for platform A but not platform B. The only viable solution is having an early console. >> >> Anything else like forcing to always have one GICvX selected is just a hack that would work for one set of people but not the others. > > I agree with you in part, when you say that it would work for one set of people but not the others, isn’t it always the case? We are providing > a defconfig that would fit the majority of the people, but it’s always a set of them. Correct. > > More specific to this patch, with the provided Kconfig “hack” as you say, it is not only providing the same user experience as the current state, > it is doing more, it is providing a way to exclude from the build the GICv2 which is not possible currently, this work aims to provide a more > fine granular configuration so that experienced users can remove entire modules that they don’t need in their platform so that they don’t have > to take them into account when going to, for example, safety critical audits. There seems to be some misunderstanding about the hack I am referring to. This is not about the patch itself but the following line: select GICV2 if !GICV3 && !NEW_VGIC Now that GICv2 can be deselecting, I see no point to force select GICV2 if GICV3 is not selected. As I wrote earlier, there is so many way for someone to create .config that is wrong or doesn't boot on their platform. So someone tweaking .config should always be careful when selecting/deselecting options. > > I agree with you that earlycon would be perfect, so that we could make the user remove every module and let him know quickly the issue, > but on the other hand it is a complete new feature that you are requesting (are you?) to make this patch go forward. I didn't suggest it. I was making the point that if you have time, it is best to spend it trying to enable the early console rather than trying to prevent the user to disable both GIC. Cheers,
> On 3 Aug 2023, at 09:29, Julien Grall <julien@xen.org> wrote: > > > > On 03/08/2023 09:04, Luca Fancellu wrote: >>>> There are so many things that can go wrong during early boot including the entire boofdt parsing >>>> and having earlycon would be so desirable. >>> It is not clear what to take from your reply. Earlier you were concerned about the error not showing up in the log if the .config is not correct. >>> >>> There is no really quick fix for that as a .config may work for platform A but not platform B. The only viable solution is having an early console. >>> >>> Anything else like forcing to always have one GICvX selected is just a hack that would work for one set of people but not the others. >> I agree with you in part, when you say that it would work for one set of people but not the others, isn’t it always the case? We are providing >> a defconfig that would fit the majority of the people, but it’s always a set of them. > > Correct. > >> More specific to this patch, with the provided Kconfig “hack” as you say, it is not only providing the same user experience as the current state, >> it is doing more, it is providing a way to exclude from the build the GICv2 which is not possible currently, this work aims to provide a more >> fine granular configuration so that experienced users can remove entire modules that they don’t need in their platform so that they don’t have >> to take them into account when going to, for example, safety critical audits. > > There seems to be some misunderstanding about the hack I am referring to. This is not about the patch itself but the following line: > > select GICV2 if !GICV3 && !NEW_VGIC > > Now that GICv2 can be deselecting, I see no point to force select GICV2 if GICV3 is not selected. Ok I see > > As I wrote earlier, there is so many way for someone to create .config that is wrong or doesn't boot on their platform. So someone tweaking .config should always be careful when selecting/deselecting options. Yes indeed > >> I agree with you that earlycon would be perfect, so that we could make the user remove every module and let him know quickly the issue, >> but on the other hand it is a complete new feature that you are requesting (are you?) to make this patch go forward. > > I didn't suggest it. I was making the point that if you have time, it is best to spend it trying to enable the early console rather than trying to prevent the user to disable both GIC. It will be for sure something I will investigate, but in the mean time unfortunately I won’t have time at least until October to even start an investigation. I would like to ask if you see this one going forwards or not (as it is), because I have a set of patches to isolate and Kconfig-out the dom0less code, that is depending on this one (for a small bit) and before sending them I need to understand if this one can see the light or not. > > Cheers, > > -- > Julien Grall
Hi, On 03/08/2023 09:39, Luca Fancellu wrote: > I would like to ask if you see this one going forwards or not (as it is), because I have a set of patches to isolate and Kconfig-out > the dom0less code, that is depending on this one (for a small bit) and before sending them I need to understand if this one can > see the light or not. In principle, I have no issues with trying to disable some part of Xen code. I will still need to look at each one to confirm I am happy with the changes. For this case, I will have a look in a bit. Cheers,
Hi, On 02/08/2023 14: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@arm.com> > --- > 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 > + bool "GICv2 driver" > + default y > + select VGICV2 > + help > + Driver for the ARM Generic Interrupt Controller v2. > + If unsure, say Y > + > config GICV3 > bool "GICv3 driver" > depends on !NEW_VGIC > @@ -92,6 +100,11 @@ config HAS_ITS > bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED > depends on GICV3 && !NEW_VGIC && !ARM_32 > > +config VGICV2 > + bool "vGICv2 interface for guests" This description is a bit misleading as the vGICv2 will also be used for dom0 in the case of vGICv2. > + default y Please add a longer help. > + depends on (GICV2 || GICV3) && !NEW_VGIC In the near future, I don't expect anyone to introduce a new non-GIC of interrupt controller for Arm. But I would expect new version of the GIC. So I would drop (GICV2 || GICV3). Also when !NEW_VGIC is selected, this will make VGICV2 will be unselected. I was actually expecting the other way around given that new vGIC only offer v2 support. The rest of the changes LGTM. Cheers,
Hi Julien, >> + >> config GICV3 >> bool "GICv3 driver" >> depends on !NEW_VGIC >> @@ -92,6 +100,11 @@ config HAS_ITS >> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED >> depends on GICV3 && !NEW_VGIC && !ARM_32 >> +config VGICV2 >> + bool "vGICv2 interface for guests" > > This description is a bit misleading as the vGICv2 will also be used for dom0 in the case of vGICv2. > >> + default y > > Please add a longer help. > >> + depends on (GICV2 || GICV3) && !NEW_VGIC > > In the near future, I don't expect anyone to introduce a new non-GIC of interrupt controller for Arm. But I would expect new version of the GIC. So I would drop (GICV2 || GICV3). > > Also when !NEW_VGIC is selected, this will make VGICV2 will be unselected. I was actually expecting the other way around given that new vGIC only offer v2 support. > > The rest of the changes LGTM. Thanks a lot for having a look on this patch, you are right the NEW_VGIC is offering only v2 support at the moment, does this changes captures your Comments? diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 5cdba07df964..1c600b3b8d04 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -110,15 +110,19 @@ config HAS_ITS depends on GICV3 && !NEW_VGIC && !ARM_32 config VGICV2 - bool "vGICv2 interface for guests" + bool "vGICv2 interface for domains" default y - depends on (GICV2 || GICV3) && !NEW_VGIC + help + Provides a virtualised interface for the Generic Interrupt Controller that + can be used by Xen's domains. + If unsure, say Y config HVM def_bool y config NEW_VGIC bool "Use new VGIC implementation" + select VGICV2 ---help--- This is an alternative implementation of the ARM GIC interrupt diff --git a/xen/arch/arm/vgic/Makefile b/xen/arch/arm/vgic/Makefile index 806826948e20..60cbf7f2f94a 100644 --- a/xen/arch/arm/vgic/Makefile +++ b/xen/arch/arm/vgic/Makefile @@ -1,5 +1,5 @@ obj-y += vgic.o -obj-y += vgic-v2.o +obj-$(CONFIG_VGICV2) += vgic-v2.o obj-y += vgic-mmio.o -obj-y += vgic-mmio-v2.o +obj-$(CONFIG_VGICV2) += vgic-mmio-v2.o obj-y += vgic-init.o > > Cheers, > > -- > Julien Grall
> On 3 Aug 2023, at 14:03, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > > Hi Julien, > >>> + >>> config GICV3 >>> bool "GICv3 driver" >>> depends on !NEW_VGIC >>> @@ -92,6 +100,11 @@ config HAS_ITS >>> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED >>> depends on GICV3 && !NEW_VGIC && !ARM_32 >>> +config VGICV2 >>> + bool "vGICv2 interface for guests" >> >> This description is a bit misleading as the vGICv2 will also be used for dom0 in the case of vGICv2. >> >>> + default y >> >> Please add a longer help. >> >>> + depends on (GICV2 || GICV3) && !NEW_VGIC >> >> In the near future, I don't expect anyone to introduce a new non-GIC of interrupt controller for Arm. But I would expect new version of the GIC. So I would drop (GICV2 || GICV3). >> >> Also when !NEW_VGIC is selected, this will make VGICV2 will be unselected. I was actually expecting the other way around given that new vGIC only offer v2 support. >> >> The rest of the changes LGTM. > > Thanks a lot for having a look on this patch, you are right the NEW_VGIC is offering only v2 support at the moment, does this changes captures your > Comments? > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index 5cdba07df964..1c600b3b8d04 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -110,15 +110,19 @@ config HAS_ITS > depends on GICV3 && !NEW_VGIC && !ARM_32 > > config VGICV2 > - bool "vGICv2 interface for guests" > + bool "vGICv2 interface for domains" > default y > - depends on (GICV2 || GICV3) && !NEW_VGIC > + help > + Provides a virtualised interface for the Generic Interrupt Controller that > + can be used by Xen's domains. > + If unsure, say Y > > config HVM > def_bool y > > config NEW_VGIC > bool "Use new VGIC implementation" > + select VGICV2 Sorry, I meant to use here “select GICV2" > ---help--- > > This is an alternative implementation of the ARM GIC interrupt > diff --git a/xen/arch/arm/vgic/Makefile b/xen/arch/arm/vgic/Makefile > index 806826948e20..60cbf7f2f94a 100644 > --- a/xen/arch/arm/vgic/Makefile > +++ b/xen/arch/arm/vgic/Makefile > @@ -1,5 +1,5 @@ > obj-y += vgic.o > -obj-y += vgic-v2.o > +obj-$(CONFIG_VGICV2) += vgic-v2.o > obj-y += vgic-mmio.o > -obj-y += vgic-mmio-v2.o > +obj-$(CONFIG_VGICV2) += vgic-mmio-v2.o > obj-y += vgic-init.o > > > >> >> Cheers, >> >> -- >> Julien Grall
On 03/08/2023 14:03, Luca Fancellu wrote: > Hi Julien, Hi, >>> + >>> config GICV3 >>> bool "GICv3 driver" >>> depends on !NEW_VGIC >>> @@ -92,6 +100,11 @@ config HAS_ITS >>> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED >>> depends on GICV3 && !NEW_VGIC && !ARM_32 >>> +config VGICV2 >>> + bool "vGICv2 interface for guests" >> >> This description is a bit misleading as the vGICv2 will also be used for dom0 in the case of vGICv2. >> >>> + default y >> >> Please add a longer help. >> >>> + depends on (GICV2 || GICV3) && !NEW_VGIC >> >> In the near future, I don't expect anyone to introduce a new non-GIC of interrupt controller for Arm. But I would expect new version of the GIC. So I would drop (GICV2 || GICV3). >> >> Also when !NEW_VGIC is selected, this will make VGICV2 will be unselected. I was actually expecting the other way around given that new vGIC only offer v2 support. >> >> The rest of the changes LGTM. > > Thanks a lot for having a look on this patch, you are right the NEW_VGIC is offering only v2 support at the moment, does this changes captures your > Comments? > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index 5cdba07df964..1c600b3b8d04 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -110,15 +110,19 @@ config HAS_ITS > depends on GICV3 && !NEW_VGIC && !ARM_32 > > config VGICV2 > - bool "vGICv2 interface for guests" > + bool "vGICv2 interface for domains" > default y > - depends on (GICV2 || GICV3) && !NEW_VGIC > + help > + Provides a virtualised interface for the Generic Interrupt Controller that > + can be used by Xen's domains. How about: Allow Xen to expose a Generic Interrupt Controller version 2 like to Xen domains. This can be configured at the domain creation. This option is mandatory when using GICv2. For GICv3, this allows domain to use GICv2 when the hardware supports it. If unsure say Y. > + If unsure, say Y > > config HVM > def_bool y > > config NEW_VGIC > bool "Use new VGIC implementation" > + select VGICV2 > ---help--- > > This is an alternative implementation of the ARM GIC interrupt > diff --git a/xen/arch/arm/vgic/Makefile b/xen/arch/arm/vgic/Makefile > index 806826948e20..60cbf7f2f94a 100644 > --- a/xen/arch/arm/vgic/Makefile > +++ b/xen/arch/arm/vgic/Makefile > @@ -1,5 +1,5 @@ > obj-y += vgic.o > -obj-y += vgic-v2.o > +obj-$(CONFIG_VGICV2) += vgic-v2.o > obj-y += vgic-mmio.o > -obj-y += vgic-mmio-v2.o > +obj-$(CONFIG_VGICV2) += vgic-mmio-v2.o > obj-y += vgic-init.o > > > >> >> Cheers, >> >> -- >> Julien Grall >
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 + bool "GICv2 driver" + default y + select VGICV2 + help + Driver for the ARM Generic Interrupt Controller v2. + If unsure, say Y + config GICV3 bool "GICv3 driver" depends on !NEW_VGIC @@ -92,6 +100,11 @@ config HAS_ITS bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED depends on GICV3 && !NEW_VGIC && !ARM_32 +config VGICV2 + bool "vGICv2 interface for guests" + default y + depends on (GICV2 || GICV3) && !NEW_VGIC + config HVM def_bool y diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 7bf07e992046..81c31c36fc3d 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -22,7 +22,7 @@ obj-y += domctl.o obj-$(CONFIG_EARLY_PRINTK) += early_printk.o obj-y += efi/ obj-y += gic.o -obj-y += gic-v2.o +obj-$(CONFIG_GICV2) += gic-v2.o obj-$(CONFIG_GICV3) += gic-v3.o obj-$(CONFIG_HAS_ITS) += gic-v3-its.o obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o @@ -57,7 +57,7 @@ obj-$(CONFIG_NEW_VGIC) += vgic/ ifneq ($(CONFIG_NEW_VGIC),y) obj-y += gic-vgic.o obj-y += vgic.o -obj-y += vgic-v2.o +obj-$(CONFIG_VGICV2) += vgic-v2.o obj-$(CONFIG_GICV3) += vgic-v3.o obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o endif diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 39b4ee03a505..b2b609eb0c2d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2775,6 +2775,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, return res; } +#ifdef CONFIG_VGICV2 static int __init make_gicv2_domU_node(struct kernel_info *kinfo) { void *fdt = kinfo->fdt; @@ -2826,6 +2827,7 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) return res; } +#endif #ifdef CONFIG_GICV3 static int __init make_gicv3_domU_node(struct kernel_info *kinfo) @@ -2901,8 +2903,10 @@ static int __init make_gic_domU_node(struct kernel_info *kinfo) case GIC_V3: return make_gicv3_domU_node(kinfo); #endif +#ifdef CONFIG_VGICV2 case GIC_V2: return make_gicv2_domU_node(kinfo); +#endif default: panic("Unsupported GIC version\n"); } diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 95e4f020febe..d18a3317ccc4 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1334,6 +1334,7 @@ static paddr_t __initdata dbase = INVALID_PADDR; static paddr_t __initdata vbase = INVALID_PADDR, vsize = 0; static paddr_t __initdata cbase = INVALID_PADDR, csize = 0; +#ifdef CONFIG_VGICV2 /* If the GICv3 supports GICv2, initialize it */ static void __init gicv3_init_v2(void) { @@ -1359,6 +1360,9 @@ static void __init gicv3_init_v2(void) vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0); } +#else +static inline void gicv3_init_v2(void) { } +#endif static void __init gicv3_ioremap_distributor(paddr_t dist_paddr) { diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 97d6f6106638..86fa8bc7e894 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -95,10 +95,12 @@ int domain_vgic_register(struct domain *d, int *mmio_count) return -ENODEV; break; #endif +#ifdef CONFIG_VGICV2 case GIC_V2: if ( vgic_v2_init(d, mmio_count) ) return -ENODEV; break; +#endif default: printk(XENLOG_G_ERR "d%d: Unknown vGIC version %u\n", d->domain_id, d->arch.vgic.version);
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@arm.com> --- 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(-)