Re: [PATCH RESEND v2 4/6] hw/intc/arm_gicv3: Enable support for setting vGIC maintenance IRQ

[Andrew Jones <drjones@redhat.com>]

On Thu, Apr 01, 2021 at 05:55:36AM -0700, Haibo Xu wrote:
> Using the new VGIC KVM device attribute to set the maintenance IRQ.
> This is fixed to use IRQ 25(PPI 9), as a platform decision matching
> the arm64 SBSA recommendation.
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  hw/arm/virt.c                      |  5 +++++
>  hw/intc/arm_gicv3_common.c         |  1 +
>  hw/intc/arm_gicv3_kvm.c            | 16 ++++++++++++++++
>  include/hw/intc/arm_gicv3_common.h |  1 +
>  4 files changed, 23 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index aa2bbd14e0..92d46ebcfe 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -663,6 +663,11 @@ static void create_gic(VirtMachineState *vms)
>              qdev_prop_set_uint32(vms->gic, "redist-region-count[1]",
>                  MIN(smp_cpus - redist0_count, redist1_capacity));
>          }
> +
> +        if (kvm_irqchip_in_kernel()) {
> +            bool el2 = object_property_get_bool(OBJECT(first_cpu), "el2", NULL);
> +            qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", el2);
> +        }
>      } 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 58ef65f589..3ac10c8e61 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -495,6 +495,7 @@ static Property arm_gicv3_common_properties[] = {
>      DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
>      DEFINE_PROP_UINT32("revision", GICv3State, revision, 3),
>      DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0),
> +    DEFINE_PROP_BOOL("has-virtualization-extensions", GICv3State, virt_extn, 0),
>      DEFINE_PROP_ARRAY("redist-region-count", GICv3State, nb_redist_regions,
>                        redist_region_count, qdev_prop_uint32, uint32_t),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 65a4c880a3..1e1ca66e2c 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -826,6 +826,22 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>      kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>                        KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true, &error_abort);
>  
> +    if (s->virt_extn) {
> +        bool maint_irq_allowed;
> +        uint32_t maint_irq = 25;

Please use KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ + 16, or better would be
something like PPI(KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ). We have a PPI() macro
in virt.h. I'm not sure if/where we could move that, though.

> +
> +        maint_irq_allowed =
> +            kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0);
> +        if (!maint_irq_allowed) {

I'll defer to the maintainers, but I'd rather see

        if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0)) {

which is slightly longer than 80 chars, then require the use of a local
variable and the broken assignment line.

> +            error_setg(errp, "VGICv3 setting maintenance IRQ are not "
> +                             "supported by this host kernel");

"VGICv3 maintenance IRQ setting is not supported by this host kernel"

Also, I think we're trying not to brake error lines like this. It makes
grepping harder.

> +            return;
> +        }
> +
> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ,
> +                          0, &maint_irq, true, &error_abort);
> +    }
> +
>      kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>                              KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd, 0);
>  
> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
> index 91491a2f66..921ddc2c5f 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -220,6 +220,7 @@ struct GICv3State {
>      uint32_t num_irq;
>      uint32_t revision;
>      bool security_extn;
> +    bool virt_extn;
>      bool irq_reset_nonsecure;
>      bool gicd_no_migration_shift_bug;
>  
> -- 
> 2.17.1
> 
>

Thanks,
drew