Re: [RFC PATCH 2/5] hw/intc/gicv3: add support for setting KVM vGIC maintenance IRQ

[Peter Maydell]

On Mon, 27 Feb 2023 at 16:37, Miguel Luis <miguel.luis@oracle.com> wrote:
>
> From: Haibo Xu <haibo.xu@linaro.org>
>
> Use the VGIC maintenance IRQ if VHE is requested. As per the ARM GIC
> Architecture Specification for GICv3 and GICv4 Arm strongly recommends that
> maintenance interrupts are configured to use INTID 25 matching the
> Server Base System Architecture (SBSA) recomendation.

What does this mean for QEMU, though? If the issue is
"KVM doesn't support the maintenance interrupt being anything
other than INTID 25" then we should say so (and have our code
error out if the board tries to use some other value). If the
issue is "the *host* has to be using the right INTID" then I
would hope that KVM simply doesn't expose the capability if
the host h/w won't let it work correctly. If KVM can happily
use any maintenance interrupt ID that the board model wants,
then we should make that work, rather than hardcoding 25 into
our gicv3 code.

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b871350856..377181e009 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -759,6 +759,11 @@ static void create_gic(VirtMachineState *vms, 
> MemoryRegion *mem)
>              qdev_prop_set_uint32(vms->gic, "redist-region-count[1]",
>                  MIN(smp_cpus - redist0_count, redist1_capacity));
>          }
> +
> +        if (kvm_irqchip_in_kernel()) {
> +            qdev_prop_set_bit(vms->gic, "has-virtualization-extensions",
> +                              vms->virt);
> +        }
>      } else {
>          if (!kvm_irqchip_in_kernel()) {
>              qdev_prop_set_bit(vms->gic, "has-virtualization-extensions",
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 351843db4a..e2a6ff1b49 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -563,6 +563,7 @@ static Property arm_gicv3_common_properties[] = {
>      DEFINE_PROP_UINT32("revision", GICv3State, revision, 3),
>      DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0),
>      DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 
> 0),
> +    DEFINE_PROP_BOOL("has-virtualization-extensions", GICv3State, virt_extn, 
> 0),
>      /*
>       * Compatibility property: force 8 bits of physical priority, even
>       * if the CPU being emulated should have fewer.
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 3ca643ecba..ce924753bb 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -22,6 +22,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/intc/arm_gicv3_common.h"
> +#include "hw/arm/virt.h"
>  #include "qemu/error-report.h"
>  #include "qemu/module.h"
>  #include "sysemu/kvm.h"
> @@ -803,6 +804,30 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, 
> Error **errp)
>          return;
>      }
>
> +
> +    if (s->virt_extn) {
> +        /*
> +         * Arm strongly recommends that maintenance interrupts are configured
> +         * to use INTID 25. For more information, see Server Base System
> +         * Architecture (SBSA)
> +         */
> +        uint32_t maint_irq = PPI(ARCH_GIC_MAINT_IRQ);
> +
> +        struct kvm_device_attr kdevattr = {
> +            .group = KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ,
> +            .addr = (uint64_t)&maint_irq
> +        };
> +
> +        if (!kvm_device_ioctl(s->dev_fd, KVM_GET_DEVICE_ATTR, &kdevattr)) {
> +            error_setg(errp, "VGICv3 setting maintenance IRQ are not "
> +                            "supported by this host kernel");
> +            return;
> +        }
> +
> +        kvm_device_ioctl(s->dev_fd, KVM_SET_DEVICE_ATTR, &kdevattr);
> +    }

So if I understand this correctly, the requirement here is basically
"tell the kernel which IRQ line is being used by the board code
for the maintenance interrupt", right? It seems to me that the
straightforward way to do that is to have an integer QOM property on
the GICv3 device like "maintenance-interrupt-id", and make the
board code set it (whether using KVM or not). The TCG implementation
doesn't care, and the KVM implementation can set it up in
kvm_arm_gicv3_realize(). Then you don't need to specifically tell
the GIC device that the guest is using the virtualization extensions.

thanks
-- PMM