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
