| Message ID | 20180209143937.28866-10-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 09/02/18 14:38, Andre Przywara wrote: > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 5f47aa84a9..2fc6e19625 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -285,7 +285,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) > vgic_remove_irq_from_queues(old, p); > irq_set_affinity(p->desc, cpumask_of(new->processor)); > spin_unlock_irqrestore(&old->arch.vgic.lock, flags); > - vgic_vcpu_inject_irq(new, irq); > + vgic_inject_irq(new->domain, new, irq, true); > return true; > } > /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING > @@ -444,7 +444,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, > sgir, target->list); > continue; > } > - vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq); > + vgic_inject_irq(d, d->vcpu[vcpuid], virq, true); > } > break; > case SGI_TARGET_OTHERS: > @@ -453,12 +453,12 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, > { > if ( i != current->vcpu_id && d->vcpu[i] != NULL && > is_vcpu_online(d->vcpu[i]) ) > - vgic_vcpu_inject_irq(d->vcpu[i], virq); > + vgic_inject_irq(d, d->vcpu[i], virq, true); > } > break; > case SGI_TARGET_SELF: > perfc_incr(vgic_sgi_self); > - vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq); > + vgic_inject_irq(d, current, virq, true); > break; > default: > gprintk(XENLOG_WARNING, > @@ -518,13 +518,29 @@ void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p) > gic_remove_from_lr_pending(v, p); > } > > -void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) > +int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq, > + bool level) Looking at the code after the series has been applied, no one is caring about the return value of vgic_inject_irq. So what is the rationale behind changing the return type from void to int? Cheers,
Hi, On 12/02/18 11:15, Julien Grall wrote: > Hi Andre, > > On 09/02/18 14:38, Andre Przywara wrote: >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 5f47aa84a9..2fc6e19625 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -285,7 +285,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct >> vcpu *new, unsigned int irq) >> vgic_remove_irq_from_queues(old, p); >> irq_set_affinity(p->desc, cpumask_of(new->processor)); >> spin_unlock_irqrestore(&old->arch.vgic.lock, flags); >> - vgic_vcpu_inject_irq(new, irq); >> + vgic_inject_irq(new->domain, new, irq, true); >> return true; >> } >> /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING >> @@ -444,7 +444,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, >> enum gic_sgi_mode irqmode, >> sgir, target->list); >> continue; >> } >> - vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq); >> + vgic_inject_irq(d, d->vcpu[vcpuid], virq, true); >> } >> break; >> case SGI_TARGET_OTHERS: >> @@ -453,12 +453,12 @@ bool vgic_to_sgi(struct vcpu *v, register_t >> sgir, enum gic_sgi_mode irqmode, >> { >> if ( i != current->vcpu_id && d->vcpu[i] != NULL && >> is_vcpu_online(d->vcpu[i]) ) >> - vgic_vcpu_inject_irq(d->vcpu[i], virq); >> + vgic_inject_irq(d, d->vcpu[i], virq, true); >> } >> break; >> case SGI_TARGET_SELF: >> perfc_incr(vgic_sgi_self); >> - vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq); >> + vgic_inject_irq(d, current, virq, true); >> break; >> default: >> gprintk(XENLOG_WARNING, >> @@ -518,13 +518,29 @@ void vgic_remove_irq_from_queues(struct vcpu *v, >> struct pending_irq *p) >> gic_remove_from_lr_pending(v, p); >> } >> -void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) >> +int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq, >> + bool level) > > Looking at the code after the series has been applied, no one is caring > about the return value of vgic_inject_irq. So what is the rationale > behind changing the return type from void to int? The KVM version returns an error value, in particular when: - the VGIC has not been initialized yet - we can't determine the VCPU for a private interrupt - the interrupt ID is invalid (SPI beyond limit, not mapped LPI) In the moment it's not very useful for Xen: the first two conditions don't really happen, consequently I removed those checks. But the third check may become interesting once we get LPIs. Also since Xen currently uses a void prototype for injection, *this* patch *now* doesn't exploit the newly gained possibility of properly handling errors. From briefly checking all the users, all of them seem to be in void functions, so indeed an error return is not very useful. The reasons I kept it in was to allow introduction of checks later. I think having a function returning an error where some users ignore this is better than the other way round. So of course I can easily make this void, but I wonder what we do in those cases where the SPI is not valid, for instance? Shall we print some (rate-limited) warning? Cheers, Andre.
On 12/02/18 11:59, Andre Przywara wrote: > Hi, Hi Andre, > On 12/02/18 11:15, Julien Grall wrote: >> Hi Andre, >> >> On 09/02/18 14:38, Andre Przywara wrote: >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>> index 5f47aa84a9..2fc6e19625 100644 >>> --- a/xen/arch/arm/vgic.c >>> +++ b/xen/arch/arm/vgic.c >>> @@ -285,7 +285,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct >>> vcpu *new, unsigned int irq) >>> vgic_remove_irq_from_queues(old, p); >>> irq_set_affinity(p->desc, cpumask_of(new->processor)); >>> spin_unlock_irqrestore(&old->arch.vgic.lock, flags); >>> - vgic_vcpu_inject_irq(new, irq); >>> + vgic_inject_irq(new->domain, new, irq, true); >>> return true; >>> } >>> /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING >>> @@ -444,7 +444,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, >>> enum gic_sgi_mode irqmode, >>> sgir, target->list); >>> continue; >>> } >>> - vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq); >>> + vgic_inject_irq(d, d->vcpu[vcpuid], virq, true); >>> } >>> break; >>> case SGI_TARGET_OTHERS: >>> @@ -453,12 +453,12 @@ bool vgic_to_sgi(struct vcpu *v, register_t >>> sgir, enum gic_sgi_mode irqmode, >>> { >>> if ( i != current->vcpu_id && d->vcpu[i] != NULL && >>> is_vcpu_online(d->vcpu[i]) ) >>> - vgic_vcpu_inject_irq(d->vcpu[i], virq); >>> + vgic_inject_irq(d, d->vcpu[i], virq, true); >>> } >>> break; >>> case SGI_TARGET_SELF: >>> perfc_incr(vgic_sgi_self); >>> - vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq); >>> + vgic_inject_irq(d, current, virq, true); >>> break; >>> default: >>> gprintk(XENLOG_WARNING, >>> @@ -518,13 +518,29 @@ void vgic_remove_irq_from_queues(struct vcpu *v, >>> struct pending_irq *p) >>> gic_remove_from_lr_pending(v, p); >>> } >>> -void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) >>> +int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq, >>> + bool level) >> >> Looking at the code after the series has been applied, no one is caring >> about the return value of vgic_inject_irq. So what is the rationale >> behind changing the return type from void to int? > > The KVM version returns an error value, in particular when: > - the VGIC has not been initialized yet > - we can't determine the VCPU for a private interrupt > - the interrupt ID is invalid (SPI beyond limit, not mapped LPI) > In the moment it's not very useful for Xen: the first two conditions > don't really happen, consequently I removed those checks. But the third > check may become interesting once we get LPIs. Also since Xen currently > uses a void prototype for injection, *this* patch *now* doesn't exploit > the newly gained possibility of properly handling errors. From briefly > checking all the users, all of them seem to be in void functions, so > indeed an error return is not very useful. > The reasons I kept it in was to allow introduction of checks later. I > think having a function returning an error where some users ignore this > is better than the other way round. I don't think it is much better. This is a way to expose yet another security issue because the return is not correctly checked (see XSA-246 for instance). Any return value should be checked or have a comment explaining why it is fine. > > So of course I can easily make this void, but I wonder what we do in > those cases where the SPI is not valid, for instance? Shall we print > some (rate-limited) warning? I can understand why KVM needs such interface as the interrupt controller may be emulated QEMU. But I can't see why a SPI would not be valid in Xen context (except programming error). So could give an example? What would you expect the caller to do on error? Except printing an error message? Cheers,
Hi, On 12/02/18 12:19, Julien Grall wrote: > > > On 12/02/18 11:59, Andre Przywara wrote: >> Hi, > > Hi Andre, > >> On 12/02/18 11:15, Julien Grall wrote: >>> Hi Andre, >>> >>> On 09/02/18 14:38, Andre Przywara wrote: >>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>>> index 5f47aa84a9..2fc6e19625 100644 >>>> --- a/xen/arch/arm/vgic.c >>>> +++ b/xen/arch/arm/vgic.c >>>> @@ -285,7 +285,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct >>>> vcpu *new, unsigned int irq) >>>> vgic_remove_irq_from_queues(old, p); >>>> irq_set_affinity(p->desc, cpumask_of(new->processor)); >>>> spin_unlock_irqrestore(&old->arch.vgic.lock, flags); >>>> - vgic_vcpu_inject_irq(new, irq); >>>> + vgic_inject_irq(new->domain, new, irq, true); >>>> return true; >>>> } >>>> /* if the IRQ is in a GICH_LR register, set >>>> GIC_IRQ_GUEST_MIGRATING >>>> @@ -444,7 +444,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, >>>> enum gic_sgi_mode irqmode, >>>> sgir, target->list); >>>> continue; >>>> } >>>> - vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq); >>>> + vgic_inject_irq(d, d->vcpu[vcpuid], virq, true); >>>> } >>>> break; >>>> case SGI_TARGET_OTHERS: >>>> @@ -453,12 +453,12 @@ bool vgic_to_sgi(struct vcpu *v, register_t >>>> sgir, enum gic_sgi_mode irqmode, >>>> { >>>> if ( i != current->vcpu_id && d->vcpu[i] != NULL && >>>> is_vcpu_online(d->vcpu[i]) ) >>>> - vgic_vcpu_inject_irq(d->vcpu[i], virq); >>>> + vgic_inject_irq(d, d->vcpu[i], virq, true); >>>> } >>>> break; >>>> case SGI_TARGET_SELF: >>>> perfc_incr(vgic_sgi_self); >>>> - vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq); >>>> + vgic_inject_irq(d, current, virq, true); >>>> break; >>>> default: >>>> gprintk(XENLOG_WARNING, >>>> @@ -518,13 +518,29 @@ void vgic_remove_irq_from_queues(struct vcpu *v, >>>> struct pending_irq *p) >>>> gic_remove_from_lr_pending(v, p); >>>> } >>>> -void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) >>>> +int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int >>>> virq, >>>> + bool level) >>> >>> Looking at the code after the series has been applied, no one is caring >>> about the return value of vgic_inject_irq. So what is the rationale >>> behind changing the return type from void to int? >> >> The KVM version returns an error value, in particular when: >> - the VGIC has not been initialized yet >> - we can't determine the VCPU for a private interrupt >> - the interrupt ID is invalid (SPI beyond limit, not mapped LPI) >> In the moment it's not very useful for Xen: the first two conditions >> don't really happen, consequently I removed those checks. But the third >> check may become interesting once we get LPIs. Also since Xen currently >> uses a void prototype for injection, *this* patch *now* doesn't exploit >> the newly gained possibility of properly handling errors. From briefly >> checking all the users, all of them seem to be in void functions, so >> indeed an error return is not very useful. >> The reasons I kept it in was to allow introduction of checks later. I >> think having a function returning an error where some users ignore this >> is better than the other way round. > > I don't think it is much better. This is a way to expose yet another > security issue because the return is not correctly checked (see XSA-246 > for instance). Any return value should be checked or have a comment > explaining why it is fine. > >> >> So of course I can easily make this void, but I wonder what we do in >> those cases where the SPI is not valid, for instance? Shall we print >> some (rate-limited) warning? > > I can understand why KVM needs such interface as the interrupt > controller may be emulated QEMU. It is also that interrupts from emulated devices are injected from QEMU in userland, and the ioctl used for that can and will return an error. So we can propagate this condition to the device. If and what the devices does with that information, is another question, though, but out of scope for KVM. > But I can't see why a SPI would not be > valid in Xen context (except programming error). So could give an example? In KVM the KVM_IRQ_LINE ioctl allows to inject an arbitrary number, so checking this and returning an error is natural and mandatory. > What would you expect the caller to do on error? Except printing an > error message? I don't know either. Comparing this to hardware, an IRQ is usually fire-and-forget (separating the interrupt line from the interrupt state here), so a device doesn't really handle the case when an IRQ does not make it through (it can't know easily anyway). However the whole state machine might get busted in the process (if no one lowers the line, for instance). So looking at this printing a message looks like the best choice. I checked all users of vgic_inject_irq(), at the moment all IRQ numbers passed in look safe: they are either hardcoded (timer, evtchn) or validated before (hardware IRQs, when they are tied to a virtual IRQ). So indeed we *should* never see an invalid IRQ number, at the moment. I need to check how this changes with the ITS, though. So we could change the prototype (back) to void, but print some error message if the vgic_get_irq() call fails within vgic_inject_irq(). Cheers, Andre.
Hi, On 12/02/18 14:24, Andre Przywara wrote: >> What would you expect the caller to do on error? Except printing an >> error message? > > I don't know either. Comparing this to hardware, an IRQ is usually > fire-and-forget (separating the interrupt line from the interrupt state > here), so a device doesn't really handle the case when an IRQ does not > make it through (it can't know easily anyway). However the whole state > machine might get busted in the process (if no one lowers the line, for > instance). > So looking at this printing a message looks like the best choice. > > I checked all users of vgic_inject_irq(), at the moment all IRQ numbers > passed in look safe: they are either hardcoded (timer, evtchn) or > validated before (hardware IRQs, when they are tied to a virtual IRQ). > So indeed we *should* never see an invalid IRQ number, at the moment. > I need to check how this changes with the ITS, though. > > So we could change the prototype (back) to void, but print some error > message if the vgic_get_irq() call fails within vgic_inject_irq(). Sounds good to me. Make sure to have those message using the log level guest debug message. I might even be tempt to use dgprintk(...) here so they get dropped in non-debug build. Cheers,
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 9ad4cd0a6e..e76cfdfe83 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -952,14 +952,14 @@ void vcpu_mark_events_pending(struct vcpu *v) if ( already_pending ) return; - vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq); + vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, true); } /* The ARM spec declares that even if local irqs are masked in * the CPSR register, an irq should wake up a cpu from WFI anyway. * For this reason we need to check for irqs that need delivery, * ignoring the CPSR register, *after* calling SCHEDOP_block to - * avoid races with vgic_vcpu_inject_irq. + * avoid races with vgic_inject_irq. */ void vcpu_block_unless_event_pending(struct vcpu *v) { diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c index 84582157b8..efd5cd62fb 100644 --- a/xen/arch/arm/gic-v3-lpi.c +++ b/xen/arch/arm/gic-v3-lpi.c @@ -153,7 +153,7 @@ void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq) if ( vcpu_id >= d->max_vcpus ) return; - vgic_vcpu_inject_irq(d->vcpu[vcpu_id], virq); + vgic_inject_irq(d, d->vcpu[vcpu_id], virq, true); } /* diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 29af10e82c..aa4e832cae 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -225,7 +225,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) * The irq cannot be a PPI, we only support delivery of SPIs to * guests. */ - vgic_vcpu_inject_spi(info->d, info->virq); + vgic_inject_irq(info->d, NULL, info->virq, true); goto out_no_end; } diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index 36f640f0c1..c11fcfeadd 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -260,7 +260,7 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0); WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0); - vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq); + vgic_inject_irq(current->domain, current, current->arch.virt_timer.irq, true); } /* diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 5f47aa84a9..2fc6e19625 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -285,7 +285,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) vgic_remove_irq_from_queues(old, p); irq_set_affinity(p->desc, cpumask_of(new->processor)); spin_unlock_irqrestore(&old->arch.vgic.lock, flags); - vgic_vcpu_inject_irq(new, irq); + vgic_inject_irq(new->domain, new, irq, true); return true; } /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING @@ -444,7 +444,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, sgir, target->list); continue; } - vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq); + vgic_inject_irq(d, d->vcpu[vcpuid], virq, true); } break; case SGI_TARGET_OTHERS: @@ -453,12 +453,12 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, { if ( i != current->vcpu_id && d->vcpu[i] != NULL && is_vcpu_online(d->vcpu[i]) ) - vgic_vcpu_inject_irq(d->vcpu[i], virq); + vgic_inject_irq(d, d->vcpu[i], virq, true); } break; case SGI_TARGET_SELF: perfc_incr(vgic_sgi_self); - vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq); + vgic_inject_irq(d, current, virq, true); break; default: gprintk(XENLOG_WARNING, @@ -518,13 +518,29 @@ void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p) gic_remove_from_lr_pending(v, p); } -void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) +int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq, + bool level) { uint8_t priority; struct pending_irq *iter, *n; unsigned long flags; bool running; + /* + * For edge triggered interrupts we always ignore a "falling edge". + * For level triggered interrupts we shouldn't, but do anyways. + */ + if ( !level ) + return 0; + + if ( !v ) + { + /* The IRQ needs to be an SPI if no vCPU is specified. */ + ASSERT(virq >= 32 && virq <= vgic_num_irqs(d)); + + v = vgic_get_target_vcpu(d->vcpu[0], virq); + }; + spin_lock_irqsave(&v->arch.vgic.lock, flags); n = irq_to_pending(v, virq); @@ -532,14 +548,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) if ( unlikely(!n) ) { spin_unlock_irqrestore(&v->arch.vgic.lock, flags); - return; + return 0; } /* vcpu offline */ if ( test_bit(_VPF_down, &v->pause_flags) ) { spin_unlock_irqrestore(&v->arch.vgic.lock, flags); - return; + return 0; } set_bit(GIC_IRQ_GUEST_QUEUED, &n->status); @@ -576,22 +592,13 @@ out: perfc_incr(vgic_cross_cpu_intr_inject); smp_send_event_check_mask(cpumask_of(v->processor)); } -} - -void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq) -{ - struct vcpu *v; - /* the IRQ needs to be an SPI */ - ASSERT(virq >= 32 && virq <= vgic_num_irqs(d)); - - v = vgic_get_target_vcpu(d->vcpu[0], virq); - vgic_vcpu_inject_irq(v, virq); + return 0; } void arch_evtchn_inject(struct vcpu *v) { - vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq); + vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, true); } bool vgic_evtchn_irq_pending(struct vcpu *v) diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index 7788c2fc32..5dcf4bec18 100644 --- a/xen/arch/arm/vpl011.c +++ b/xen/arch/arm/vpl011.c @@ -68,7 +68,7 @@ static void vpl011_update_interrupt_status(struct domain *d) * status bit has been set since the last time. */ if ( uartmis & ~vpl011->shadow_uartmis ) - vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI); + vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true); vpl011->shadow_uartmis = uartmis; } diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c index f52a723a5f..8164f6c7f1 100644 --- a/xen/arch/arm/vtimer.c +++ b/xen/arch/arm/vtimer.c @@ -46,7 +46,7 @@ static void phys_timer_expired(void *data) if ( !(t->ctl & CNTx_CTL_MASK) ) { perfc_incr(vtimer_phys_inject); - vgic_vcpu_inject_irq(t->v, t->irq); + vgic_inject_irq(t->v->domain, t->v, t->irq, true); } else perfc_incr(vtimer_phys_masked); @@ -56,7 +56,7 @@ static void virt_timer_expired(void *data) { struct vtimer *t = data; t->ctl |= CNTx_CTL_MASK; - vgic_vcpu_inject_irq(t->v, t->irq); + vgic_inject_irq(t->v->domain, t->v, t->irq, true); perfc_incr(vtimer_virt_inject); } diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index d03298e12c..b75fdeb068 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -202,8 +202,8 @@ extern int domain_vgic_init(struct domain *d, unsigned int nr_spis); extern void domain_vgic_free(struct domain *d); extern int vcpu_vgic_init(struct vcpu *v); extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq); -extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq); -extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq); +extern int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq, + bool level); extern void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p); extern void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p); extern void vgic_clear_pending_irqs(struct vcpu *v);
At the moment vgic_vcpu_inject_irq() is the interface for Xen internal code and virtual devices to inject IRQs into a guest. This interface has two shortcomings: 1) It requires a VCPU pointer, which we may not know (and don't need!) for shared interrupts. A second function (vgic_vcpu_inject_spi()), was there to work around this issue. 2) This interface only really supports edge triggered IRQs, which is what the Xen VGIC emulates only anyway. However this needs to and will change, so we need to add the desired level (high or low) to the interface. This replaces the existing injection call (taking a VCPU and and IRQ parameter) with a new one, taking domain, VCPU, IRQ and level parameters. The VCPU can be NULL in case we don't know and don't care. We change all call sites to use this new interface. This still doesn't give us the missing level IRQ handling, but at least prepares the callers to do the right thing later automatically. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/domain.c | 4 ++-- xen/arch/arm/gic-v3-lpi.c | 2 +- xen/arch/arm/irq.c | 2 +- xen/arch/arm/time.c | 2 +- xen/arch/arm/vgic.c | 43 +++++++++++++++++++++++++------------------ xen/arch/arm/vpl011.c | 2 +- xen/arch/arm/vtimer.c | 4 ++-- xen/include/asm-arm/vgic.h | 4 ++-- 8 files changed, 35 insertions(+), 28 deletions(-)