If APICv is disabled for this vCPU, assigned devices may still attempt to post interrupts. In that case, we need to cancel the vmentry and deliver the interrupt with KVM_REQ_EVENT. Extend the existing code that handles injection of L1 interrupts into L2 to cover this case as well.
vmx_hwapic_irr_update is only called when APICv is active so it would be confusing to add a check for vcpu->arch.apicv_active in there. Instead, just use vmx_set_rvi directly in vmx_sync_pir_to_irr.
Cc: stable@vger.kernel.org Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index ba66c171d951..cccf1eab58ac 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6264,7 +6264,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) int max_irr; bool max_irr_updated;
- if (KVM_BUG_ON(!vcpu->arch.apicv_active, vcpu->kvm)) + if (KVM_BUG_ON(!enable_apicv, vcpu->kvm)) return -EIO;
if (pi_test_on(&vmx->pi_desc)) { @@ -6276,20 +6276,31 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) smp_mb__after_atomic(); max_irr_updated = kvm_apic_update_irr(vcpu, vmx->pi_desc.pir, &max_irr); - - /* - * If we are running L2 and L1 has a new pending interrupt - * which can be injected, this may cause a vmexit or it may - * be injected into L2. Either way, this interrupt will be - * processed via KVM_REQ_EVENT, not RVI, because we do not use - * virtual interrupt delivery to inject L1 interrupts into L2. - */ - if (is_guest_mode(vcpu) && max_irr_updated) - kvm_make_request(KVM_REQ_EVENT, vcpu); } else { max_irr = kvm_lapic_find_highest_irr(vcpu); + max_irr_updated = false; } - vmx_hwapic_irr_update(vcpu, max_irr); + + /* + * If virtual interrupt delivery is not in use, the interrupt + * will be processed via KVM_REQ_EVENT, not RVI. This can happen + * in two cases: + * + * 1) If we are running L2 and L1 has a new pending interrupt + * which can be injected, this may cause a vmexit or it may + * be injected into L2. We do not use virtual interrupt + * delivery to inject L1 interrupts into L2. + * + * 2) If APICv is disabled for this vCPU, assigned devices may + * still attempt to post interrupts. The posted interrupt + * vector will cause a vmexit and the subsequent entry will + * call sync_pir_to_irr. + */ + if (!is_guest_mode(vcpu) && vcpu->arch.apicv_active) + vmx_set_rvi(max_irr); + else if (max_irr_updated) + kvm_make_request(KVM_REQ_EVENT, vcpu); + return max_irr; }
On Mon, Nov 22, 2021, Paolo Bonzini wrote:
If APICv is disabled for this vCPU, assigned devices may still attempt to post interrupts. In that case, we need to cancel the vmentry and deliver the interrupt with KVM_REQ_EVENT. Extend the existing code that handles injection of L1 interrupts into L2 to cover this case as well.
vmx_hwapic_irr_update is only called when APICv is active so it would be confusing to add a check for vcpu->arch.apicv_active in there. Instead, just use vmx_set_rvi directly in vmx_sync_pir_to_irr.
Overzealous newlines.
If APICv is disabled for this vCPU, assigned devices may still attempt to post interrupts. In that case, we need to cancel the vmentry and deliver the interrupt with KVM_REQ_EVENT. Extend the existing code that handles injection of L1 interrupts into L2 to cover this case as well.
vmx_hwapic_irr_update is only called when APICv is active so it would be confusing to add a check for vcpu->arch.apicv_active in there. Instead, just use vmx_set_rvi directly in vmx_sync_pir_to_irr.
Cc: stable@vger.kernel.org Signed-off-by: Paolo Bonzini pbonzini@redhat.com
arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index ba66c171d951..cccf1eab58ac 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6264,7 +6264,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) int max_irr; bool max_irr_updated;
- if (KVM_BUG_ON(!vcpu->arch.apicv_active, vcpu->kvm))
- if (KVM_BUG_ON(!enable_apicv, vcpu->kvm)) return -EIO;
if (pi_test_on(&vmx->pi_desc)) { @@ -6276,20 +6276,31 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) smp_mb__after_atomic(); max_irr_updated = kvm_apic_update_irr(vcpu, vmx->pi_desc.pir, &max_irr);
/** If we are running L2 and L1 has a new pending interrupt* which can be injected, this may cause a vmexit or it may* be injected into L2. Either way, this interrupt will be* processed via KVM_REQ_EVENT, not RVI, because we do not use* virtual interrupt delivery to inject L1 interrupts into L2.*/if (is_guest_mode(vcpu) && max_irr_updated)kvm_make_request(KVM_REQ_EVENT, vcpu);
max_irr_updated = false;
Heh, maybe s/max_irr_updated/new_pir_found or so? This is a bit weird:
1. Update max_irr 2. max_irr_updated = false
}
- vmx_hwapic_irr_update(vcpu, max_irr);
- /*
* If virtual interrupt delivery is not in use, the interrupt* will be processed via KVM_REQ_EVENT, not RVI. This can happen
I'd strongly prefer to phrase this as a command, e.g. "process the interrupt via KVM_REQ_EVENT". "will be processed" makes it sound like some other flow is handling the event, which confused me.
* in two cases:** 1) If we are running L2 and L1 has a new pending interrupt
Hmm, calling it L1's interrupt isn't technically wrong, but it's a bit confusing because it's handled in L2. Maybe just say "vCPU"?
* which can be injected, this may cause a vmexit or it may
Overzealous newlines again.
* be injected into L2. We do not use virtual interrupt* delivery to inject L1 interrupts into L2.
I found the part of "may cause a vmexit or may be injected into L2" hard to follow, I think because this doesn't explicit state that the KVM_REQ_EVENT is needed to synthesize a VM-Exit.
How about this for the comment?
/* * If virtual interrupt delivery is not in use, process the interrupt * via KVM_REQ_EVENT, not RVI. This is necessary in two cases: * * 1) If L2 is running and the vCPU has a new pending interrupt. If L1 * wants to exit on interrupts, KVM_REQ_EVENT is needed to synthesize a * VM-Exit to L1. If L1 doesn't want to exit, the interrupt is injected * into L2, but KVM doesn't use virtual interrupt delivery to inject * interrupts into L2, and so KVM_REQ_EVENT is again needed. * * 2) If APICv is disabled for this vCPU, assigned devices may still * attempt to post interrupts. The posted interrupt vector will cause * a VM-Exit and the subsequent entry will call sync_pir_to_irr. */
** 2) If APICv is disabled for this vCPU, assigned devices may* still attempt to post interrupts. The posted interrupt* vector will cause a vmexit and the subsequent entry will* call sync_pir_to_irr.*/- if (!is_guest_mode(vcpu) && vcpu->arch.apicv_active)
If the second check uses kvm_vcpu_apicv_active(), this will avoid a silent conflict when kvm/master and kvm/queue are merged.
Nits aside, the logic is good:
Reviewed-by: Sean Christopherson seanjc@google.com
vmx_set_rvi(max_irr);- else if (max_irr_updated)
kvm_make_request(KVM_REQ_EVENT, vcpu);- return max_irr;
} -- 2.27.0
On 11/29/21 23:14, Sean Christopherson wrote:
Heh, maybe s/max_irr_updated/new_pir_found or so? This is a bit weird:
- Update max_irr
- max_irr_updated = false
Sounds good (I went for got_posted_interrupt).
}
- vmx_hwapic_irr_update(vcpu, max_irr);
- /*
* If virtual interrupt delivery is not in use, the interrupt* will be processed via KVM_REQ_EVENT, not RVI. This can happenI'd strongly prefer to phrase this as a command, e.g. "process the interrupt via KVM_REQ_EVENT". "will be processed" makes it sound like some other flow is handling the event, which confused me.
What I wanted to convey is that the interrupt is not processed yet, and the vmentry might have to be canceled. I changed it to
* Newly recognized interrupts are injected via either virtual interrupt * delivery (RVI) or KVM_REQ_EVENT. Virtual interrupt delivery is * disabled in two cases:
* 1) If L2 is running and the vCPU has a new pending interrupt. If L1 * wants to exit on interrupts, KVM_REQ_EVENT is needed to synthesize a * VM-Exit to L1. If L1 doesn't want to exit, the interrupt is injected * into L2, but KVM doesn't use virtual interrupt delivery to inject * interrupts into L2, and so KVM_REQ_EVENT is again needed. * * 2) If APICv is disabled for this vCPU, assigned devices may still * attempt to post interrupts. The posted interrupt vector will cause * a VM-Exit and the subsequent entry will call sync_pir_to_irr. */
Applied these.
Paolo
On Mon, Nov 22, 2021 at 07:43:09PM -0500, Paolo Bonzini wrote:
If APICv is disabled for this vCPU, assigned devices may still attempt to post interrupts. In that case, we need to cancel the vmentry and deliver the interrupt with KVM_REQ_EVENT. Extend the existing code that handles injection of L1 interrupts into L2 to cover this case as well.
vmx_hwapic_irr_update is only called when APICv is active so it would be confusing to add a check for vcpu->arch.apicv_active in there. Instead, just use vmx_set_rvi directly in vmx_sync_pir_to_irr.
Cc: stable@vger.kernel.org Signed-off-by: Paolo Bonzini pbonzini@redhat.com
Reviewed-by: David Matlack dmatlack@google.com
(Although I agree with Sean's suggestions and 1 nit below.)
arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index ba66c171d951..cccf1eab58ac 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6264,7 +6264,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) int max_irr; bool max_irr_updated;
- if (KVM_BUG_ON(!vcpu->arch.apicv_active, vcpu->kvm))
- if (KVM_BUG_ON(!enable_apicv, vcpu->kvm)) return -EIO;
if (pi_test_on(&vmx->pi_desc)) { @@ -6276,20 +6276,31 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) smp_mb__after_atomic(); max_irr_updated = kvm_apic_update_irr(vcpu, vmx->pi_desc.pir, &max_irr);
/** If we are running L2 and L1 has a new pending interrupt* which can be injected, this may cause a vmexit or it may* be injected into L2. Either way, this interrupt will be* processed via KVM_REQ_EVENT, not RVI, because we do not use* virtual interrupt delivery to inject L1 interrupts into L2.*/if (is_guest_mode(vcpu) && max_irr_updated)kvm_make_request(KVM_REQ_EVENT, vcpu);
max_irr_updated = false;
- vmx_hwapic_irr_update(vcpu, max_irr);
- /*
* If virtual interrupt delivery is not in use, the interrupt* will be processed via KVM_REQ_EVENT, not RVI. This can happen* in two cases:** 1) If we are running L2 and L1 has a new pending interrupt* which can be injected, this may cause a vmexit or it may* be injected into L2. We do not use virtual interrupt* delivery to inject L1 interrupts into L2.** 2) If APICv is disabled for this vCPU, assigned devices may* still attempt to post interrupts. The posted interrupt* vector will cause a vmexit and the subsequent entry will* call sync_pir_to_irr.*/- if (!is_guest_mode(vcpu) && vcpu->arch.apicv_active)
vmx_set_rvi(max_irr);- else if (max_irr_updated)
kvm_make_request(KVM_REQ_EVENT, vcpu);
nit: In the version of this code that is currently in kvm/queue the indentation of the previous 3 lines uses spaces instead of tabs. I see tabs in this mail though.
- return max_irr;
} -- 2.27.0
On Mon, 2021-11-22 at 19:43 -0500, Paolo Bonzini wrote:
If APICv is disabled for this vCPU, assigned devices may still attempt to post interrupts. In that case, we need to cancel the vmentry and deliver the interrupt with KVM_REQ_EVENT. Extend the existing code that handles injection of L1 interrupts into L2 to cover this case as well.
vmx_hwapic_irr_update is only called when APICv is active so it would be confusing to add a check for vcpu->arch.apicv_active in there. Instead, just use vmx_set_rvi directly in vmx_sync_pir_to_irr.
Cc: stable@vger.kernel.org Signed-off-by: Paolo Bonzini pbonzini@redhat.com
arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index ba66c171d951..cccf1eab58ac 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6264,7 +6264,7 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) int max_irr; bool max_irr_updated;
- if (KVM_BUG_ON(!vcpu->arch.apicv_active, vcpu->kvm))
- if (KVM_BUG_ON(!enable_apicv, vcpu->kvm)) return -EIO;
if (pi_test_on(&vmx->pi_desc)) { @@ -6276,20 +6276,31 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) smp_mb__after_atomic(); max_irr_updated = kvm_apic_update_irr(vcpu, vmx->pi_desc.pir, &max_irr);
/** If we are running L2 and L1 has a new pending interrupt* which can be injected, this may cause a vmexit or it may* be injected into L2. Either way, this interrupt will be* processed via KVM_REQ_EVENT, not RVI, because we do not use* virtual interrupt delivery to inject L1 interrupts into L2.*/if (is_guest_mode(vcpu) && max_irr_updated)kvm_make_request(KVM_REQ_EVENT, vcpu);
max_irr_updated = false;
- vmx_hwapic_irr_update(vcpu, max_irr);
- /*
* If virtual interrupt delivery is not in use, the interrupt* will be processed via KVM_REQ_EVENT, not RVI. This can happen* in two cases:** 1) If we are running L2 and L1 has a new pending interrupt* which can be injected, this may cause a vmexit or it may* be injected into L2. We do not use virtual interrupt* delivery to inject L1 interrupts into L2.** 2) If APICv is disabled for this vCPU, assigned devices may* still attempt to post interrupts. The posted interrupt* vector will cause a vmexit and the subsequent entry will* call sync_pir_to_irr.*/- if (!is_guest_mode(vcpu) && vcpu->arch.apicv_active)
vmx_set_rvi(max_irr);- else if (max_irr_updated)
kvm_make_request(KVM_REQ_EVENT, vcpu);- return max_irr;
}
Reviewed-by: Maxim Levitsky mlevitsk@redhat.com
Best regards, Maxim Levitsky
linux-stable-mirror@lists.linaro.org
-
David Matlack -
Maxim Levitsky -
Paolo Bonzini -
Sean Christopherson