[RFC,XEN,v3,08/11] xen: arm: vgic: don't fail if IRQ is already connected
diff mbox series

Message ID 20191115201037.44982-4-stewart.hildebrand@dornerworks.com
State New
Headers show
Series
  • xen: arm: context switch vtimer PPI state
Related show

Commit Message

Stewart Hildebrand Nov. 15, 2019, 8:10 p.m. UTC
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@dornerworks.com>

---
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'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?
---
 xen/arch/arm/gic-vgic.c  | 9 +++++++--
 xen/arch/arm/vgic/vgic.c | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Stefano Stabellini Nov. 26, 2019, 11:16 p.m. UTC | #1
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@dornerworks.com>
> 
> ---
> 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;
>      }
Julien Grall Dec. 3, 2019, 2:24 p.m. UTC | #2
Hi,

On 15/11/2019 20:10, 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@dornerworks.com>
> 
> ---
> 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'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?

This can happen with PPIs as the desc is per-CPU. If you migrate the 
vCPU to another pCPU, you will likely hit the BUG() below if the guest 
disabled the interrupt.

But I don't think we should call vgic_connect_hw_irq() multiples time on 
the same IRQ. The upper layer should take care of such issue (such as 
setup_guest_irq()).

Cheers,

Patch
diff mbox series

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)
+            {
+                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;
     }