| Message ID | 20191115201037.44982-4-stewart.hildebrand@dornerworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: arm: context switch vtimer PPI state | expand |
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; > }
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,
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; }
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(-)