Re: [Xen-devel] [RFC XEN PATCH v3 08/11] xen: arm: vgic: don't fail if IRQ is already connected

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
> There are some IRQs that happen to have multiple "interrupts = < ... >;"
> properties with the same IRQ in the device tree. For example:
> 
> interrupts = <0 123 4>,
>              <0 123 4>,
>              <0 123 4>,
>              <0 123 4>,
>              <0 123 4>;
> 
> In this case it seems that we are invoking vgic_connect_hw_irq multiple
> times for the same IRQ.
> 
> Rework the checks to allow booting in this scenario.
> 
> I have not seen any cases where the pre-existing p->desc is any different from
> the new desc, so BUG() out if they're different for now.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxxxxxxxxxx>
> 
> ---
> v3: new patch
> 
> I tested on Xilinx Zynq UltraScale+ with the old vGIC. I have not fully
> tested with CONFIG_NEW_VGIC. This hack only became necessary after
> introducing the PPI series, and I'm not entirely sure what the reason
> is for that.

I think the reason is actually very simple: with the previous code if
the irq was already setup and the details matched it would "goto out"
all the way out of route_irq_to_guest.

Now with the new code, it would "goto out" of setup_guest_irq returning
zero, which means that gic_route_irq_to_guest is actually going to be
called anyway, which is a mistake. I think we want to avoid that by
returning an appropriate error condition from setup_guest_irq so that we
also return early from route_irq_to_guest.


> I'm also unsure if BUG()ing out is the right thing to do in case of
> desc != p->desc, or what conditions would even trigger this? Is this
> function exposed to guests?

I think the original code printed a warning and returned an error.
That's probably still what we want.



> ---
>  xen/arch/arm/gic-vgic.c  | 9 +++++++--
>  xen/arch/arm/vgic/vgic.c | 4 ++++
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 2c66a8fa92..5c16e66b32 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -460,9 +460,14 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu 
> *v, unsigned int virq,
>      if ( connect )
>      {
>          /* The VIRQ should not be already enabled by the guest */
> -        if ( !p->desc &&
> -             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> +        if ( !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> +        {
> +            if (p->desc && p->desc != desc)

Code style


> +            {
> +                BUG();
> +            }
>              p->desc = desc;
> +        }
>          else
>              ret = -EBUSY;
>      }
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index f0f2ea5021..aa775f7668 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -882,6 +882,10 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu 
> *vcpu,
>              irq->hw = true;
>              irq->hwintid = desc->irq;
>          }
> +        else if ( irq->hw && !irq->enabled && irq->hwintid == desc->irq )
> +        {
> +            /* The IRQ was already connected. No action is necessary. */
> +        }
>          else
>              ret = -EBUSY;
>      }


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel