| Message ID | 20191106175602.4515-2-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Posted Interrupts fixes | expand |
On 11/6/2019 12:56 PM, Joao Martins wrote: > Commit 17e433b54393 ("KVM: Fix leak vCPU's VMCS value into other pCPU") > introduced vmx_dy_apicv_has_pending_interrupt() in order to determine > if a vCPU have a pending posted interrupt. This routine is used by > kvm_vcpu_on_spin() when searching for a a new runnable vCPU to schedule > on pCPU instead of a vCPU doing busy loop. > > vmx_dy_apicv_has_pending_interrupt() determines if a > vCPU has a pending posted interrupt solely based on PID.ON. However, > when a vCPU is preempted, vmx_vcpu_pi_put() sets PID.SN which cause > raised posted interrupts to only set bit in PID.PIR without setting > PID.ON (and without sending notification vector), as depicted in VT-d > manual section 5.2.3 "Interrupt-Posting Hardware Operation". > > Therefore, checking PID.ON is insufficient to determine if a vCPU has > pending posted interrupts and instead we should also check if there is > some bit set on PID.PIR. > > Fixes: 17e433b54393 ("KVM: Fix leak vCPU's VMCS value into other pCPU") > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> Reviewed-by: Jagannathan Raman <jag.raman@oracle.com> > --- > arch/x86/kvm/vmx/vmx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 31ce6bc2c371..18b0bee662a5 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6141,7 +6141,10 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) > > static bool vmx_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu) > { > - return pi_test_on(vcpu_to_pi_desc(vcpu)); > + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu); > + > + return pi_test_on(pi_desc) || > + !bitmap_empty((unsigned long *)pi_desc->pir, NR_VECTORS); > } > > static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap) >
On 06/11/19 18:56, Joao Martins wrote: > Commit 17e433b54393 ("KVM: Fix leak vCPU's VMCS value into other pCPU") > introduced vmx_dy_apicv_has_pending_interrupt() in order to determine > if a vCPU have a pending posted interrupt. This routine is used by > kvm_vcpu_on_spin() when searching for a a new runnable vCPU to schedule > on pCPU instead of a vCPU doing busy loop. > > vmx_dy_apicv_has_pending_interrupt() determines if a > vCPU has a pending posted interrupt solely based on PID.ON. However, > when a vCPU is preempted, vmx_vcpu_pi_put() sets PID.SN which cause > raised posted interrupts to only set bit in PID.PIR without setting > PID.ON (and without sending notification vector), as depicted in VT-d > manual section 5.2.3 "Interrupt-Posting Hardware Operation". > > Therefore, checking PID.ON is insufficient to determine if a vCPU has > pending posted interrupts and instead we should also check if there is > some bit set on PID.PIR. > > Fixes: 17e433b54393 ("KVM: Fix leak vCPU's VMCS value into other pCPU") > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/kvm/vmx/vmx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 31ce6bc2c371..18b0bee662a5 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6141,7 +6141,10 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) > > static bool vmx_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu) > { > - return pi_test_on(vcpu_to_pi_desc(vcpu)); > + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu); > + > + return pi_test_on(pi_desc) || > + !bitmap_empty((unsigned long *)pi_desc->pir, NR_VECTORS); > } > > static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap) Should we check the bitmap only if SN is false? We have a precondition that if SN is clear then non-empty PIR implies ON=1 (modulo the small window in vmx_vcpu_pi_load of course), so that'd be a bit faster. Paolo
On 11/11/19 2:46 PM, Paolo Bonzini wrote: > On 06/11/19 18:56, Joao Martins wrote: >> Commit 17e433b54393 ("KVM: Fix leak vCPU's VMCS value into other pCPU") >> introduced vmx_dy_apicv_has_pending_interrupt() in order to determine >> if a vCPU have a pending posted interrupt. This routine is used by >> kvm_vcpu_on_spin() when searching for a a new runnable vCPU to schedule >> on pCPU instead of a vCPU doing busy loop. >> >> vmx_dy_apicv_has_pending_interrupt() determines if a >> vCPU has a pending posted interrupt solely based on PID.ON. However, >> when a vCPU is preempted, vmx_vcpu_pi_put() sets PID.SN which cause >> raised posted interrupts to only set bit in PID.PIR without setting >> PID.ON (and without sending notification vector), as depicted in VT-d >> manual section 5.2.3 "Interrupt-Posting Hardware Operation". >> >> Therefore, checking PID.ON is insufficient to determine if a vCPU has >> pending posted interrupts and instead we should also check if there is >> some bit set on PID.PIR. >> >> Fixes: 17e433b54393 ("KVM: Fix leak vCPU's VMCS value into other pCPU") >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> >> --- >> arch/x86/kvm/vmx/vmx.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index 31ce6bc2c371..18b0bee662a5 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -6141,7 +6141,10 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) >> >> static bool vmx_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu) >> { >> - return pi_test_on(vcpu_to_pi_desc(vcpu)); >> + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu); >> + >> + return pi_test_on(pi_desc) || >> + !bitmap_empty((unsigned long *)pi_desc->pir, NR_VECTORS); >> } >> >> static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap) > > Should we check the bitmap only if SN is false? We have a precondition > that if SN is clear then non-empty PIR implies ON=1 (modulo the small > window in vmx_vcpu_pi_load of course), so that'd be a bit faster. Makes sense; The bitmap check was really meant for SN=1. Should SN=0 we would be saving ~22-27 cycles as far as I micro-benchmarked a few weeks ago. Now that you suggest it, it would be also good for older platforms too. Cheers, Joao
On 11/11/19 15:59, Joao Martins wrote: >> Should we check the bitmap only if SN is false? ^^^^^ Of course it should be skipped if SN is false, as you correctly say below. >> We have a precondition >> that if SN is clear then non-empty PIR implies ON=1 (modulo the small >> window in vmx_vcpu_pi_load of course), so that'd be a bit faster. > Makes sense; > > The bitmap check was really meant for SN=1. > > Should SN=0 we would be saving ~22-27 cycles as far as I micro-benchmarked a few > weeks ago. Now that you suggest it, it would be also good for older platforms too. Or even newer platforms if they don't use VT-d. Paolo
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 31ce6bc2c371..18b0bee662a5 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6141,7 +6141,10 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) static bool vmx_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu) { - return pi_test_on(vcpu_to_pi_desc(vcpu)); + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu); + + return pi_test_on(pi_desc) || + !bitmap_empty((unsigned long *)pi_desc->pir, NR_VECTORS); } static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
Commit 17e433b54393 ("KVM: Fix leak vCPU's VMCS value into other pCPU") introduced vmx_dy_apicv_has_pending_interrupt() in order to determine if a vCPU have a pending posted interrupt. This routine is used by kvm_vcpu_on_spin() when searching for a a new runnable vCPU to schedule on pCPU instead of a vCPU doing busy loop. vmx_dy_apicv_has_pending_interrupt() determines if a vCPU has a pending posted interrupt solely based on PID.ON. However, when a vCPU is preempted, vmx_vcpu_pi_put() sets PID.SN which cause raised posted interrupts to only set bit in PID.PIR without setting PID.ON (and without sending notification vector), as depicted in VT-d manual section 5.2.3 "Interrupt-Posting Hardware Operation". Therefore, checking PID.ON is insufficient to determine if a vCPU has pending posted interrupts and instead we should also check if there is some bit set on PID.PIR. Fixes: 17e433b54393 ("KVM: Fix leak vCPU's VMCS value into other pCPU") Signed-off-by: Joao Martins <joao.m.martins@oracle.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- arch/x86/kvm/vmx/vmx.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)