Re: [PATCH v3 09/10] KVM: arm/arm64: use vcpu requests for irq injection

Christoffer Dall
2017-05-08T08:56:13Z

On Mon, May 08, 2017 at 10:48:57AM +0200, Paolo Bonzini wrote:
>
>
> On 06/05/2017 20:49, Christoffer Dall wrote:
> > On Thu, May 04, 2017 at 01:47:41PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 03/05/2017 18:06, Andrew Jones wrote:
> >>> Don't use request-less VCPU kicks when injecting IRQs, as a VCPU
> >>> kick meant to trigger the interrupt injection could be sent while
> >>> the VCPU is outside guest mode, which means no IPI is sent, and
> >>> after it has called kvm_vgic_flush_hwstate(), meaning it won't see
> >>> the updated GIC state until its next exit some time later for some
> >>> other reason. The receiving VCPU only needs to check this request
> >>> in VCPU RUN to handle it. By checking it, if it's pending, a
> >>> memory barrier will be issued that ensures all state is visible.
> >>> We still create a vcpu_req_irq_pending() function (which is a nop),
> >>> though, in order to allow us to use the standard request checking
> >>> pattern.
> >>
> >> I wonder if you aren't just papering over this race:
> >>
> >> /*
> >> * If there are no virtual interrupts active or pending for this
> >> * VCPU, then there is no work to do and we can bail out without
> >> * taking any lock. There is a potential race with someone injecting
> >> * interrupts to the VCPU, but it is a benign race as the VCPU will
> >> * either observe the new interrupt before or after doing this check,
> >> * and introducing additional synchronization mechanism doesn't change
> >> * this.
> >> */
> >> if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
> >> return;
> >>
> >> spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
> >> vgic_flush_lr_state(vcpu);
> >> spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> >>
> >> not being so "benign" after all. :) Maybe you can remove the if (list_empty()),
> >> and have kvm_arch_vcpu_ioctl_run do this instead:
> >
> > I don't see how removing this shortcut improves anything. You'd still
> > have the same window where you could loose an interrupt right after the
> > spin_unlock.
>
> It's not removing it that matters; it's just unnecessary if you add
> KVM_REQ_IRQ_PENDING and you key the call to kvm_vgic_flush_hwstate on it.
>

That doesn't work, because you can have active interrupts in flight long
after someone sent you that request which means you'll have interrupts
on the ap_list that you need to flush.

Thanks,
-Christoffer