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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

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.

Cheers,

-- 
Julien Grall