Re: [Xen-devel] [PATCH v3 12/24] xen/arm: Release IRQ routed to a domain when it's destroying
Hi Ian, On 20/02/2015 17:41, Julien Grall wrote: >>> +/* Only SPIs are supported */ +if ( virq < 32 || virq >= vgic_num_irqs(d) ) +return -EINVAL; + +p = irq_to_pending(d->vcpu[0], virq); +if ( !p->desc ) +return -EINVAL; Are we seeing this pattern a lot? It seems so, I wonder if a helper could be useful: p = spi_to_pending(d, virq); if ( !p->desc ) return -EINVAL; with the < 32 check etc in the helper where it only needs commenting on once. IIRC, there is only 2 places. I will replace it by an helper. H I only found 1 place. So I will keep open-coding in the function. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 12/24] xen/arm: Release IRQ routed to a domain when it's destroying
Hi Ian,
On 23/02/15 15:25, Ian Campbell wrote:
> On Fri, 2015-02-20 at 17:41 +, Julien Grall wrote:
>
+/* TODO: Handle eviction from LRs. For now, deny remove if the IRQ
+ * is inflight and not disabled.
>>>
>>> If we are ungracefully killing a guest doesn't this risk ending up with
>>> an undestroyable domain? i.e. if the guest is paused with an inflight
>>> IRQ and then destroyed, how does the inflight IRQ ever become
>>> not-inflight again? A similar argument could apply if the guest has e.g.
>>> crashed, paniced or is otherwise not processing any more interrupts or
>>> generating EOIs for existing ones.
>>
>> No, this will fall on the "is_dying" part. During domain destruction,
>> the hypervisor will release all the IRQ still assigned to the guest one
>> by one.
>>
>>> We need to be able to kill a guest in such a state somehow.
>>
>> The TODO is here for running domain where we try to deassign an inflight
>> IRQ.
>
> I see. I think either expand the comment to say "for a running domain"
> or, probably better, put this bit of code (and the comment) in an else
> of the is_dying and get rid of the goto in the is_dying==true case.
I will do.
+for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
+{
+struct pending_irq *p = &d->arch.vgic.pending_irqs[i];
>>>
>>> Is there not a helper for this lookup? If so it should be used.
>>
>> The irq_pending code is adding extra-check. But I guess we don't care
>> for domain destruction?
>
> I don't think so.
Ok.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 12/24] xen/arm: Release IRQ routed to a domain when it's destroying
On Fri, 2015-02-20 at 17:41 +, Julien Grall wrote:
> >> +/* TODO: Handle eviction from LRs. For now, deny remove if the IRQ
> >> + * is inflight and not disabled.
> >
> > If we are ungracefully killing a guest doesn't this risk ending up with
> > an undestroyable domain? i.e. if the guest is paused with an inflight
> > IRQ and then destroyed, how does the inflight IRQ ever become
> > not-inflight again? A similar argument could apply if the guest has e.g.
> > crashed, paniced or is otherwise not processing any more interrupts or
> > generating EOIs for existing ones.
>
> No, this will fall on the "is_dying" part. During domain destruction,
> the hypervisor will release all the IRQ still assigned to the guest one
> by one.
>
> > We need to be able to kill a guest in such a state somehow.
>
> The TODO is here for running domain where we try to deassign an inflight
> IRQ.
I see. I think either expand the comment to say "for a running domain"
or, probably better, put this bit of code (and the comment) in an else
of the is_dying and get rid of the goto in the is_dying==true case.
> >> +for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
> >> +{
> >> +struct pending_irq *p = &d->arch.vgic.pending_irqs[i];
> >
> > Is there not a helper for this lookup? If so it should be used.
>
> The irq_pending code is adding extra-check. But I guess we don't care
> for domain destruction?
I don't think so.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 12/24] xen/arm: Release IRQ routed to a domain when it's destroying
Hi Ian,
On 20/02/15 16:31, Ian Campbell wrote:
> On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote:
>
>> Futhermore, a guest can crash and let the IRQ in an incorrect state (i.e has
>
> "Furthermore" (I think your finger macros have this one wrong, you might
> want to grep the series ;-))
I do this mistake every-time. I will check this series (and upcoming series)
>
>> +/* TODO: Handle eviction from LRs. For now, deny remove if the IRQ
>> + * is inflight and not disabled.
>
> If we are ungracefully killing a guest doesn't this risk ending up with
> an undestroyable domain? i.e. if the guest is paused with an inflight
> IRQ and then destroyed, how does the inflight IRQ ever become
> not-inflight again? A similar argument could apply if the guest has e.g.
> crashed, paniced or is otherwise not processing any more interrupts or
> generating EOIs for existing ones.
No, this will fall on the "is_dying" part. During domain destruction,
the hypervisor will release all the IRQ still assigned to the guest one
by one.
> We need to be able to kill a guest in such a state somehow.
The TODO is here for running domain where we try to deassign an inflight
IRQ.
> (BTW, I notice the comment style is consistently wrong through the
> series)
I will check all the patches.
>> +/* Only SPIs are supported */
>> +if ( virq < 32 || virq >= vgic_num_irqs(d) )
>> +return -EINVAL;
>> +
>> +p = irq_to_pending(d->vcpu[0], virq);
>> +if ( !p->desc )
>> +return -EINVAL;
>
> Are we seeing this pattern a lot? It seems so, I wonder if a helper
> could be useful:
>p = spi_to_pending(d, virq);
>if ( !p->desc )
>return -EINVAL;
>
> with the < 32 check etc in the helper where it only needs commenting on
> once.
IIRC, there is only 2 places. I will replace it by an helper.
>> +
>> +desc = p->desc;
>> +
>> +spin_lock_irqsave(&desc->lock, flags);
>> +
>> +ret = -EINVAL;
>> +if ( !test_bit(_IRQ_GUEST, &desc->status) )
>> +goto unlock;
>> +
>> +ret = -EINVAL;
>
> A bit redundant, or should nestle the info = like you did above with the
> test_bit.
I will invert it.
>> +info = irq_get_guest_info(desc);
>> +if ( d != info->d )
>> +goto unlock;
>> +
>> +ret = gic_remove_irq_from_guest(d, virq, desc);
>> +
>> +spin_unlock_irqrestore(&desc->lock, flags);
>> +
>> +if ( !ret )
>
> This is a bit unconventional (it looks essentially like you have three
> exit paths, with this extra special success case and two error cases).
>
> It would better if the gic_remove_irq_from_guest failure case went to
> unlock like all the other failure cases, then the success path would be
> a straight line.
>
> (yes, that would mean a goto unlock right before the first spin_unlock,
> but the code flow would be clearer).
I will do.
>
>> +{
>> +release_irq(desc->irq, info);
>> +xfree(info);
>> +}
>> +
>> +return ret;
>> +
>> +unlock:
>> +spin_unlock_irqrestore(&desc->lock, flags);
>> +
>> +return ret;
>> +}
>> +
>> /*
>> * pirq event channels. We don't use these on ARM, instead we use the
>> * features of the GIC to inject virtualised normal interrupts.
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index fc8a270..4ddfd73 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -133,6 +133,22 @@ void register_vgic_ops(struct domain *d, const struct
>> vgic_ops *ops)
>>
>> void domain_vgic_free(struct domain *d)
>> {
>> +int i;
>> +int ret;
>> +
>> +for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
>> +{
>> +struct pending_irq *p = &d->arch.vgic.pending_irqs[i];
>
> Is there not a helper for this lookup? If so it should be used.
The irq_pending code is adding extra-check. But I guess we don't care
for domain destruction?
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 12/24] xen/arm: Release IRQ routed to a domain when it's destroying
On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote:
> Futhermore, a guest can crash and let the IRQ in an incorrect state (i.e has
"Furthermore" (I think your finger macros have this one wrong, you might
want to grep the series ;-))
> +/* TODO: Handle eviction from LRs. For now, deny remove if the IRQ
> + * is inflight and not disabled.
If we are ungracefully killing a guest doesn't this risk ending up with
an undestroyable domain? i.e. if the guest is paused with an inflight
IRQ and then destroyed, how does the inflight IRQ ever become
not-inflight again? A similar argument could apply if the guest has e.g.
crashed, paniced or is otherwise not processing any more interrupts or
generating EOIs for existing ones.
We need to be able to kill a guest in such a state somehow.
(BTW, I notice the comment style is consistently wrong through the
series)
> +/* Only SPIs are supported */
> +if ( virq < 32 || virq >= vgic_num_irqs(d) )
> +return -EINVAL;
> +
> +p = irq_to_pending(d->vcpu[0], virq);
> +if ( !p->desc )
> +return -EINVAL;
Are we seeing this pattern a lot? It seems so, I wonder if a helper
could be useful:
p = spi_to_pending(d, virq);
if ( !p->desc )
return -EINVAL;
with the < 32 check etc in the helper where it only needs commenting on
once.
> +
> +desc = p->desc;
> +
> +spin_lock_irqsave(&desc->lock, flags);
> +
> +ret = -EINVAL;
> +if ( !test_bit(_IRQ_GUEST, &desc->status) )
> +goto unlock;
> +
> +ret = -EINVAL;
A bit redundant, or should nestle the info = like you did above with the
test_bit.
> +info = irq_get_guest_info(desc);
> +if ( d != info->d )
> +goto unlock;
> +
> +ret = gic_remove_irq_from_guest(d, virq, desc);
> +
> +spin_unlock_irqrestore(&desc->lock, flags);
> +
> +if ( !ret )
This is a bit unconventional (it looks essentially like you have three
exit paths, with this extra special success case and two error cases).
It would better if the gic_remove_irq_from_guest failure case went to
unlock like all the other failure cases, then the success path would be
a straight line.
(yes, that would mean a goto unlock right before the first spin_unlock,
but the code flow would be clearer).
> +{
> +release_irq(desc->irq, info);
> +xfree(info);
> +}
> +
> +return ret;
> +
> +unlock:
> +spin_unlock_irqrestore(&desc->lock, flags);
> +
> +return ret;
> +}
> +
> /*
> * pirq event channels. We don't use these on ARM, instead we use the
> * features of the GIC to inject virtualised normal interrupts.
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index fc8a270..4ddfd73 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -133,6 +133,22 @@ void register_vgic_ops(struct domain *d, const struct
> vgic_ops *ops)
>
> void domain_vgic_free(struct domain *d)
> {
> +int i;
> +int ret;
> +
> +for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
> +{
> +struct pending_irq *p = &d->arch.vgic.pending_irqs[i];
Is there not a helper for this lookup? If so it should be used.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 12/24] xen/arm: Release IRQ routed to a domain when it's destroying
On Tue, 13 Jan 2015, Julien Grall wrote:
> Xen has to release IRQ routed to a domain in order to reuse later. Currently
> only SPIs can be routed to the guest so we only need to browse SPIs for a
> specific domain.
>
> Futhermore, a guest can crash and let the IRQ in an incorrect state (i.e has
> not being EOIed). Xen will have to reset the IRQ in order to be able to reuse
> the IRQ later.
>
> Introduce 2 new functions for release an IRQ routed to a domain:
> - release_guest_irq: upper level to retrieve the IRQ, call the GIC
> code and release the action
> - gic_remove_guest_irq: Check if we can remove the IRQ, and reset
> it if necessary
>
> Signed-off-by: Julien Grall
Acked-by: Stefano Stabellini
> ---
> Changes in v3:
> - Take the vgic rank lock to protect p->desc
> - Correctly check if the IRQ is disabled
> - Extend the check on the virq in release_guest_irq
> - Use vgic_get_target_vcpu to get the target vCPU
> - Remove spurious change
>
> Changes in v2:
> - Drop the desc->handler = &no_irq_type in release_irq as it's
> buggy if the IRQ is routed to Xen
> - Add release_guest_irq and gic_remove_guest_irq
> ---
> xen/arch/arm/gic.c| 46 +
> xen/arch/arm/irq.c| 48
> +++
> xen/arch/arm/vgic.c | 16
> xen/include/asm-arm/gic.h | 4
> xen/include/asm-arm/irq.h | 2 ++
> 5 files changed, 116 insertions(+)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 240870f..bb298e9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -162,6 +162,52 @@ out:
> return res;
> }
>
> +/* This function only works with SPIs for now */
> +int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> + struct irq_desc *desc)
> +{
> +struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> +struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> +struct pending_irq *p = irq_to_pending(v_target, virq);
> +unsigned long flags;
> +
> +ASSERT(spin_is_locked(&desc->lock));
> +ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> +ASSERT(p->desc == desc);
> +
> +vgic_lock_rank(v_target, rank, flags);
> +
> +/* If the IRQ is removed when the domain is dying, we only need to
> + * EOI the IRQ if it has not been done by the guest
> + */
> +if ( d->is_dying )
> +{
> +desc->handler->shutdown(desc);
> +if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
> +gic_hw_ops->deactivate_irq(desc);
> +clear_bit(_IRQ_INPROGRESS, &desc->status);
> +goto end;
> +}
> +
> +/* TODO: Handle eviction from LRs. For now, deny remove if the IRQ
> + * is inflight and not disabled.
> + */
> +if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
> + !test_bit(_IRQ_DISABLED, &desc->status) )
> +return -EBUSY;
> +
> +end:
> +clear_bit(_IRQ_GUEST, &desc->status);
> +desc->handler = &no_irq_type;
> +
> +p->desc = NULL;
> +
> +vgic_unlock_rank(v_target, rank, flags);
> +
> +
> +return 0;
> +}
> +
> int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>unsigned int *out_hwirq,
>unsigned int *out_type)
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 0072347..ce5ae1a 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -504,6 +504,54 @@ free_info:
> return retval;
> }
>
> +int release_guest_irq(struct domain *d, unsigned int virq)
> +{
> +struct irq_desc *desc;
> +struct irq_guest *info;
> +unsigned long flags;
> +struct pending_irq *p;
> +int ret;
> +
> +/* Only SPIs are supported */
> +if ( virq < 32 || virq >= vgic_num_irqs(d) )
> +return -EINVAL;
> +
> +p = irq_to_pending(d->vcpu[0], virq);
> +if ( !p->desc )
> +return -EINVAL;
> +
> +desc = p->desc;
> +
> +spin_lock_irqsave(&desc->lock, flags);
> +
> +ret = -EINVAL;
> +if ( !test_bit(_IRQ_GUEST, &desc->status) )
> +goto unlock;
> +
> +ret = -EINVAL;
> +
> +info = irq_get_guest_info(desc);
> +if ( d != info->d )
> +goto unlock;
> +
> +ret = gic_remove_irq_from_guest(d, virq, desc);
> +
> +spin_unlock_irqrestore(&desc->lock, flags);
> +
> +if ( !ret )
> +{
> +release_irq(desc->irq, info);
> +xfree(info);
> +}
> +
> +return ret;
> +
> +unlock:
> +spin_unlock_irqrestore(&desc->lock, flags);
> +
> +return ret;
> +}
> +
> /*
> * pirq event channels. We don't use these on ARM, instead we use the
> * features of the GIC to inject virtualised normal interrupts.
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index fc8a270..4ddfd73 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -133,6 +133,22 @@ void register_vgic_ops(
[Xen-devel] [PATCH v3 12/24] xen/arm: Release IRQ routed to a domain when it's destroying
Xen has to release IRQ routed to a domain in order to reuse later. Currently
only SPIs can be routed to the guest so we only need to browse SPIs for a
specific domain.
Futhermore, a guest can crash and let the IRQ in an incorrect state (i.e has
not being EOIed). Xen will have to reset the IRQ in order to be able to reuse
the IRQ later.
Introduce 2 new functions for release an IRQ routed to a domain:
- release_guest_irq: upper level to retrieve the IRQ, call the GIC
code and release the action
- gic_remove_guest_irq: Check if we can remove the IRQ, and reset
it if necessary
Signed-off-by: Julien Grall
---
Changes in v3:
- Take the vgic rank lock to protect p->desc
- Correctly check if the IRQ is disabled
- Extend the check on the virq in release_guest_irq
- Use vgic_get_target_vcpu to get the target vCPU
- Remove spurious change
Changes in v2:
- Drop the desc->handler = &no_irq_type in release_irq as it's
buggy if the IRQ is routed to Xen
- Add release_guest_irq and gic_remove_guest_irq
---
xen/arch/arm/gic.c| 46 +
xen/arch/arm/irq.c| 48 +++
xen/arch/arm/vgic.c | 16
xen/include/asm-arm/gic.h | 4
xen/include/asm-arm/irq.h | 2 ++
5 files changed, 116 insertions(+)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 240870f..bb298e9 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -162,6 +162,52 @@ out:
return res;
}
+/* This function only works with SPIs for now */
+int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
+ struct irq_desc *desc)
+{
+struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
+struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
+struct pending_irq *p = irq_to_pending(v_target, virq);
+unsigned long flags;
+
+ASSERT(spin_is_locked(&desc->lock));
+ASSERT(test_bit(_IRQ_GUEST, &desc->status));
+ASSERT(p->desc == desc);
+
+vgic_lock_rank(v_target, rank, flags);
+
+/* If the IRQ is removed when the domain is dying, we only need to
+ * EOI the IRQ if it has not been done by the guest
+ */
+if ( d->is_dying )
+{
+desc->handler->shutdown(desc);
+if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
+gic_hw_ops->deactivate_irq(desc);
+clear_bit(_IRQ_INPROGRESS, &desc->status);
+goto end;
+}
+
+/* TODO: Handle eviction from LRs. For now, deny remove if the IRQ
+ * is inflight and not disabled.
+ */
+if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
+ !test_bit(_IRQ_DISABLED, &desc->status) )
+return -EBUSY;
+
+end:
+clear_bit(_IRQ_GUEST, &desc->status);
+desc->handler = &no_irq_type;
+
+p->desc = NULL;
+
+vgic_unlock_rank(v_target, rank, flags);
+
+
+return 0;
+}
+
int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
unsigned int *out_hwirq,
unsigned int *out_type)
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 0072347..ce5ae1a 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -504,6 +504,54 @@ free_info:
return retval;
}
+int release_guest_irq(struct domain *d, unsigned int virq)
+{
+struct irq_desc *desc;
+struct irq_guest *info;
+unsigned long flags;
+struct pending_irq *p;
+int ret;
+
+/* Only SPIs are supported */
+if ( virq < 32 || virq >= vgic_num_irqs(d) )
+return -EINVAL;
+
+p = irq_to_pending(d->vcpu[0], virq);
+if ( !p->desc )
+return -EINVAL;
+
+desc = p->desc;
+
+spin_lock_irqsave(&desc->lock, flags);
+
+ret = -EINVAL;
+if ( !test_bit(_IRQ_GUEST, &desc->status) )
+goto unlock;
+
+ret = -EINVAL;
+
+info = irq_get_guest_info(desc);
+if ( d != info->d )
+goto unlock;
+
+ret = gic_remove_irq_from_guest(d, virq, desc);
+
+spin_unlock_irqrestore(&desc->lock, flags);
+
+if ( !ret )
+{
+release_irq(desc->irq, info);
+xfree(info);
+}
+
+return ret;
+
+unlock:
+spin_unlock_irqrestore(&desc->lock, flags);
+
+return ret;
+}
+
/*
* pirq event channels. We don't use these on ARM, instead we use the
* features of the GIC to inject virtualised normal interrupts.
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index fc8a270..4ddfd73 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -133,6 +133,22 @@ void register_vgic_ops(struct domain *d, const struct
vgic_ops *ops)
void domain_vgic_free(struct domain *d)
{
+int i;
+int ret;
+
+for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
+{
+struct pending_irq *p = &d->arch.vgic.pending_irqs[i];
+
+if ( p->desc )
+{
+ret = release_guest_irq(d, p->irq);
+if ( ret )
+dprintk(XENLOG_G_
