Re: [PATCH v3 05/39] ARM: timer: Handle level triggered IRQs correctly

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 26/03/18 21:28, Stefano Stabellini wrote:
> On Wed, 21 Mar 2018, Andre Przywara wrote:
>> The ARM Generic Timer uses a level-sensitive interrupt semantic. We
>> easily catch when the line goes high, as this triggers the hardware IRQ.
>> However we also have to keep track of when the line lowers, as the
>> emulation depends on it: Upon entering the guest, the new VGIC will
>> *clear* the virtual interrupt line, so it needs to re-sample the actual
>> state after returning from the guest.
>> So we have to sync the state of the interrupt condition at certain
>> points to catch when the line goes low and we can remove the vtimer vIRQ
>> from the vGIC (and the LR).
>> The VGIC in Xen so far only implemented edge triggered vIRQs, really, so
>> we need to add new functionality to re-sample the interrupt state.
>> Do this only when the new VGIC is in use.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>> Changelog v2 ... v3:
>> - move vtimer_sync() from time.c into vtimer.c
>> - rename function to vtimer_update_irqs()
>> - refactor functionality into new static function, to ...
>> - handle physical timer as well
>> - extending comments
>>
>> Changelog v1 ... v2:
>> - restrict to new VGIC
>> - add TODO: comment
>>
>>  xen/arch/arm/traps.c         | 11 ++++++++++
>>  xen/arch/arm/vtimer.c        | 49 ++++++++++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-arm/vtimer.h |  1 +
>>  3 files changed, 61 insertions(+)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 7411bff7a7..2638446693 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2024,6 +2024,17 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs)
>>          if ( current->arch.hcr_el2 & HCR_VA )
>>              current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>>  
>> +#ifdef CONFIG_NEW_VGIC
>> +        /*
>> +         * We need to update the state of our emulated devices using level
>> +         * triggered interrupts before syncing back the VGIC state.
>> +         *
>> +         * TODO: Investigate whether this is necessary to do on every
>> +         * trap and how it can be optimised.
>> +         */
>> +        vtimer_update_irqs(current);
>> +#endif
>> +
>>          vgic_sync_from_lrs(current);
>>      }
>>  }
>> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
>> index 8164f6c7f1..c99dd237d1 100644
>> --- a/xen/arch/arm/vtimer.c
>> +++ b/xen/arch/arm/vtimer.c
>> @@ -334,6 +334,55 @@ bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr)
>>      }
>>  }
>>  
>> +static void vtimer_update_irq(struct vcpu *v, struct vtimer *vtimer,
>> +                              uint32_t vtimer_ctl)
>> +{
>> +    bool level;
>> +
>> +    /* Filter for the three bits that determine the status of the timer */
>> +    vtimer_ctl &= (CNTx_CTL_ENABLE | CNTx_CTL_PENDING | CNTx_CTL_MASK);
>> +
>> +    /* The level is high if the timer is pending and enabled, but not masked. */
>> +    level = (vtimer_ctl == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING));
>> +
>> +    /*
>> +     * This is mostly here to *lower* the virtual interrupt line if the timer
>> +     * is no longer pending.
>> +     * We would have injected an IRQ already via SOFTIRQ when the timer expired.
>> +     * Doing it here again is basically a NOP if the line was already high.
>> +     */
>> +    vgic_inject_irq(v->domain, v, vtimer->irq, level);
>> +}
>> +
>> +/**
>> + * vtimer_update_irqs() - update the virtual timers' IRQ lines after a guest run
>> + * @vcpu: The VCPU to sync the timer state
>> + *
>> + * After returning from a guest, update the state of the timers' virtual
>> + * interrupt lines, to model the level triggered interrupts correctly.
>> + * If the guest has handled a timer interrupt, the virtual interrupt line
>> + * needs to be lowered explicitly. vgic_inject_irq() takes care of that.
>> + */
>> +void vtimer_update_irqs(struct vcpu *v)
>> +{
>> +    /*
>> +     * For the virtual timer we read the current state from the hardware.
>> +     * Technically we should keep the CNTx_CTL_MASK bit here, to catch if
>> +     * the timer interrupt is masked. However Xen *always* masks the timer
>> +     * upon entering the hypervisor, leaving it up to the guest to un-mask it.
>> +     * So we would always read a "low" level, despite the condition being
>> +     * actually "high".  Ignoring the mask bit solves this (for now).
>> +     *
>> +     * TODO: The proper fix for this is to make vtimer vIRQ hardware mapped,
>> +     * but this requires reworking the arch timer to implement this.
>> +     */
>> +    vtimer_update_irq(v, &v->arch.virt_timer,
>> +                      READ_SYSREG32(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
> 
> Yes, but won't this have the opposite effect? Meaning that it will
> always read as "high" for the virtual timer (because we remove the MASK
> and that is the only thing that can cause a "low" read in
> vtimer_update_irq if it's enabled and pending)?

What we want to know here is the status of the interrupt line of the
virtual timer. We don't know if it's still pending or not. So we are
very much interested in the pending bit of CNTV_CTL_EL0.
We could read the distributor's ISPENDR register as well, but this is
more costly.

> It seems to me that it would be better to remove the update of the
> virtual timer -- this seems to have the potential of causing problems.

Removing this makes Dom0 hang very early. The reason is that in that
case we never clear the line_level in the vtimer's struct vgic_irq:
When the h/w IRQ fires, we set line_level by injecting the corresponding
virtual IRQ. But if the emulated line_level is still high,
vgic_inject_irq() will bail out early, as making an IRQ pending when it
is already pending is a NOP, so vgic_validate_injection() denies that case.

Properly emulating the actual state of a virtual level triggered
interrupt line is something we were totally ignoring so far, because we
only dealt with edge interrupts. In case of the timer and also the event
channel this is wrong, as both devices are actually using level
triggered interrupts semantics.

Cheers,
Andre.

>> +    /* For the physical timer we rely on our emulated state. */
>> +    vtimer_update_irq(v, &v->arch.phys_timer, v->arch.phys_timer.ctl);
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/include/asm-arm/vtimer.h b/xen/include/asm-arm/vtimer.h
>> index 5aaddc6f63..91d88b377f 100644
>> --- a/xen/include/asm-arm/vtimer.h
>> +++ b/xen/include/asm-arm/vtimer.h
>> @@ -27,6 +27,7 @@ extern bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr);
>>  extern int virt_timer_save(struct vcpu *v);
>>  extern int virt_timer_restore(struct vcpu *v);
>>  extern void vcpu_timer_destroy(struct vcpu *v);
>> +void vtimer_update_irqs(struct vcpu *v);
>>  
>>  #endif
>>  
>> -- 
>> 2.14.1
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel