diff mbox series

[v2,3/3] KVM: X86: Let's harden the ipi fastpath condition edge-trigger mode

Message ID 1623050385-100988-3-git-send-email-wanpengli@tencent.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/3] KVM: LAPIC: Write 0 to TMICT should also cancel vmx-preemption timer | expand

Commit Message

Wanpeng Li June 7, 2021, 7:19 a.m. UTC 
From: Wanpeng Li <wanpengli@tencent.com>

Let's harden the ipi fastpath condition edge-trigger mode.

Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/x86.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Paolo Bonzini June 8, 2021, 4:27 p.m. UTC | #1 
On 07/06/21 09:19, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Let's harden the ipi fastpath condition edge-trigger mode.

This is not a good commit message...  And if it's a bug, it needs a 
kvm-unit-tests testcase.

Paolo

> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>   arch/x86/kvm/x86.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b594275..dbd3e9d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1922,6 +1922,7 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
>   		return 1;
>   
>   	if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
> +		((data & APIC_INT_LEVELTRIG) == 0) &&
>   		((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
>   		((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
>   		((u32)(data >> 32) != X2APIC_BROADCAST)) {
>
Sean Christopherson June 8, 2021, 4:35 p.m. UTC | #2 
On Mon, Jun 07, 2021, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Let's harden the ipi fastpath condition edge-trigger mode.

Can you elaborate on the motivation for this patch?

Intel's SDM states that the trigger mode is ignored for all IPIs except INIT,
and even clarifies that the local xAPIC will override the bit and send the IPI
as edge-triggered.

AMD's APM on the other hand explicitly lists level-triggered Fixed IPIs as a
valid ICR combination.

Regardless of which of the two conflicting specs we want KVM to emulate (which
is currently AMD), I don't see why the fastpath code should care, as I can't
find anything in the kvm_apic_send_ipi() path that would go awry if it's called
from the fastpath for a level-triggered IPI.

Related side topic, anyone happen to know if KVM (and Qemu's) emulation of IPIs
intentionally follows AMD instead of Intel?  I suspect it's unintentional,
especially since KVM's initial xAPIC emulation came from Intel.  Not that it's
likely to matter, but allowing level-triggered IPIs is bizarre, e.g. getting an
EOI sent to the right I/O APIC at the right time via a level-triggered IPI seems
extremely convoluted.

> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/x86.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b594275..dbd3e9d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1922,6 +1922,7 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
>  		return 1;
>  
>  	if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
> +		((data & APIC_INT_LEVELTRIG) == 0) &&
>  		((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
>  		((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
>  		((u32)(data >> 32) != X2APIC_BROADCAST)) {
> -- 
> 2.7.4
>
Paolo Bonzini June 8, 2021, 5:40 p.m. UTC | #3 
On 08/06/21 18:35, Sean Christopherson wrote:
> Related side topic, anyone happen to know if KVM (and Qemu's) emulation of IPIs
> intentionally follows AMD instead of Intel?  I suspect it's unintentional,
> especially since KVM's initial xAPIC emulation came from Intel.  Not that it's
> likely to matter, but allowing level-triggered IPIs is bizarre, e.g. getting an
> EOI sent to the right I/O APIC at the right time via a level-triggered IPI seems
> extremely convoluted.

QEMU traditionally followed AMD a bit more than Intel for historical 
reasons.  Probably the code went QEMU->Xen->KVM even though it was 
contributed by Intel.

Paolo
Wanpeng Li June 9, 2021, 2:08 a.m. UTC | #4 
On Wed, 9 Jun 2021 at 00:35, Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Jun 07, 2021, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > Let's harden the ipi fastpath condition edge-trigger mode.
>
> Can you elaborate on the motivation for this patch?
>
> Intel's SDM states that the trigger mode is ignored for all IPIs except INIT,
> and even clarifies that the local xAPIC will override the bit and send the IPI
> as edge-triggered.
>
> AMD's APM on the other hand explicitly lists level-triggered Fixed IPIs as a
> valid ICR combination.
>
> Regardless of which of the two conflicting specs we want KVM to emulate (which
> is currently AMD), I don't see why the fastpath code should care, as I can't
> find anything in the kvm_apic_send_ipi() path that would go awry if it's called
> from the fastpath for a level-triggered IPI.

Fair enough.

    Wanpeng
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b594275..dbd3e9d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1922,6 +1922,7 @@  static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
 		return 1;
 
 	if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
+		((data & APIC_INT_LEVELTRIG) == 0) &&
 		((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
 		((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
 		((u32)(data >> 32) != X2APIC_BROADCAST)) {