Re: kvm-unit-tests: Question about the "no interrupt when timer is disabled" case

[Zenghui Yu <yuzenghui@huawei.com>]

Hi Alex,

[+cc Nianyao]

On 2020/7/24 19:08, Alexandru Elisei wrote:
> Hi Zenghui,
> 
> I don't believe this issue can be triggered by a Linux guest. Details below.
> 
> On 7/23/20 9:56 AM, Zenghui Yu wrote:
>> Hi Alexandru,
>>
>> I've noticed that the timer case will fail in the -stable 4.19 kernel.
>> The log is as follows:
>>
>> FAIL: vtimer-busy-loop: no interrupt when timer is disabled
>> FAIL: vtimer-busy-loop: interrupt signal no longer pending
>>
>> And it's because the related fix [16e604a437c8, "KVM: arm/arm64: vgic:
>> Reevaluate level sensitive interrupts on enable"] hasn't been backported
>> to the stable tree.
> 
> This is not an actual fix (hence no "Fixes" tag), this is more like an improvement
> of the behaviour of the GIC. Like the patch description says, this can happen even
> on hardware if the GIC hasn't sampled the device interrupt state (or the device
> itself hasn't updated it) before the CPU re-enables the interrupt.

Fair enough.

>>
>> Just out of curiosity, _without_ this fix, had you actually seen the
>> guest getting into trouble due to an un-retired level-sensitive
>> interrupt and your patch fixed it? Or this was found by code inspection?
> 
> This issue was found when running kvm-unit-tests on the model.
> 
>>
>> Take the exact vtimer case as an example, is it possible that the Linux
>> guest would disable the vtimer (the input interrupt line is driven to 0
>> but the old KVM doesn't take this into account) and potentially hit this
>> issue? I'm not familiar with it.
> 
> To trigger this, a guest has to do the following steps:
> 
> 1. Disable the timer interrupt at the Redistributor level.
> 2. Trigger the timer interrupt in the timer.
> 3. Disable the timer entirely (CNT{P,V}_CTL_EL0.ENABLE = 0), which also disables
> the timer interrupt.
> 4. Enable the timer interrupt at the Redistributor level.
> 
> I believe there are two reasons why this will never happen for a Linux guest:
> 
> - This isn't the way Linux handles interrupts. Furthermore, I don't believe Linux
> will ever disable a specific interrupt at the irqchip level.

This can at least happen in arch_timer_stop() [arm_arch_timer.c], where
the disable_percpu_irq() API will disable the interrupt (via irq_mask()
callback which will in turn disable the interrupt at GIC level by
programming the ICENABLER0).

What I'm worried is something like:

1. Disable the timer interrupt (at RDist level by poking the ICENABLER0,
    or at CPU level by masking PSTATE.I)

   [ timer interrupt is made pending, and remains pending in (v)GIC. ]

2. Disable the timer
3. Enable the timer interrupt (at RDist level by poking the ISENABLER0,
    or at CPU level by unmasking PSTATE.I)

   [ The interrupt is forwarded to (v)CPU, and we end-up re-enabling the
     timer inside the timer IRQ handler, which may not be as expected. ]

I'm just not sure if this will be a possible scenario in the Linux, and
is it harmful if this would happen.

> - The timer IRQ handler checks the ISTATUS flag in the timer control register
> before handling the interrupt. The flag is unset if the timer is disabled.

This actually doesn't match the spec. Arm ARM D13.8.25 has a description
about the ISTATUS field as below,

"When the value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN."

I guess what Nianyao had posted [*] may address the concern above...

[*] 
http://lore.kernel.org/r/1595584037-6877-1-git-send-email-zhangshaokun@hisilicon.com/

> 
> I hope my explanation made sense, please chime in if I missed something or you
> want more details.

Thanks Alex,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm