On 28/05/2019 12:01, Christoffer Dall wrote:
> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
>> The emulated ptimer needs to track the level changes, otherwise the
>> the interrupt will never get deasserted, resulting in the guest getting
>> stuck in an interrupt storm if it enables ptimer interrupts. This was
>> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
>> were enabled. Typical Linux guests don't have a problem as they prefer
>> using the virtual timer.
>>
>> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
>> timer_map")
>> Signed-off-by: Andrew Jones <drjo...@redhat.com>
>> ---
>> virt/kvm/arm/arch_timer.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 7fc272ecae16..9f5d8cc8b5e5 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu
>> *vcpu, bool new_level,
>> static void timer_emulate(struct arch_timer_context *ctx)
>> {
>> bool should_fire = kvm_timer_should_fire(ctx);
>> + struct timer_map map;
>> +
>> + get_timer_map(ctx->vcpu, &map);
>>
>> trace_kvm_timer_emulate(ctx, should_fire);
>>
>> - if (should_fire) {
>> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
>> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
>> + } else if (should_fire) {
>> kvm_timer_update_irq(ctx->vcpu, true, ctx);
>> return;
>> }
>
> Hmm, this doesn't feel completely right.
>
> Lowering the line of an emulated timer should only ever happen when the
> guest (or user space) writes to one of the system registers for that
> timer, which should be trapped and that should cause an update of the
> line.
>
> Are we missing a call to kvm_timer_update_irq() from
> kvm_arm_timer_set_reg() ?
Which is exactly what we removed in 6bc210003dff, for good reasons.
Looking at kvm_arm_timer_write_sysreg(), we end-up calling kvm_timer_vcpu_load,
but not updating the irq status.
How about something like this instead (untested):
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 7fc272ecae16..6a418dcc5433 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
enum kvm_arch_timer_regs treg,
u64 val)
{
+ struct arch_timer_context *timer;
+
preempt_disable();
kvm_timer_vcpu_put(vcpu);
- kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
+ timer = vcpu_get_timer(vcpu, tmr);
+ kvm_arm_timer_write(vcpu, timer, treg, val);
+ kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
kvm_timer_vcpu_load(vcpu);
preempt_enable();
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
