* [PATCH v2 1/2] KVM: X86: Filter out the broadcast dest for IPI fastpath
@ 2020-04-01 0:19 Wanpeng Li
2020-04-01 0:19 ` [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode Wanpeng Li
0 siblings, 1 reply; 8+ messages in thread
From: Wanpeng Li @ 2020-04-01 0:19 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel
From: Wanpeng Li <wanpengli@tencent.com>
Except destination shorthand, a destination value 0xffffffff is used to
broadcast interrupts, let's also filter out this for single target IPI
fastpath.
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
v1 -> v2:
* update subject and patch description
arch/x86/kvm/lapic.c | 3 ---
arch/x86/kvm/lapic.h | 3 +++
arch/x86/kvm/x86.c | 3 ++-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index e24d405..d528bed 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -59,9 +59,6 @@
#define MAX_APIC_VECTOR 256
#define APIC_VECTORS_PER_REG 32
-#define APIC_BROADCAST 0xFF
-#define X2APIC_BROADCAST 0xFFFFFFFFul
-
static bool lapic_timer_advance_dynamic __read_mostly;
#define LAPIC_TIMER_ADVANCE_ADJUST_MIN 100 /* clock cycles */
#define LAPIC_TIMER_ADVANCE_ADJUST_MAX 10000 /* clock cycles */
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index bc76860..25b77a6 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -17,6 +17,9 @@
#define APIC_BUS_CYCLE_NS 1
#define APIC_BUS_FREQUENCY (1000000000ULL / APIC_BUS_CYCLE_NS)
+#define APIC_BROADCAST 0xFF
+#define X2APIC_BROADCAST 0xFFFFFFFFul
+
enum lapic_mode {
LAPIC_MODE_DISABLED = 0,
LAPIC_MODE_INVALID = X2APIC_ENABLE,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5e95950..5a645df 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1559,7 +1559,8 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
- ((data & APIC_MODE_MASK) == APIC_DM_FIXED)) {
+ ((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
+ ((u32)(data >> 32) != X2APIC_BROADCAST)) {
data &= ~(1 << 12);
kvm_apic_send_ipi(vcpu->arch.apic, (u32)data, (u32)(data >> 32));
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode
2020-04-01 0:19 [PATCH v2 1/2] KVM: X86: Filter out the broadcast dest for IPI fastpath Wanpeng Li
@ 2020-04-01 0:19 ` Wanpeng Li
2020-04-01 0:35 ` Paolo Bonzini
0 siblings, 1 reply; 8+ messages in thread
From: Wanpeng Li @ 2020-04-01 0:19 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel
From: Wanpeng Li <wanpengli@tencent.com>
IPI delivery status field is not present in x2apic mode, don't need
to clear IPI delivery status in x2apic mode.
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
v1 -> v2:
* update code comments
* update subject and patch description
arch/x86/kvm/lapic.c | 5 +++--
arch/x86/kvm/x86.c | 1 -
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d528bed..5efca58 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1941,8 +1941,9 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
break;
}
case APIC_ICR:
- /* No delay here, so we always clear the pending bit */
- val &= ~(1 << 12);
+ /* Immediately clear Delivery Status in xAPIC mode */
+ if (!apic_x2apic_mode(apic))
+ val &= ~(1 << 12);
kvm_apic_send_ipi(apic, val, kvm_lapic_get_reg(apic, APIC_ICR2));
kvm_lapic_set_reg(apic, APIC_ICR, val);
break;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5a645df..ececc09 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1562,7 +1562,6 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
((u32)(data >> 32) != X2APIC_BROADCAST)) {
- data &= ~(1 << 12);
kvm_apic_send_ipi(vcpu->arch.apic, (u32)data, (u32)(data >> 32));
kvm_lapic_set_reg(vcpu->arch.apic, APIC_ICR2, (u32)(data >> 32));
kvm_lapic_set_reg(vcpu->arch.apic, APIC_ICR, (u32)data);
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode
2020-04-01 0:19 ` [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode Wanpeng Li
@ 2020-04-01 0:35 ` Paolo Bonzini
2020-04-01 6:46 ` Wanpeng Li
2020-04-01 10:17 ` Wanpeng Li
0 siblings, 2 replies; 8+ messages in thread
From: Paolo Bonzini @ 2020-04-01 0:35 UTC (permalink / raw)
To: Wanpeng Li, linux-kernel, kvm
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel
On 01/04/20 02:19, Wanpeng Li wrote:
> - /* No delay here, so we always clear the pending bit */
> - val &= ~(1 << 12);
> + /* Immediately clear Delivery Status in xAPIC mode */
> + if (!apic_x2apic_mode(apic))
> + val &= ~(1 << 12);
This adds a conditional, and the old behavior was valid according to the
SDM: "software should not assume the value returned by reading the ICR
is the last written value".
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode
2020-04-01 0:35 ` Paolo Bonzini
@ 2020-04-01 6:46 ` Wanpeng Li
2020-04-01 17:40 ` Nadav Amit
2020-04-01 10:17 ` Wanpeng Li
1 sibling, 1 reply; 8+ messages in thread
From: Wanpeng Li @ 2020-04-01 6:46 UTC (permalink / raw)
To: Paolo Bonzini
Cc: LKML, kvm, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Nadav Amit
Cc more people,
On Wed, 1 Apr 2020 at 08:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 01/04/20 02:19, Wanpeng Li wrote:
> > - /* No delay here, so we always clear the pending bit */
> > - val &= ~(1 << 12);
> > + /* Immediately clear Delivery Status in xAPIC mode */
> > + if (!apic_x2apic_mode(apic))
> > + val &= ~(1 << 12);
>
> This adds a conditional, and the old behavior was valid according to the
> SDM: "software should not assume the value returned by reading the ICR
> is the last written value".
Nadav, Sean, what do you think?
Wanpeng
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode
2020-04-01 0:35 ` Paolo Bonzini
2020-04-01 6:46 ` Wanpeng Li
@ 2020-04-01 10:17 ` Wanpeng Li
1 sibling, 0 replies; 8+ messages in thread
From: Wanpeng Li @ 2020-04-01 10:17 UTC (permalink / raw)
To: Paolo Bonzini
Cc: LKML, kvm, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel
On Wed, 1 Apr 2020 at 08:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 01/04/20 02:19, Wanpeng Li wrote:
> > - /* No delay here, so we always clear the pending bit */
> > - val &= ~(1 << 12);
> > + /* Immediately clear Delivery Status in xAPIC mode */
> > + if (!apic_x2apic_mode(apic))
> > + val &= ~(1 << 12);
>
> This adds a conditional, and the old behavior was valid according to the
> SDM: "software should not assume the value returned by reading the ICR
> is the last written value".
We can queue patch 1/2 separately to catch the merge window. :)
Wanpeng
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode
2020-04-01 6:46 ` Wanpeng Li
@ 2020-04-01 17:40 ` Nadav Amit
2020-04-01 23:01 ` Sean Christopherson
0 siblings, 1 reply; 8+ messages in thread
From: Nadav Amit @ 2020-04-01 17:40 UTC (permalink / raw)
To: Wanpeng Li
Cc: Paolo Bonzini, LKML, kvm, Sean Christopherson, Vitaly Kuznetsov,
Wanpeng Li, Jim Mattson, Joerg Roedel
> On Mar 31, 2020, at 11:46 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>
> Cc more people,
> On Wed, 1 Apr 2020 at 08:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 01/04/20 02:19, Wanpeng Li wrote:
>>> - /* No delay here, so we always clear the pending bit */
>>> - val &= ~(1 << 12);
>>> + /* Immediately clear Delivery Status in xAPIC mode */
>>> + if (!apic_x2apic_mode(apic))
>>> + val &= ~(1 << 12);
>>
>> This adds a conditional, and the old behavior was valid according to the
>> SDM: "software should not assume the value returned by reading the ICR
>> is the last written value".
>
> Nadav, Sean, what do you think?
I do not know. But if you write a KVM unit-test, I can run it on bare-metal
and give you feedback about how it behaves.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode
2020-04-01 17:40 ` Nadav Amit
@ 2020-04-01 23:01 ` Sean Christopherson
2020-04-02 0:14 ` Wanpeng Li
0 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2020-04-01 23:01 UTC (permalink / raw)
To: Nadav Amit
Cc: Wanpeng Li, Paolo Bonzini, LKML, kvm, Vitaly Kuznetsov,
Wanpeng Li, Jim Mattson, Joerg Roedel
On Wed, Apr 01, 2020 at 05:40:03PM +0000, Nadav Amit wrote:
> > On Mar 31, 2020, at 11:46 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> >
> > Cc more people,
> > On Wed, 1 Apr 2020 at 08:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> On 01/04/20 02:19, Wanpeng Li wrote:
> >>> - /* No delay here, so we always clear the pending bit */
> >>> - val &= ~(1 << 12);
> >>> + /* Immediately clear Delivery Status in xAPIC mode */
> >>> + if (!apic_x2apic_mode(apic))
> >>> + val &= ~(1 << 12);
> >>
> >> This adds a conditional, and the old behavior was valid according to the
> >> SDM: "software should not assume the value returned by reading the ICR
> >> is the last written value".
> >
> > Nadav, Sean, what do you think?
>
> I do not know. But if you write a KVM unit-test, I can run it on bare-metal
> and give you feedback about how it behaves.
I agree with Paolo, clearing the bit doesn't violate the SDM. The
conditional is just as costly as the AND, if not more so, even for x2APIC.
I would play it safe and clear the bit even in the x2APIC only path to
avoid tripping up guest kernels that loop on the delivery status even when
using x2APIC.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode
2020-04-01 23:01 ` Sean Christopherson
@ 2020-04-02 0:14 ` Wanpeng Li
0 siblings, 0 replies; 8+ messages in thread
From: Wanpeng Li @ 2020-04-02 0:14 UTC (permalink / raw)
To: Sean Christopherson
Cc: Nadav Amit, Paolo Bonzini, LKML, kvm, Vitaly Kuznetsov,
Wanpeng Li, Jim Mattson, Joerg Roedel
On Thu, 2 Apr 2020 at 07:01, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Apr 01, 2020 at 05:40:03PM +0000, Nadav Amit wrote:
> > > On Mar 31, 2020, at 11:46 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> > >
> > > Cc more people,
> > > On Wed, 1 Apr 2020 at 08:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >> On 01/04/20 02:19, Wanpeng Li wrote:
> > >>> - /* No delay here, so we always clear the pending bit */
> > >>> - val &= ~(1 << 12);
> > >>> + /* Immediately clear Delivery Status in xAPIC mode */
> > >>> + if (!apic_x2apic_mode(apic))
> > >>> + val &= ~(1 << 12);
> > >>
> > >> This adds a conditional, and the old behavior was valid according to the
> > >> SDM: "software should not assume the value returned by reading the ICR
> > >> is the last written value".
> > >
> > > Nadav, Sean, what do you think?
> >
> > I do not know. But if you write a KVM unit-test, I can run it on bare-metal
> > and give you feedback about how it behaves.
>
> I agree with Paolo, clearing the bit doesn't violate the SDM. The
> conditional is just as costly as the AND, if not more so, even for x2APIC.
>
> I would play it safe and clear the bit even in the x2APIC only path to
> avoid tripping up guest kernels that loop on the delivery status even when
> using x2APIC.
Fair enough.
Wanpeng
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-04-02 0:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 0:19 [PATCH v2 1/2] KVM: X86: Filter out the broadcast dest for IPI fastpath Wanpeng Li
2020-04-01 0:19 ` [PATCH v2 2/2] KVM: LAPIC: Don't need to clear IPI delivery status in x2apic mode Wanpeng Li
2020-04-01 0:35 ` Paolo Bonzini
2020-04-01 6:46 ` Wanpeng Li
2020-04-01 17:40 ` Nadav Amit
2020-04-01 23:01 ` Sean Christopherson
2020-04-02 0:14 ` Wanpeng Li
2020-04-01 10:17 ` Wanpeng Li
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).