| Message ID | 1406818852-31856-10-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 31 Jul 2014, 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 <julien.grall@linaro.org> > > --- > Changes in v2: > - Drop the desc->handler = &no_irq_type in release_irq as it's > buggy the IRQ is routed to Xen > - Add release_guest_irq and gic_remove_guest_irq > --- > xen/arch/arm/gic.c | 36 ++++++++++++++++++++++++++++++++++ > 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, 106 insertions(+) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 8ef8764..22f331a 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -144,6 +144,42 @@ void gic_route_irq_to_guest(struct domain *d, unsigned int virq, > p->desc = desc; > } > > +/* 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 pending_irq *p = irq_to_pending(d->vcpu[0], virq); Use vgic_get_target_vcpu to get the target vcpu of virq. You can pass d->vcpu[0] as first argument to vgic_get_target_vcpu. I am sorry that doing that is going to add a dependency on my gic series. > + ASSERT(spin_is_locked(&desc->lock)); > + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); > + ASSERT(p->desc == desc); > + > + /* 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: > + desc->handler = &no_irq_type; > + p->desc = NULL; > + > + 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 7eeb8dd..ba33571 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -292,6 +292,7 @@ void release_irq(unsigned int irq, const void *dev_id) > if ( !desc->action ) > { > desc->handler->shutdown(desc); > + > clear_bit(_IRQ_GUEST, &desc->status); > } spurious change > @@ -479,6 +480,53 @@ out: > 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; > + > + if ( virq >= vgic_num_irqs(d) ) > + return -EINVAL; > + > + p = irq_to_pending(d->vcpu[0], virq); > + if ( !p->desc ) > + return -EINVAL; Same here: call vgic_get_target_vcpu. Also if this function is supposed to work only with SPIs, you should add a comment or explicitly check for it. > + 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 a037ecc..2a5fc18 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -118,6 +118,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_WARNING, "d%u: Failed to release virq %u ret = %d\n", > + d->domain_id, p->irq, ret); > + } > + } > + > xfree(d->arch.vgic.shared_irqs); > xfree(d->arch.vgic.pending_irqs); > } > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 89816cd..46d7fd6 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -201,6 +201,10 @@ extern void gic_route_irq_to_guest(struct domain *, unsigned int virq, > const cpumask_t *cpu_mask, > unsigned int priority); > > +/* Remove an IRQ passthrough to a guest */ > +int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, > + struct irq_desc *desc); > + > extern void gic_inject(void); > extern void gic_clear_pending_irqs(struct vcpu *v); > extern int gic_events_need_delivery(void); > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index a7174f3..d25e1a1 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -44,6 +44,8 @@ void init_secondary_IRQ(void); > > int route_irq_to_guest(struct domain *d, unsigned int virq, > unsigned int irq, const char *devname); > +int release_guest_irq(struct domain *d, unsigned int irq); > + > void arch_move_irqs(struct vcpu *v); > > /* Set IRQ type for an SPI */ > -- > 1.7.10.4 >
Hi Stefano, On 08/06/2014 04:49 PM, Stefano Stabellini wrote: > On Thu, 31 Jul 2014, 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 <julien.grall@linaro.org> >> >> --- >> Changes in v2: >> - Drop the desc->handler = &no_irq_type in release_irq as it's >> buggy the IRQ is routed to Xen >> - Add release_guest_irq and gic_remove_guest_irq >> --- >> xen/arch/arm/gic.c | 36 ++++++++++++++++++++++++++++++++++ >> 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, 106 insertions(+) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 8ef8764..22f331a 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -144,6 +144,42 @@ void gic_route_irq_to_guest(struct domain *d, unsigned int virq, >> p->desc = desc; >> } >> >> +/* 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 pending_irq *p = irq_to_pending(d->vcpu[0], virq); > > Use vgic_get_target_vcpu to get the target vcpu of virq. You can pass > d->vcpu[0] as first argument to vgic_get_target_vcpu. Why do I need to add vgic_get_target_vcpu? This function is only able to handle SPIs which is shared between VCPU. It might be interesting to introduce spi_to_pending function. > I am sorry that doing that is going to add a dependency on my gic series. I'm already depends on your series :). >> + ASSERT(spin_is_locked(&desc->lock)); >> + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); >> + ASSERT(p->desc == desc); >> + >> + /* 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: >> + desc->handler = &no_irq_type; >> + p->desc = NULL; >> + >> + 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 7eeb8dd..ba33571 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -292,6 +292,7 @@ void release_irq(unsigned int irq, const void *dev_id) >> if ( !desc->action ) >> { >> desc->handler->shutdown(desc); >> + >> clear_bit(_IRQ_GUEST, &desc->status); >> } > > > spurious change Will drop the line. > >> @@ -479,6 +480,53 @@ out: >> 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; >> + >> + if ( virq >= vgic_num_irqs(d) ) >> + return -EINVAL; >> + >> + p = irq_to_pending(d->vcpu[0], virq); >> + if ( !p->desc ) >> + return -EINVAL; > > Same here: call vgic_get_target_vcpu. > Also if this function is supposed to work only with SPIs, you should add > a comment or explicitly check for it. route_irq_to_guest already check if we are able to route an IRQ or not. For non-SPIs the function will bailout. So, here, it's impossible to have p->desc set to another value than NULL for non-SPIs. Or Xen is buggy will likely fail in another place. Regards,
On Wed, 6 Aug 2014, Julien Grall wrote: > Hi Stefano, > > On 08/06/2014 04:49 PM, Stefano Stabellini wrote: > > On Thu, 31 Jul 2014, 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 <julien.grall@linaro.org> > >> > >> --- > >> Changes in v2: > >> - Drop the desc->handler = &no_irq_type in release_irq as it's > >> buggy the IRQ is routed to Xen > >> - Add release_guest_irq and gic_remove_guest_irq > >> --- > >> xen/arch/arm/gic.c | 36 ++++++++++++++++++++++++++++++++++ > >> 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, 106 insertions(+) > >> > >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >> index 8ef8764..22f331a 100644 > >> --- a/xen/arch/arm/gic.c > >> +++ b/xen/arch/arm/gic.c > >> @@ -144,6 +144,42 @@ void gic_route_irq_to_guest(struct domain *d, unsigned int virq, > >> p->desc = desc; > >> } > >> > >> +/* 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 pending_irq *p = irq_to_pending(d->vcpu[0], virq); > > > > Use vgic_get_target_vcpu to get the target vcpu of virq. You can pass > > d->vcpu[0] as first argument to vgic_get_target_vcpu. > > Why do I need to add vgic_get_target_vcpu? This function is only able to > handle SPIs which is shared between VCPU. OK, in that case ASSERT(virq >= 32 && virq < nr_lines). I am fine either way. Also see below. > It might be interesting to introduce spi_to_pending function. > > > I am sorry that doing that is going to add a dependency on my gic series. > > I'm already depends on your series :). > > >> + ASSERT(spin_is_locked(&desc->lock)); > >> + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); > >> + ASSERT(p->desc == desc); > >> + > >> + /* 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: > >> + desc->handler = &no_irq_type; > >> + p->desc = NULL; > >> + > >> + 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 7eeb8dd..ba33571 100644 > >> --- a/xen/arch/arm/irq.c > >> +++ b/xen/arch/arm/irq.c > >> @@ -292,6 +292,7 @@ void release_irq(unsigned int irq, const void *dev_id) > >> if ( !desc->action ) > >> { > >> desc->handler->shutdown(desc); > >> + > >> clear_bit(_IRQ_GUEST, &desc->status); > >> } > > > > > > spurious change > > Will drop the line. > > > > >> @@ -479,6 +480,53 @@ out: > >> 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; > >> + > >> + if ( virq >= vgic_num_irqs(d) ) > >> + return -EINVAL; > >> + > >> + p = irq_to_pending(d->vcpu[0], virq); > >> + if ( !p->desc ) > >> + return -EINVAL; > > > > Same here: call vgic_get_target_vcpu. > > Also if this function is supposed to work only with SPIs, you should add > > a comment or explicitly check for it. > > route_irq_to_guest already check if we are able to route an IRQ or not. > For non-SPIs the function will bailout. > > So, here, it's impossible to have p->desc set to another value than NULL > for non-SPIs. > > Or Xen is buggy will likely fail in another place. If you do: p = irq_to_pending(d->vcpu[0], virq); you are actually introducing more code that cannot cope with non-SGIs. So you should either: 1) esplicitely check for it (add an ASSERT) 2) replace it with code that can cope with non-SGIs, such us irq_to_pending(vgic_get_target_vcpu(d->vcpu[0], virq), virq) I would prefer 2), but 1) is also an option
On 08/06/2014 05:53 PM, Stefano Stabellini wrote: > On Wed, 6 Aug 2014, Julien Grall wrote: >> Hi Stefano, >> >> On 08/06/2014 04:49 PM, Stefano Stabellini wrote: >>> On Thu, 31 Jul 2014, 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 <julien.grall@linaro.org> >>>> >>>> --- >>>> Changes in v2: >>>> - Drop the desc->handler = &no_irq_type in release_irq as it's >>>> buggy the IRQ is routed to Xen >>>> - Add release_guest_irq and gic_remove_guest_irq >>>> --- >>>> xen/arch/arm/gic.c | 36 ++++++++++++++++++++++++++++++++++ >>>> 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, 106 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>>> index 8ef8764..22f331a 100644 >>>> --- a/xen/arch/arm/gic.c >>>> +++ b/xen/arch/arm/gic.c >>>> @@ -144,6 +144,42 @@ void gic_route_irq_to_guest(struct domain *d, unsigned int virq, >>>> p->desc = desc; >>>> } >>>> >>>> +/* 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 pending_irq *p = irq_to_pending(d->vcpu[0], virq); >>> >>> Use vgic_get_target_vcpu to get the target vcpu of virq. You can pass >>> d->vcpu[0] as first argument to vgic_get_target_vcpu. >> >> Why do I need to add vgic_get_target_vcpu? This function is only able to >> handle SPIs which is shared between VCPU. > > OK, in that case ASSERT(virq >= 32 && virq < nr_lines). I am fine either way. > Also see below. It's implicitly done by (p->desc == desc). p->desc is only set for SPIs assigned to a guest. If desc is NULL, then it will fault a bit later. If someone doesn't use this API to route an IRQ then it's his fault. Hence, this as been checked in route_irq_guest. I don't think we should bother to check again. >>> >>>> @@ -479,6 +480,53 @@ out: >>>> 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; >>>> + >>>> + if ( virq >= vgic_num_irqs(d) ) >>>> + return -EINVAL; >>>> + >>>> + p = irq_to_pending(d->vcpu[0], virq); >>>> + if ( !p->desc ) >>>> + return -EINVAL; >>> >>> Same here: call vgic_get_target_vcpu. >>> Also if this function is supposed to work only with SPIs, you should add >>> a comment or explicitly check for it. >> >> route_irq_to_guest already check if we are able to route an IRQ or not. >> For non-SPIs the function will bailout. >> >> So, here, it's impossible to have p->desc set to another value than NULL >> for non-SPIs. >> >> Or Xen is buggy will likely fail in another place. > > If you do: > > p = irq_to_pending(d->vcpu[0], virq); > > you are actually introducing more code that cannot cope with non-SGIs. > So you should either: > > 1) esplicitely check for it (add an ASSERT) Already done in route_irq_to_guest. I don't think we have to add yet another assert here. > 2) replace it with code that can cope with non-SGIs, such us > irq_to_pending(vgic_get_target_vcpu(d->vcpu[0], virq), virq) This code won't cope with non-SGIs (here PPIs). As PPIs have an irq_desc per CPU we will have to loop on every VCPU to unmap it. But I doubt we will have PPIs in future, there is more issues to handle (such as the number of VCPUs doesn't match the number of physical CPUs).
On Wed, 6 Aug 2014, Julien Grall wrote: > On 08/06/2014 05:53 PM, Stefano Stabellini wrote: > > On Wed, 6 Aug 2014, Julien Grall wrote: > >> Hi Stefano, > >> > >> On 08/06/2014 04:49 PM, Stefano Stabellini wrote: > >>> On Thu, 31 Jul 2014, 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 <julien.grall@linaro.org> > >>>> > >>>> --- > >>>> Changes in v2: > >>>> - Drop the desc->handler = &no_irq_type in release_irq as it's > >>>> buggy the IRQ is routed to Xen > >>>> - Add release_guest_irq and gic_remove_guest_irq > >>>> --- > >>>> xen/arch/arm/gic.c | 36 ++++++++++++++++++++++++++++++++++ > >>>> 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, 106 insertions(+) > >>>> > >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >>>> index 8ef8764..22f331a 100644 > >>>> --- a/xen/arch/arm/gic.c > >>>> +++ b/xen/arch/arm/gic.c > >>>> @@ -144,6 +144,42 @@ void gic_route_irq_to_guest(struct domain *d, unsigned int virq, > >>>> p->desc = desc; > >>>> } > >>>> > >>>> +/* 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 pending_irq *p = irq_to_pending(d->vcpu[0], virq); > >>> > >>> Use vgic_get_target_vcpu to get the target vcpu of virq. You can pass > >>> d->vcpu[0] as first argument to vgic_get_target_vcpu. > >> > >> Why do I need to add vgic_get_target_vcpu? This function is only able to > >> handle SPIs which is shared between VCPU. > > > > OK, in that case ASSERT(virq >= 32 && virq < nr_lines). I am fine either way. > > Also see below. > > It's implicitly done by (p->desc == desc). p->desc is only set for SPIs > assigned to a guest. If desc is NULL, then it will fault a bit later. > If someone doesn't use this API to route an IRQ then it's his fault. > > Hence, this as been checked in route_irq_guest. I don't think we should > bother to check again. > > >>> > >>>> @@ -479,6 +480,53 @@ out: > >>>> 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; > >>>> + > >>>> + if ( virq >= vgic_num_irqs(d) ) > >>>> + return -EINVAL; > >>>> + > >>>> + p = irq_to_pending(d->vcpu[0], virq); > >>>> + if ( !p->desc ) > >>>> + return -EINVAL; > >>> > >>> Same here: call vgic_get_target_vcpu. > >>> Also if this function is supposed to work only with SPIs, you should add > >>> a comment or explicitly check for it. > >> > >> route_irq_to_guest already check if we are able to route an IRQ or not. > >> For non-SPIs the function will bailout. > >> > >> So, here, it's impossible to have p->desc set to another value than NULL > >> for non-SPIs. > >> > >> Or Xen is buggy will likely fail in another place. > > > > If you do: > > > > p = irq_to_pending(d->vcpu[0], virq); > > > > you are actually introducing more code that cannot cope with non-SGIs. > > So you should either: > > > > 1) esplicitely check for it (add an ASSERT) > > Already done in route_irq_to_guest. I don't think we have to add yet > another assert here. > > > 2) replace it with code that can cope with non-SGIs, such us > > irq_to_pending(vgic_get_target_vcpu(d->vcpu[0], virq), virq) > > This code won't cope with non-SGIs (here PPIs). As PPIs have an irq_desc > per CPU we will have to loop on every VCPU to unmap it. > > But I doubt we will have PPIs in future, there is more issues to handle > (such as the number of VCPUs doesn't match the number of physical CPUs). In that case, why not call the two functions release_guest_spi and gic_remove_spi_from_guest?
On 08/07/2014 04:36 PM, Stefano Stabellini wrote: >> But I doubt we will have PPIs in future, there is more issues to handle >> (such as the number of VCPUs doesn't match the number of physical CPUs). > > In that case, why not call the two functions release_guest_spi and > gic_remove_spi_from_guest? Because the 2 functions to route the irq are called route_irq_to_guest and gic_route_irq_to_guest.
On Thu, 7 Aug 2014, Julien Grall wrote: > On 08/07/2014 04:36 PM, Stefano Stabellini wrote: > >> But I doubt we will have PPIs in future, there is more issues to handle > >> (such as the number of VCPUs doesn't match the number of physical CPUs). > > > > In that case, why not call the two functions release_guest_spi and > > gic_remove_spi_from_guest? > > Because the 2 functions to route the irq are called route_irq_to_guest > and gic_route_irq_to_guest. I would prefer to avoid introducing more functions that look like they can handle any irqs but actually they cannot by design. I would be OK with the asymmetry in function names. We could also turn route_irq_to_guest into route_spi_to_guest and gic_route_irq_to_guest into gic_route_spi_to_guest.
On 08/07/2014 05:31 PM, Stefano Stabellini wrote: > On Thu, 7 Aug 2014, Julien Grall wrote: >> On 08/07/2014 04:36 PM, Stefano Stabellini wrote: >>>> But I doubt we will have PPIs in future, there is more issues to handle >>>> (such as the number of VCPUs doesn't match the number of physical CPUs). >>> >>> In that case, why not call the two functions release_guest_spi and >>> gic_remove_spi_from_guest? >> >> Because the 2 functions to route the irq are called route_irq_to_guest >> and gic_route_irq_to_guest. > > I would prefer to avoid introducing more functions that look like they > can handle any irqs but actually they cannot by design. That why Xen checks the IRQ number at the beginning of route_irq_to_guest. > I would be OK with the asymmetry in function names. We could also turn > route_irq_to_guest into route_spi_to_guest and gic_route_irq_to_guest > into gic_route_spi_to_guest. Those functions will be used for MSI sooner or later. I would prefer to keep the current name and add an ASSERT.
On Thu, 7 Aug 2014, Julien Grall wrote: > On 08/07/2014 05:31 PM, Stefano Stabellini wrote: > > On Thu, 7 Aug 2014, Julien Grall wrote: > >> On 08/07/2014 04:36 PM, Stefano Stabellini wrote: > >>>> But I doubt we will have PPIs in future, there is more issues to handle > >>>> (such as the number of VCPUs doesn't match the number of physical CPUs). > >>> > >>> In that case, why not call the two functions release_guest_spi and > >>> gic_remove_spi_from_guest? > >> > >> Because the 2 functions to route the irq are called route_irq_to_guest > >> and gic_route_irq_to_guest. > > > > I would prefer to avoid introducing more functions that look like they > > can handle any irqs but actually they cannot by design. > > That why Xen checks the IRQ number at the beginning of route_irq_to_guest. > > > I would be OK with the asymmetry in function names. We could also turn > > route_irq_to_guest into route_spi_to_guest and gic_route_irq_to_guest > > into gic_route_spi_to_guest. > > Those functions will be used for MSI sooner or later. I would prefer to > keep the current name and add an ASSERT. OK then, I can settle for that
On Thu, 2014-08-07 at 17:39 +0100, Stefano Stabellini wrote: > On Thu, 7 Aug 2014, Julien Grall wrote: > > On 08/07/2014 05:31 PM, Stefano Stabellini wrote: > > > On Thu, 7 Aug 2014, Julien Grall wrote: > > >> On 08/07/2014 04:36 PM, Stefano Stabellini wrote: > > >>>> But I doubt we will have PPIs in future, there is more issues to handle > > >>>> (such as the number of VCPUs doesn't match the number of physical CPUs). > > >>> > > >>> In that case, why not call the two functions release_guest_spi and > > >>> gic_remove_spi_from_guest? > > >> > > >> Because the 2 functions to route the irq are called route_irq_to_guest > > >> and gic_route_irq_to_guest. > > > > > > I would prefer to avoid introducing more functions that look like they > > > can handle any irqs but actually they cannot by design. > > > > That why Xen checks the IRQ number at the beginning of route_irq_to_guest. > > > > > I would be OK with the asymmetry in function names. We could also turn > > > route_irq_to_guest into route_spi_to_guest and gic_route_irq_to_guest > > > into gic_route_spi_to_guest. > > > > Those functions will be used for MSI sooner or later. I would prefer to > > keep the current name and add an ASSERT. > > OK then, I can settle for that Does that mean you are happy with the patch as is or were there other changes you wanted?
On Tue, 9 Sep 2014, Ian Campbell wrote: > On Thu, 2014-08-07 at 17:39 +0100, Stefano Stabellini wrote: > > On Thu, 7 Aug 2014, Julien Grall wrote: > > > On 08/07/2014 05:31 PM, Stefano Stabellini wrote: > > > > On Thu, 7 Aug 2014, Julien Grall wrote: > > > >> On 08/07/2014 04:36 PM, Stefano Stabellini wrote: > > > >>>> But I doubt we will have PPIs in future, there is more issues to handle > > > >>>> (such as the number of VCPUs doesn't match the number of physical CPUs). > > > >>> > > > >>> In that case, why not call the two functions release_guest_spi and > > > >>> gic_remove_spi_from_guest? > > > >> > > > >> Because the 2 functions to route the irq are called route_irq_to_guest > > > >> and gic_route_irq_to_guest. > > > > > > > > I would prefer to avoid introducing more functions that look like they > > > > can handle any irqs but actually they cannot by design. > > > > > > That why Xen checks the IRQ number at the beginning of route_irq_to_guest. > > > > > > > I would be OK with the asymmetry in function names. We could also turn > > > > route_irq_to_guest into route_spi_to_guest and gic_route_irq_to_guest > > > > into gic_route_spi_to_guest. > > > > > > Those functions will be used for MSI sooner or later. I would prefer to > > > keep the current name and add an ASSERT. > > > > OK then, I can settle for that > > Does that mean you are happy with the patch as is or were there other > changes you wanted? The latter. I am waiting for an update on this patch with the ASSERTs.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 8ef8764..22f331a 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -144,6 +144,42 @@ void gic_route_irq_to_guest(struct domain *d, unsigned int virq, p->desc = desc; } +/* 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 pending_irq *p = irq_to_pending(d->vcpu[0], virq); + + ASSERT(spin_is_locked(&desc->lock)); + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); + ASSERT(p->desc == desc); + + /* 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: + desc->handler = &no_irq_type; + p->desc = NULL; + + 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 7eeb8dd..ba33571 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -292,6 +292,7 @@ void release_irq(unsigned int irq, const void *dev_id) if ( !desc->action ) { desc->handler->shutdown(desc); + clear_bit(_IRQ_GUEST, &desc->status); } @@ -479,6 +480,53 @@ out: 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; + + if ( 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 a037ecc..2a5fc18 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -118,6 +118,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_WARNING, "d%u: Failed to release virq %u ret = %d\n", + d->domain_id, p->irq, ret); + } + } + xfree(d->arch.vgic.shared_irqs); xfree(d->arch.vgic.pending_irqs); } diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 89816cd..46d7fd6 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -201,6 +201,10 @@ extern void gic_route_irq_to_guest(struct domain *, unsigned int virq, const cpumask_t *cpu_mask, unsigned int priority); +/* Remove an IRQ passthrough to a guest */ +int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, + struct irq_desc *desc); + extern void gic_inject(void); extern void gic_clear_pending_irqs(struct vcpu *v); extern int gic_events_need_delivery(void); diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h index a7174f3..d25e1a1 100644 --- a/xen/include/asm-arm/irq.h +++ b/xen/include/asm-arm/irq.h @@ -44,6 +44,8 @@ void init_secondary_IRQ(void); int route_irq_to_guest(struct domain *d, unsigned int virq, unsigned int irq, const char *devname); +int release_guest_irq(struct domain *d, unsigned int irq); + void arch_move_irqs(struct vcpu *v); /* Set IRQ type for an SPI */
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 <julien.grall@linaro.org> --- Changes in v2: - Drop the desc->handler = &no_irq_type in release_irq as it's buggy the IRQ is routed to Xen - Add release_guest_irq and gic_remove_guest_irq --- xen/arch/arm/gic.c | 36 ++++++++++++++++++++++++++++++++++ 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, 106 insertions(+)