| Message ID | 1418395392-30460-4-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Ian, On 13/01/15 15:55, Ian Campbell wrote: > On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote: >> This help for guest interrupts debugging. If the vIRQ is not allocate, >> this means that nothing is wired to it. > > Should we short circuit the rest of the enable operation for this IRQ > then? i.e. implement such writes as ignored, e.g. not reflect it in > reads of ISENABLER etc. > > What (if anything) does the GIC spec have to say on the subject? "A register bit corresponding to an unimplemented interrupt is RAZ/WI." The goal of this print was mostly for debugging physical IRQ routed to a guest. I could extend to ignore write to any register that should be RAZ/WI for this specific interrupt. But, I will have to think about possible race condition with the hypercall to route a physical IRQ to the guest (see [1] and [2]). The vIRQ is reserved before the physical IRQ is effectively routed. So a guest may enable the vIRQ before this time lapse. Though, the patch [2] protected for a such case. Not sure if we should take care of a such case. Regards,
On 14/01/15 12:28, Ian Campbell wrote: > On Tue, 2015-01-13 at 20:33 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 13/01/15 15:55, Ian Campbell wrote: >>> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote: >>>> This help for guest interrupts debugging. If the vIRQ is not allocate, >>>> this means that nothing is wired to it. >>> >>> Should we short circuit the rest of the enable operation for this IRQ >>> then? i.e. implement such writes as ignored, e.g. not reflect it in >>> reads of ISENABLER etc. >>> >>> What (if anything) does the GIC spec have to say on the subject? >> >> "A register bit corresponding to an unimplemented interrupt is RAZ/WI." >> >> The goal of this print was mostly for debugging physical IRQ routed to a >> guest. >> >> I could extend to ignore write to any register that should be RAZ/WI for >> this specific interrupt. > > Since those are the defined semantics I think that is the best thing to > do. Ok. I will look at it to see how we can implement it. >> But, I will have to think about possible race condition with the >> hypercall to route a physical IRQ to the guest (see [1] and [2]). >> >> The vIRQ is reserved before the physical IRQ is effectively routed. So >> a guest may enable the vIRQ before this time lapse. Though, the patch >> [2] protected for a such case. >> >> Not sure if we should take care of a such case. > > I don't think so, that routing hypercall ought to be happening strictly > before any hotplug event visible to the guest causes it to think there > is a device (vacuously true for coldplug too), so the guest should have > no expectation of being able to do anything with the irq in question. Good, one less headache :) Regards,
Hi Ian, On 14/01/15 12:42, Julien Grall wrote: > On 14/01/15 12:28, Ian Campbell wrote: >> On Tue, 2015-01-13 at 20:33 +0000, Julien Grall wrote: >>> On 13/01/15 15:55, Ian Campbell wrote: >>>> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote: >>>>> This help for guest interrupts debugging. If the vIRQ is not allocate, >>>>> this means that nothing is wired to it. >>>> >>>> Should we short circuit the rest of the enable operation for this IRQ >>>> then? i.e. implement such writes as ignored, e.g. not reflect it in >>>> reads of ISENABLER etc. >>>> >>>> What (if anything) does the GIC spec have to say on the subject? >>> >>> "A register bit corresponding to an unimplemented interrupt is RAZ/WI." >>> >>> The goal of this print was mostly for debugging physical IRQ routed to a >>> guest. >>> >>> I could extend to ignore write to any register that should be RAZ/WI for >>> this specific interrupt. >> >> Since those are the defined semantics I think that is the best thing to >> do. > > Ok. I will look at it to see how we can implement it. So I looked to the code. It may need some rework to effectively implement most of registers bits RAZ/WI when the interrupt doesn't exist. As this series is required for the ACPI series, I suggest to defer this work in a follow-up series. Regards,
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index dbfc259..719cb9f 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -282,6 +282,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) gic_raise_guest_irq(v_target, irq, p->priority); spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); + + if ( !test_bit(irq, d->arch.vgic.allocated_irqs) ) + gdprintk(XENLOG_DEBUG, "vIRQ %u is not allocated\n", irq); + if ( p->desc != NULL ) { irq_set_affinity(p->desc, cpumask_of(v_target->processor));
This help for guest interrupts debugging. If the vIRQ is not allocate, this means that nothing is wired to it. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/vgic.c | 4 ++++ 1 file changed, 4 insertions(+)