Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection

[Christoffer Dall <christoffer.dall@arm.com>]

On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> 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 <drjones@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.
> 

Ah well, I can be wrong twice.  Or even three times.

> 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();
> 

Yes, that looks reasonable.  Basically, in 6bc210003dff we should have
only removed the call to timer_emulate, and not messed around with
kvm_timer_update_irq()?

After this patch, we'll have moved the call to kvm_timer_update_irq()
from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg().  I can't
seem to decide if clearly belongs in one place or the other.

Thanks,

    Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm