Re: [PATCH v2] xen/arm: fix the build error for GIC on ARM64 QEMU Platform

[Dongjiu Geng <gengdongjiu1@xxxxxxxxx>]

Julien Grall <julien.grall.oss@xxxxxxxxx> 于2021年12月28日周二 18:27写道：
>
> Hello,
>
> On Sun, 26 Dec 2021 at 11:07, Dongjiu Geng <gengdongjiu1@xxxxxxxxx> wrote:
> >
> > Julien Grall <julien@xxxxxxx> 于2021年12月24日周五 22:21写道：
> > >
> > > Hi,
> > >
> > > I am not sure why you dropped the others. I have added them back.
> >
> > sorry, it is my mistake
> >
> > >
> > > On 24/12/2021 14:36, Dongjiu Geng wrote:
> > > > Julien Grall <julien@xxxxxxx> 于2021年12月24日周五 21:25写道：
> > > >>
> > > >> Hi,
> > > >>
> > > >> On 24/12/2021 13:24, Dongjiu Geng wrote:
> > > >>> when enable CONFIG_NEW_VGIC in ARM64 QEMU Platform, it will build 
> > > >>> failed.
> > > >>> so fix it and make it can select GICV2.
> > > >>
> > > >> As I said in v1, last time I checked QEMU was only able to support
> > > >> virtualization with GICv3. This is why we added a depends on.
> > > >
> > > > I enabled CONFIG_NEW_VGIC, then it will use GICv2. In my check, it
> > > > does not report an error.
> > > > My QEMU emulator version is 4.0.0.  What is the QEMU version that you 
> > > > used?
> > >
> > > I am using a more recent QEMU. However, I have always only used GICv3
> > > becuase it was IIRC there first.
> > >
> > > >
> > > >>
> > > >> If you want to remove it, then I think you ought to explain in the
> > > >> commit message why this is fine. A pointer to the commit or a QEMU
> > > >> version used would be useful.
> > > >
> > > > OK，thanks. Can you check if QEMU 4.0.0 is workable with GICv2 on your 
> > > > side?
> > >
> > > I don't have direct access to my QEMU setup at the moment. However,
> > > looking at the history.
> > >
> > > So I would suggest the following commit message:
> > >
> > > "
> > > xen/arm: Allow QEMU platform to be built with GICv2
> > >
> > > Trying to select PLATFORM_QEMU with NEW_VGIC will result to Kconfig
> > > complain about unmet dependencies:
> > >
> > > tools/kconfig/conf  --syncconfig Kconfig
> > >
> > > WARNING: unmet direct dependencies detected for GICV3
> > >    Depends on [n]: ARM_64 [=y] && !NEW_VGIC [=y]
> > >    Selected by [y]:
> > >    - QEMU [=y] && <choice> && ARM_64 [=y]
> > >
> > > WARNING: unmet direct dependencies detected for GICV3
> > >    Depends on [n]: ARM_64 [=y] && !NEW_VGIC [=y]
> > >    Selected by [y]:
> > >    - QEMU [=y] && <choice> && ARM_64 [=y]
> > >
> > > WARNING: unmet direct dependencies detected for GICV3
> > >    Depends on [n]: ARM_64 [=y] && !NEW_VGIC [=y]
> > >    Selected by [y]:
> > >    - QEMU [=y] && <choice> && ARM_64 [=y]
> > >
> > >
> > > It turns out that QEMU has been supporting GICv2 virtualization since
> > > v3.1.0. So an easy way to solve the issue and allow more custom support
> > > is to remove the dependencies on GICv3.
> > > "
> >
> > Ok, thanks very much for your suggestion.
> >
> > >
> > > > It is workable on my side.
> > > >
> > > >
> > > >>
> > > >>>
> > > >>> Signed-off-by: Dongjiu Geng <gengdongjiu1@xxxxxxxxx>
> > > >>> ---
> > > >>> $ make dist-xen XEN_TARGET_ARCH=arm64 
> > > >>> CROSS_COMPILE=aarch64-linux-gnu- -j10
> > > >>> make -C xen install
> > > >>> make[1]: Entering directory 
> > > >>> '/home/workspace/gengdongjiu/old_machine/XEN/xen/xen'
> > > >>> make -f 
> > > >>> /home/workspace/gengdongjiu/old_machine/XEN/xen/xen/tools/kconfig/Makefile.kconfig
> > > >>>  ARCH=arm64 SRCARCH=arm HOSTCC="gcc" HOSTCXX="g++" syncconfig
> > > >>> make[2]: Entering directory 
> > > >>> '/home/workspace/gengdongjiu/old_machine/XEN/xen/xen'
> > > >>> gcc -Wp,-MD,tools/kconfig/.conf.o.d      -c -o tools/kconfig/conf.o 
> > > >>> tools/kconfig/conf.c
> > > >>> gcc -Wp,-MD,tools/kconfig/.confdata.o.d      -c -o 
> > > >>> tools/kconfig/confdata.o tools/kconfig/confdata.c
> > > >>> gcc -Wp,-MD,tools/kconfig/.expr.o.d      -c -o tools/kconfig/expr.o 
> > > >>> tools/kconfig/expr.c
> > > >>> flex -otools/kconfig/lexer.lex.c -L tools/kconfig/lexer.l
> > > >>> bison -o tools/kconfig/parser.tab.c 
> > > >>> --defines=tools/kconfig/parser.tab.h -t -l tools/kconfig/parser.y
> > > >>> gcc -Wp,-MD,tools/kconfig/.preprocess.o.d      -c -o 
> > > >>> tools/kconfig/preprocess.o tools/kconfig/preprocess.c
> > > >>> gcc -Wp,-MD,tools/kconfig/.symbol.o.d      -c -o 
> > > >>> tools/kconfig/symbol.o tools/kconfig/symbol.c
> > > >>> gcc -Wp,-MD,tools/kconfig/.lexer.lex.o.d     -I 
> > > >>> /home/workspace/gengdongjiu/old_machine/XEN/xen/xen/tools/kconfig -c 
> > > >>> -o tools/kconfig/lexer.lex.o tools/kconfig/lexer.lex.c
> > > >>> gcc -Wp,-MD,tools/kconfig/.parser.tab.o.d     -I 
> > > >>> /home/workspace/gengdongjiu/old_machine/XEN/xen/xen/tools/kconfig -c 
> > > >>> -o tools/kconfig/parser.tab.o tools/kconfig/parser.tab.c
> > > >>> gcc  -o tools/kconfig/conf tools/kconfig/conf.o 
> > > >>> tools/kconfig/confdata.o tools/kconfig/expr.o 
> > > >>> tools/kconfig/lexer.lex.o tools/kconfig/parser.tab.o 
> > > >>> tools/kconfig/preprocess.o tools/kconfig/symbol.o
> > > >>> tools/kconfig/conf  --syncconfig Kconfig
> > > >>>
> > > >>> WARNING: unmet direct dependencies detected for GICV3
> > > >>>     Depends on [n]: ARM_64 [=y] && !NEW_VGIC [=y]
> > > >>>     Selected by [y]:
> > > >>>     - QEMU [=y] && <choice> && ARM_64 [=y]
> > > >>>
> > > >>> WARNING: unmet direct dependencies detected for GICV3
> > > >>>     Depends on [n]: ARM_64 [=y] && !NEW_VGIC [=y]
> > > >>>     Selected by [y]:
> > > >>>     - QEMU [=y] && <choice> && ARM_64 [=y]
> > > >>>
> > > >>> WARNING: unmet direct dependencies detected for GICV3
> > > >>>     Depends on [n]: ARM_64 [=y] && !NEW_VGIC [=y]
> > > >>>     Selected by [y]:
> > > >>>     - QEMU [=y] && <choice> && ARM_64 [=y]
> > > >>> make[2]: Leaving directory 
> > > >>> '/home/workspace/gengdongjiu/old_machine/XEN/xen/xen'
> > > >>> make -f Rules.mk _install
> > > >>> make[2]: Entering directory 
> > > >>> '/home/workspace/gengdongjiu/old_machine/XEN/xen/xen'
> > > >>> ---
> > > >>>    xen/arch/arm/Kconfig           | 5 +++--
> > > >>>    xen/arch/arm/platforms/Kconfig | 1 -
> > > >>>    2 files changed, 3 insertions(+), 3 deletions(-)
> > > >>>
> > > >>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > >>> index ecfa6822e4..373c698018 100644
> > > >>> --- a/xen/arch/arm/Kconfig
> > > >>> +++ b/xen/arch/arm/Kconfig
> > > >>
> > > >> The change in arch/arm/Kconfig is not really related to this patch.
> > > >> Technically the part in platforms/Kconfig is sufficient. I still think
> > > >> the change is good to have but it should be in a separate patch.
> > > >
> > > > sure
> > > >
> > > >>
> > > >>> @@ -35,7 +35,7 @@ config ACPI
> > > >>>
> > > >>>    config GICV3
> > > >>>        bool "GICv3 driver"
> > > >>> -     depends on ARM_64 && !NEW_VGIC
> > > >>> +     depends on ARM_64
> > > >>>        default y
> > > >>>        ---help---
> > > >>>
> > > >>> @@ -44,13 +44,14 @@ config GICV3
> > > >>>
> > > >>>    config HAS_ITS
> > > >>>            bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if 
> > > >>> UNSUPPORTED
> > > >>> -        depends on GICV3 && !NEW_VGIC
> > > >>> +        depends on GICV3
> > > >>>
> > > >>>    config HVM
> > > >>>            def_bool y
> > > >>>
> > > >>>    config NEW_VGIC
> > > >>>        bool "Use new VGIC implementation"
> > > >>> +     depends on !GICV3
> > > >>>        ---help---
> > > >>>
> > > >>>        This is an alternative implementation of the ARM GIC interrupt
> > > >>> diff --git a/xen/arch/arm/platforms/Kconfig 
> > > >>> b/xen/arch/arm/platforms/Kconfig
> > > >>> index c93a6b2756..6d3ee99456 100644
> > > >>> --- a/xen/arch/arm/platforms/Kconfig
> > > >>> +++ b/xen/arch/arm/platforms/Kconfig
> > > >>> @@ -15,7 +15,6 @@ config ALL_PLAT
> > > >>>    config QEMU
> > > >>>        bool "QEMU aarch virt machine support"
> > > >>>        depends on ARM_64
> > > >>> -     select GICV3
> > > >>>        select HAS_PL011
> > > >>>        ---help---
> > > >>>        Enable all the required drivers for QEMU aarch64 virt emulated
> > >
> > > While writing a proposal for the commit message, I remembered that the
> > > goal of CONFIG_QEMU was to select all the required drivers to be able to
> > > boot Xen on QEMU.
> > >
> > > AFAICT, if you start from tiny64_defconfig, you would not have GICv3
> > > selected. So we would technically break users of QEMU with GICv3.
> >
> > I think I can add CONFIG_GICV3=y in arch/arm/configs/tiny64_defconfig
> > to this patch, if it must to use GICV3.
>
> Please don't update tiny64_defconfig. The goal of tiny64_defconfig is to
> have a config with only the bare-minimum. If add CONFIG_GICV3=y, then it will
> be enabled when it is not necessary.

Ok，we can let the user configure the CONFIG_GICV3 in tiny64_defconfig
according his neccesary.
So I think my modification does not affect the tiny64_defconfig. what
is your concern about it?

>
> Cheers,