Re: [PATCH v3] xen/arm: Allow QEMU platform to be built with GICv2

[Dongjiu Geng <gengdongjiu1@xxxxxxxxx>]

Julien Grall <julien.grall.oss@xxxxxxxxx> 于2021年12月28日周二 18:39写道：
>
> Hi,
>
> On Tue, 28 Dec 2021 at 05:14, Dongjiu Geng <gengdongjiu1@xxxxxxxxx> wrote:
> >
> > 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.
>
> You took my proposed commit message but the diff in
> this version doesn't match it.
>
> >
> > Signed-off-by: Dongjiu Geng <gengdongjiu1@xxxxxxxxx>
> > ---
> > change since v1:
> > 1. Address Stefano's comments to add dependency
> >
> > change since v2:
> > 1. Address Julien's comments to update the commit messages.
> > 2. enable GICV3 in arch/arm/configs/tiny64_defconfig
> > ---
> >  xen/arch/arm/Kconfig                  | 5 +++--
> >  xen/arch/arm/configs/tiny64_defconfig | 2 +-
> >  xen/arch/arm/platforms/Kconfig        | 2 +-
> >  3 files changed, 5 insertions(+), 4 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
>
> Are the changes necessary in arch/arm/Kconfig to solve the issue on QEMU?
> If not, then they should be in a separate patch.
> If yes, then they ought to be explained in the commit message.
>
> > @@ -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/configs/tiny64_defconfig 
> > b/xen/arch/arm/configs/tiny64_defconfig
> > index cc6d93f2f8..165603f7e0 100644
> > --- a/xen/arch/arm/configs/tiny64_defconfig
> > +++ b/xen/arch/arm/configs/tiny64_defconfig
> > @@ -4,7 +4,7 @@ CONFIG_ARM=y
> >  #
> >  # Architecture Features
> >  #
> > -# CONFIG_GICV3 is not set
> > +CONFIG_GICV3=y
>
> The goal of tiny64_defconfig is to have nothing enabled by default.
> So we should not enable GICV3 here.
>
> >  # CONFIG_MEM_ACCESS is not set
> >  # CONFIG_SBSA_VUART_CONSOLE is not set
> >
> > diff --git a/xen/arch/arm/platforms/Kconfig b/xen/arch/arm/platforms/Kconfig
> > index c93a6b2756..925f6ef8da 100644
> > --- a/xen/arch/arm/platforms/Kconfig
> > +++ b/xen/arch/arm/platforms/Kconfig
> > @@ -15,7 +15,7 @@ config ALL_PLAT
> >  config QEMU
> >         bool "QEMU aarch virt machine support"
> >         depends on ARM_64
> > -       select GICV3
> > +       select GICv3 if !NEW_VGIC
>
> There was an open question in v2. In general, it is better to wait
> until the discussion is closed or you mention it after ---. This
> avoids being lost.

Sorry for the noise, Stefano,  do you have any suggestion for this
patch?  thanks a lot.

>
> Cheers,
>
> --
> Julien Grall