On Thu, Nov 22, 2012 at 5:57 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > On 21/11/12 21:46, Christoffer Dall wrote: >> On Wed, Nov 21, 2012 at 1:15 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: >>> While the vgic code doesn't really care about an edge interrupt >>> having its line "lowered", doing so makes more sense from a timer >>> perspective, and clearly indicates that this is an edge-triggered >>> interrupt. >>> >> >> I am extremely confused. >> >> The architected timers on an A15 is always level interrupt, correct? >> We present an A15 to the guest so the guest kernel expects that this >> is the case and that's what the emulated cfg register will contain. So >> I don't understand your reference to the edge thingy above? > > The cfg register exposes an edge interrupt to the guest. And for all > purpose, the fact that it is treated as an edge interrupt is irrelevant > to the guest, as it makes no observable difference. The A15 TRM clearly specifies the timer interrupts as being level-triggered active-LOW, but OK, I see that you ignore writes to the CFG register for PPIs, and force them to be always edge-triggered, so I guess we're not exposing the same GIC to the guest that would be there on an A15...? This may also deserve a comment in the code. > > On the host, it makes a hell of a difference (vgic maintenance interrupts). > >> If I understand the patch correctly, this relates to the fact that the >> timer interrupts are active-LOW, and then I get all scared, because I >> think I always assumed that when we set level==1 in the kvm_irq_level >> struct we mean "raise an interrupt", regardless of wether the line is >> active-LOW or active-HIGH. Otherwise we will never inject an interrupt >> that is active-LOW when it's active, but always inject that interrupt >> when it's inactive. > > It doesn't matter! We don't really inject a level, we inject an > interrupt. An event. The fact that we use the same internal API is pure > convenience. Why do you say it doesn't matter? For en edge-triggered interrupt, vgic_update_irq_state short-circuits on (is_edge && !level). ???? And by the way, that API relies on the fact that deep in the VGIC code PPIs are programmed to be edge-triggered and it's assumed that the arch. timers are always PPIs, which may be a consequence of the architecture, but it is not obvious for anyone else ever reading this code. > >>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >>> --- >>> arch/arm/kvm/timer.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/arch/arm/kvm/timer.c b/arch/arm/kvm/timer.c >>> index a241298..b9e573f 100644 >>> --- a/arch/arm/kvm/timer.c >>> +++ b/arch/arm/kvm/timer.c >>> @@ -42,6 +42,9 @@ static void kvm_timer_inject_irq(struct kvm_vcpu *vcpu) >>> kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, >>> vcpu->arch.timer_cpu.irq->irq, >>> vcpu->arch.timer_cpu.irq->level); >>> + kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, >>> + vcpu->arch.timer_cpu.irq->irq, >>> + !vcpu->arch.timer_cpu.irq->level); >> >> regardless of the above, this looks super screwy and must be explained >> with a comment. >> >> If we start supporting a core where the timer interrupt is edge-based, >> than we never raise a timer interrupt, which is just plain weird. > > You assume we care about the hardware timer. We don't. It is just a > convenient and standardized interface to let the guest request an event. > The interrupt is handled on the host, where the interrupt polarity > matters. What we inject into the guest is a different story, and I don't > think it matters. > I don't understand what you say I assume here, at all. (I'm not saying that you're wrong, it may very well be that I'm getting the whole thing wrong, but then walk me through the code and explain me why this makes sense. And please put a comment explaining other people who are as stupid as me why we do this, because it certainly looks unintuitive.) Having that negation is weird, why not just change the level field in reset.c to 0. But I think it should be 1, and not negated. If the idea is simply that that you always want to use the kvm_vgic_inject_irq interface on the host, and always make it fire, then you want to make sure that vgic_update_irq_state always reads this specific interrupt as edge triggered (which you do, although it's not obvious) and indeed inject level=1. -Christoffer _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
