* [PATCH] KVM: x86: Sync the pending Posted-Interrupts @ 2019-01-18 6:34 Luwei Kang 2019-01-25 16:33 ` Konrad Rzeszutek Wilk 2019-01-25 18:27 ` Paolo Bonzini 0 siblings, 2 replies; 9+ messages in thread From: Luwei Kang @ 2019-01-18 6:34 UTC (permalink / raw) To: kvm Cc: pbonzini, rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel, Luwei Kang Some Posted-Interrupts from passthrough devices may be lost or overwritten when the vCPU is in runnable state. The SN (Suppress Notification) of PID (Posted Interrupt Descriptor) will be set when the vCPU is preempted (vCPU in KVM_MP_STATE_RUNNABLE state but not running on physical CPU). If a posted interrupt coming at this time, the irq remmaping facility will set the bit of PIR (Posted Interrupt Requests) but ON (Outstanding Notification). So this interrupt can't be sync to APIC virtualization register and will not be handled by Guest because ON is zero. Signed-off-by: Luwei Kang <luwei.kang@intel.com> --- arch/x86/kvm/vmx/vmx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index f6915f1..820a03b 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6048,7 +6048,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) bool max_irr_updated; WARN_ON(!vcpu->arch.apicv_active); - if (pi_test_on(&vmx->pi_desc)) { + if (!bitmap_empty((unsigned long *)vmx->pi_desc.pir, NR_VECTORS)) { pi_clear_on(&vmx->pi_desc); /* * IOMMU can write to PIR.ON, so the barrier matters even on UP. -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: x86: Sync the pending Posted-Interrupts 2019-01-18 6:34 [PATCH] KVM: x86: Sync the pending Posted-Interrupts Luwei Kang @ 2019-01-25 16:33 ` Konrad Rzeszutek Wilk 2019-01-28 6:17 ` Kang, Luwei 2019-01-25 18:27 ` Paolo Bonzini 1 sibling, 1 reply; 9+ messages in thread From: Konrad Rzeszutek Wilk @ 2019-01-25 16:33 UTC (permalink / raw) To: Luwei Kang Cc: kvm, pbonzini, rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel On Fri, Jan 18, 2019 at 02:34:00PM +0800, Luwei Kang wrote: > Some Posted-Interrupts from passthrough devices may be lost or > overwritten when the vCPU is in runnable state. > > The SN (Suppress Notification) of PID (Posted Interrupt Descriptor) will > be set when the vCPU is preempted (vCPU in KVM_MP_STATE_RUNNABLE state > but not running on physical CPU). If a posted interrupt coming at this > time, the irq remmaping facility will set the bit of PIR (Posted > Interrupt Requests) but ON (Outstanding Notification). s/but ON/and ON is set too/? > So this interrupt can't be sync to APIC virtualization register and > will not be handled by Guest because ON is zero. > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index f6915f1..820a03b 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6048,7 +6048,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) > bool max_irr_updated; > > WARN_ON(!vcpu->arch.apicv_active); > - if (pi_test_on(&vmx->pi_desc)) { > + if (!bitmap_empty((unsigned long *)vmx->pi_desc.pir, NR_VECTORS)) { > pi_clear_on(&vmx->pi_desc); > /* > * IOMMU can write to PIR.ON, so the barrier matters even on UP. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] KVM: x86: Sync the pending Posted-Interrupts 2019-01-25 16:33 ` Konrad Rzeszutek Wilk @ 2019-01-28 6:17 ` Kang, Luwei 0 siblings, 0 replies; 9+ messages in thread From: Kang, Luwei @ 2019-01-28 6:17 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: kvm, pbonzini, rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel > > Some Posted-Interrupts from passthrough devices may be lost or > > overwritten when the vCPU is in runnable state. > > > > The SN (Suppress Notification) of PID (Posted Interrupt Descriptor) > > will be set when the vCPU is preempted (vCPU in KVM_MP_STATE_RUNNABLE > > state but not running on physical CPU). If a posted interrupt coming > > at this time, the irq remmaping facility will set the bit of PIR > > (Posted Interrupt Requests) but ON (Outstanding Notification). > > s/but ON/and ON is set too/? Sorry. If the interrupt remapping facility received a interrupt from device but the current SN bit is 1, the remapping facility will not set ON and not send notification event to CPU, just set the corresponding bit of PIR. Thanks, Luwei Kang > > So this interrupt can't be sync to APIC virtualization register and > > will not be handled by Guest because ON is zero. > > > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index > > f6915f1..820a03b 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -6048,7 +6048,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) > > bool max_irr_updated; > > > > WARN_ON(!vcpu->arch.apicv_active); > > - if (pi_test_on(&vmx->pi_desc)) { > > + if (!bitmap_empty((unsigned long *)vmx->pi_desc.pir, NR_VECTORS)) { > > pi_clear_on(&vmx->pi_desc); > > /* > > * IOMMU can write to PIR.ON, so the barrier matters even on UP. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: x86: Sync the pending Posted-Interrupts 2019-01-18 6:34 [PATCH] KVM: x86: Sync the pending Posted-Interrupts Luwei Kang 2019-01-25 16:33 ` Konrad Rzeszutek Wilk @ 2019-01-25 18:27 ` Paolo Bonzini 2019-01-28 8:08 ` Kang, Luwei 1 sibling, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2019-01-25 18:27 UTC (permalink / raw) To: Luwei Kang, kvm; +Cc: rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel On 18/01/19 07:34, Luwei Kang wrote: > Some Posted-Interrupts from passthrough devices may be lost or > overwritten when the vCPU is in runnable state. > > The SN (Suppress Notification) of PID (Posted Interrupt Descriptor) will > be set when the vCPU is preempted (vCPU in KVM_MP_STATE_RUNNABLE state > but not running on physical CPU). If a posted interrupt coming at this > time, the irq remmaping facility will set the bit of PIR (Posted > Interrupt Requests) but ON (Outstanding Notification). > So this interrupt can't be sync to APIC virtualization register and > will not be handled by Guest because ON is zero. > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index f6915f1..820a03b 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6048,7 +6048,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) > bool max_irr_updated; > > WARN_ON(!vcpu->arch.apicv_active); > - if (pi_test_on(&vmx->pi_desc)) { > + if (!bitmap_empty((unsigned long *)vmx->pi_desc.pir, NR_VECTORS)) { > pi_clear_on(&vmx->pi_desc); > /* > * IOMMU can write to PIR.ON, so the barrier matters even on UP. > This is a very delicate path. The bitmap check here is ordered after the vcpu->mode write in vcpu_enter_guest, matching the check of vcpu->mode in vmx_deliver_posted_interrupt (which comes after a write of PIR.ON): sender receiver write PIR write PIR.ON vcpu->mode = IN_GUEST_MODE smp_mb() smp_mb() read vcpu->mode sync_pir_to_irr read PIR.ON What you did should work, since PIR is written after PIR.ON anyway. However, you should at least change the comment in vcpu_enter_guest to mention "before reading PIR" instead of "before reading PIR.ON". Alternatively, would it be possible to instead set ON when SN is cleared? The clearing of SN is in pi_clear_sn, and you would have instead something like WRITE_ONCE(u16 *)&pi_desc->on_sn, POSTED_INTR_ON); where on_sn is added to struct pi_desc like this: @@ -61,4 +60,5 @@ struct pi_desc { u32 ndst; }; + u16 on_sn; u64 control; }; Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] KVM: x86: Sync the pending Posted-Interrupts 2019-01-25 18:27 ` Paolo Bonzini @ 2019-01-28 8:08 ` Kang, Luwei 2019-01-28 9:18 ` Paolo Bonzini 2019-01-29 9:32 ` Kang, Luwei 0 siblings, 2 replies; 9+ messages in thread From: Kang, Luwei @ 2019-01-28 8:08 UTC (permalink / raw) To: Paolo Bonzini, kvm; +Cc: rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel > > Some Posted-Interrupts from passthrough devices may be lost or > > overwritten when the vCPU is in runnable state. > > > > The SN (Suppress Notification) of PID (Posted Interrupt Descriptor) > > will be set when the vCPU is preempted (vCPU in KVM_MP_STATE_RUNNABLE > > state but not running on physical CPU). If a posted interrupt coming > > at this time, the irq remmaping facility will set the bit of PIR > > (Posted Interrupt Requests) but ON (Outstanding Notification). > > So this interrupt can't be sync to APIC virtualization register and > > will not be handled by Guest because ON is zero. > > > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index > > f6915f1..820a03b 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -6048,7 +6048,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) > > bool max_irr_updated; > > > > WARN_ON(!vcpu->arch.apicv_active); > > - if (pi_test_on(&vmx->pi_desc)) { > > + if (!bitmap_empty((unsigned long *)vmx->pi_desc.pir, NR_VECTORS)) { > > pi_clear_on(&vmx->pi_desc); > > /* > > * IOMMU can write to PIR.ON, so the barrier matters even on UP. > > > > This is a very delicate path. The bitmap check here is ordered after the vcpu->mode write in vcpu_enter_guest, matching the check of > vcpu->mode in vmx_deliver_posted_interrupt (which comes after a write of > PIR.ON): > > sender receiver > write PIR > write PIR.ON vcpu->mode = IN_GUEST_MODE > smp_mb() smp_mb() > read vcpu->mode sync_pir_to_irr > read PIR.ON > > What you did should work, since PIR is written after PIR.ON anyway. Hi Paolo: I think what you mentioned PIR.ON here is PID.ON; PID: Posted Interrupt Descriptor (a structure for PI which include 512 bits) ON: Outstanding Notification (one bit of PID) PIR: Posted Interrupt Requests (256 bits for each interrupt vector in PID) Before VT-d irq remapping, there just have PIR and ON in PID. Posted interrupt introduced by APICv can be used for send IPI. The working flow of sent a posted interrupt show in vmx_deliver_posted_interrupt(). 1. Set PIR of PID 2. Set ON of PID 3. Send a IPI to target vCPU with notification vector (POSTED_INTR_VECTOR) if target vCPU is Running on a pCPU; have no vm-exit and handed by guest interrupt handler directly. 4. if the target vCPU is not Running on pCPU, invoke kvm_vcpu_kick() to wake up this vCPU or send RESCHEDULE_VECTOR IPI to target pCPU to make the vCPU running as soon as possible. 5. follow 4. The vCPU prepare to run (vcpu_enter_guest) and sync the posted interrupt of ON is set. It looks like work well. VT-d irq remapping facility introduce SN, NV, NDST in PID. These are used by irq remapping facility and CPU don’t care these flags. 6. The bit of SN will be set when vCPU id preempted (runnable but not running). commit 28b835d60fcc2498e717cf5e6f0c3691c24546f7 KVM: Update Posted-Interrupts Descriptor when vCPU is preempted 7. if a interrupt coming at this moment, irq remapping facility just set PIR but *not* set ON (VT-d spec 9.12). So, here, the interrupt can't be sync to IRR because ON is 0. I add some log here and found some interrupt recorded in PIR but ON is zero. It will impact the performance of pass through device. > However, you should at least change the comment in vcpu_enter_guest to mention "before reading PIR" instead of "before reading > PIR.ON". Will do that. I think the "checking PIR.ON" should be PID.ON. I will fix it. > > Alternatively, would it be possible to instead set ON when SN is cleared? The clearing of SN is in pi_clear_sn, and you would have instead > something like SN is cleared when the corresponding vCPU is running on pCPU. I think we can't set ON when SN is cleared. Because there have some words in VT-d spec 9.12: If ON is set at the time of hardware posting an interrupt to PIR field, notification event is not generated. > > WRITE_ONCE(u16 *)&pi_desc->on_sn, POSTED_INTR_ON); We already have a function (pi_test_on) to check the bit of POSTED_INTR_ON. So I think it is unnecessary. Thanks, Luwei Kang > > where on_sn is added to struct pi_desc like this: > > @@ -61,4 +60,5 @@ struct pi_desc { > u32 ndst; > }; > + u16 on_sn; > u64 control; > }; > > Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: x86: Sync the pending Posted-Interrupts 2019-01-28 8:08 ` Kang, Luwei @ 2019-01-28 9:18 ` Paolo Bonzini 2019-01-29 9:32 ` Kang, Luwei 1 sibling, 0 replies; 9+ messages in thread From: Paolo Bonzini @ 2019-01-28 9:18 UTC (permalink / raw) To: Kang, Luwei, kvm; +Cc: rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel On 28/01/19 09:08, Kang, Luwei wrote: >> However, you should at least change the comment in vcpu_enter_guest to mention "before reading PIR" instead of "before reading >> PIR.ON". > > Will do that. I think the "checking PIR.ON" should be PID.ON. I will fix it. Yes. >> Alternatively, would it be possible to instead set ON when SN is >> cleared? The clearing of SN is in pi_clear_sn, and you would have instead >> something like > > SN is cleared when the corresponding vCPU is running on pCPU. I think we can't set ON when SN is cleared. Because there have some words in VT-d spec 9.12: > If ON is set at the time of hardware posting an interrupt to PIR field, notification event is not generated. This is okay, because you are setting ON and vmx_sync_pir_to_irr will read ON before the next vCPU entry and move the interrupts from PIR to IRR. Paolo >> >> WRITE_ONCE(u16 *)&pi_desc->on_sn, POSTED_INTR_ON); > > We already have a function (pi_test_on) to check the bit of POSTED_INTR_ON. So I think it is unnecessary. > > Thanks, > Luwei Kang > >> >> where on_sn is added to struct pi_desc like this: >> >> @@ -61,4 +60,5 @@ struct pi_desc { >> u32 ndst; >> }; >> + u16 on_sn; >> u64 control; >> }; >> >> Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] KVM: x86: Sync the pending Posted-Interrupts 2019-01-28 8:08 ` Kang, Luwei 2019-01-28 9:18 ` Paolo Bonzini @ 2019-01-29 9:32 ` Kang, Luwei 2019-01-29 13:30 ` Paolo Bonzini 1 sibling, 1 reply; 9+ messages in thread From: Kang, Luwei @ 2019-01-29 9:32 UTC (permalink / raw) To: Kang, Luwei, Paolo Bonzini, kvm Cc: rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel > > However, you should at least change the comment in vcpu_enter_guest to > > mention "before reading PIR" instead of "before reading PIR.ON". > > Will do that. I think the "checking PIR.ON" should be PID.ON. I will fix it. > Hi Paolo, I reconsidered the comment in vcpu_enter_guest() and think it may don't need to change. The original comment as below: * 2) For APICv, we should set ->mode before checking PIR.ON. This * pairs with the memory barrier implicit in pi_test_and_set_on * (see vmx_deliver_posted_interrupt). I think "before checking PIR.ON" is mean "before checking PID.PIR and PID.ON". Because there indeed have this two flag check in vmx_deliver_posted_interrupt() function. If PID.ON is already Set at the time of hardware posting an interrupt to PIR field, notification event is not generated (from VT-d spec 9.12). Thanks, Luwei Kang ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: x86: Sync the pending Posted-Interrupts 2019-01-29 9:32 ` Kang, Luwei @ 2019-01-29 13:30 ` Paolo Bonzini 2019-01-30 0:37 ` Kang, Luwei 0 siblings, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2019-01-29 13:30 UTC (permalink / raw) To: Kang, Luwei, kvm; +Cc: rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel On 29/01/19 10:32, Kang, Luwei wrote: >>> However, you should at least change the comment in vcpu_enter_guest to >>> mention "before reading PIR" instead of "before reading PIR.ON". >> >> Will do that. I think the "checking PIR.ON" should be PID.ON. I will fix it. >> > Hi Paolo, > I reconsidered the comment in vcpu_enter_guest() and think it may don't need to change. The original comment as below: > * 2) For APICv, we should set ->mode before checking PIR.ON. This > * pairs with the memory barrier implicit in pi_test_and_set_on > * (see vmx_deliver_posted_interrupt). > > I think "before checking PIR.ON" is mean "before checking PID.PIR and PID.ON". I can say it only means PID.ON because I wrote the comment. :) The idea is that checking ON is enough: KVM assumes that PID.PIR is only set if PID.ON is set, because it follows the definition of ON in table 29-1 of the SDM: "If this bit is set, there is a notification outstanding for one or more posted interrupts in bits 255:0". VT-D breaks this assumption whenever SN=1 ("hardware does not generate notification event nor modify the ON field"), resulting in nonzero PID.PIR but PID.ON=0. I'm sure there was a reason for that, but it does result in inconsistency between the PID definitions in the SDM and the VT-D specification. The right fix is definitely to reconcile this difference and test the bitmap after SN is cleared (with smp_mb__after_atomic after clearing SN), and set ON=1 if the bitmap is not clear. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] KVM: x86: Sync the pending Posted-Interrupts 2019-01-29 13:30 ` Paolo Bonzini @ 2019-01-30 0:37 ` Kang, Luwei 0 siblings, 0 replies; 9+ messages in thread From: Kang, Luwei @ 2019-01-30 0:37 UTC (permalink / raw) To: Paolo Bonzini, kvm; +Cc: rkrcmar, tglx, mingo, bp, hpa, x86, linux-kernel > >>> However, you should at least change the comment in vcpu_enter_guest > >>> to mention "before reading PIR" instead of "before reading PIR.ON". > >> > >> Will do that. I think the "checking PIR.ON" should be PID.ON. I will fix it. > >> > > Hi Paolo, > > I reconsidered the comment in vcpu_enter_guest() and think it may don't need to change. The original comment as below: > > * 2) For APICv, we should set ->mode before checking PIR.ON. This > > * pairs with the memory barrier implicit in pi_test_and_set_on > > * (see vmx_deliver_posted_interrupt). > > > > I think "before checking PIR.ON" is mean "before checking PID.PIR and PID.ON". > > I can say it only means PID.ON because I wrote the comment. :) > > The idea is that checking ON is enough: KVM assumes that PID.PIR is only set if PID.ON is set, because it follows the definition of ON in table > 29-1 of the SDM: "If this bit is set, there is a notification outstanding for one or more posted interrupts in bits 255:0". > > VT-D breaks this assumption whenever SN=1 ("hardware does not generate notification event nor modify the ON field"), resulting in > nonzero PID.PIR but PID.ON=0. I'm sure there was a reason for that, but it does result in inconsistency between the PID definitions in the > SDM and the VT-D specification. The right fix is definitely to reconcile this difference and test the bitmap after SN is cleared (with > smp_mb__after_atomic after clearing SN), and set ON=1 if the bitmap is not clear. > Hi Paolo Thanks for your elaboration, very clear to me. I will fix it in next version. Thanks, Luwei Kang ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-01-30 0:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-18 6:34 [PATCH] KVM: x86: Sync the pending Posted-Interrupts Luwei Kang 2019-01-25 16:33 ` Konrad Rzeszutek Wilk 2019-01-28 6:17 ` Kang, Luwei 2019-01-25 18:27 ` Paolo Bonzini 2019-01-28 8:08 ` Kang, Luwei 2019-01-28 9:18 ` Paolo Bonzini 2019-01-29 9:32 ` Kang, Luwei 2019-01-29 13:30 ` Paolo Bonzini 2019-01-30 0:37 ` Kang, Luwei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).