On Sat, Nov 24, 2012 at 2:33 PM, Christoffer Dall
<c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> 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.
>
If I read the code correctly, this comment should probably be added:
diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
index f697c14..960bbc9 100644
--- a/arch/arm/kvm/vgic.c
+++ b/arch/arm/kvm/vgic.c
@@ -35,10 +35,11 @@
* - At any time, the dist->irq_pending_on_cpu is the oracle that knows if
* something is pending
* - VGIC pending interrupts are stored on the vgic.irq_state vgic
- * bitmap (this bitmap is updated by both user land ioctls and guest
- * mmio ops) and indicate the 'wire' state.
- * - Every time the bitmap changes, the irq_pending_on_cpu oracle is
- * recalculated
+ * bitmap (this bitmap is updated by both user land ioctls, guest
+ * mmio ops, and other in-kernel peripherals such as the arch. timers) and
+ * indicate the 'wire' state.
+ * - Every time the irq_state bitmap changes, the irq_pending_on_cpu oracle
+ * is recalculated
* - To calculate the oracle, we need info for each cpu from
* compute_pending_for_cpu, which considers:
* - PPI: dist->irq_state & dist->irq_enable
@@ -988,6 +989,20 @@ out:
return ret;
}
+/**
+ * kvm_vgic_inject_irq - Inject an IRQ from a device to the vgic
+ * @kvm: The VM structure pointer
+ * @cpuid: The CPU for PPIs
+ * @irq_num: The IRQ number that is assigned to the device
+ * @level: Edge-triggered: true: to trigger the interrupt
+ * false: to ignore the call
+ * Level-sensitive true: activates an interrupt
+ * false: deactivates an interrupt
+ *
+ * The GIC is not concerned with devices being active-LOW or active-HIGH for
+ * level-sensitive interrupts. You can think of the level parameter as 1
+ * being HIGH and 0 bing LOW and all devices being active-HIGH.
+ */
int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
bool level)
{
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
