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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 28, 2019 at 05:08:53PM +0100, Marc Zyngier wrote:
> On 28/05/2019 14:40, Andrew Jones wrote:
> > On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
> >> 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@xxxxxxxxxx>
> >>>>> ---
> >>>>>  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.
> > 
> > I won't try to argue that this is the right fix, as I haven't fully
> > grasped how all this code works, but, afaict, this is how it worked
> > prior to bee038a6.
> > 
> >>>>
> >>>> 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();
> >>>
> > 
> > Marc, I've tested this and it resolves the issue for me. If/when you post
> > it you can add a t-b from me if you like.
> > 
> >>
> >> 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.
> >>
> > 
> > Isn't kvm_arm_timer_set_reg() only for userspace setting of the register?
> > In this test case I don't think userspace is involved at that point.
> 
> It still remains that userspace writing to any of the registers has an
> effect on the interrupt line. Or rather that it should.
> 
> And the more I look at this, the more I have the feeling this thing
> should happen on kvm_timer_vcpu_load(), wherever the writes comes from.
> It'd have slightly more overhead than doing it from every register
> access path, but at least it'd be clearer... Untested, again.
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 7fc272ecae16..8244e40af196 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -557,8 +557,12 @@ void kvm_timer_vcpu_load(struct kvm_vcpu *vcpu)
>  	if (map.direct_ptimer)
>  		timer_restore_state(map.direct_ptimer);
>  
> -	if (map.emul_ptimer)
> +	if (map.emul_ptimer) {
> +		kvm_timer_update_irq(vcpu,
> +				     kvm_timer_should_fire(map.emul_ptimer),
> +				     map.emul_ptimer);
>  		timer_emulate(map.emul_ptimer);
> +	}
>  }
>  
>  bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
> 

But do we do the put/load dance when we trap a write to a register from
the VM ?

Thanks,

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



[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux