* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2023-09-30 13:58 [PATCH v3] KVM: x86: Use fast path for Xen timer delivery David Woodhouse
@ 2023-10-02 10:35 ` Paul Durrant
2023-10-02 17:00 ` Sean Christopherson
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Paul Durrant @ 2023-10-02 10:35 UTC (permalink / raw)
To: David Woodhouse, kvm
Cc: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
On 30/09/2023 14:58, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Most of the time there's no need to kick the vCPU and deliver the timer
> event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> directly from the timer callback, and only fall back to the slow path
> when it's necessary to do so.
>
> This gives a significant improvement in timer latency testing (using
> nanosleep() for various periods and then measuring the actual time
> elapsed).
>
> However, there was a reason¹ the fast path was dropped when this support
> was first added. The current code holds vcpu->mutex for all operations
> on the kvm->arch.timer_expires field, and the fast path introduces a
> potential race condition. Avoid that race by ensuring the hrtimer is
> (temporarily) cancelled before making changes in kvm_xen_start_timer(),
> and also when reading the values out for KVM_XEN_VCPU_ATTR_TYPE_TIMER.
>
> ¹ https://lore.kernel.org/kvm/846caa99-2e42-4443-1070-84e49d2f11d2@redhat.com/
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> • v2: Remember, and deal with, those races.
>
> • v3: Drop the assertions for vcpu being loaded; those can be done
> separately if at all.
>
> Reorder the code in xen_timer_callback() to make it clearer
> that kvm->arch.xen.timer_expires is being cleared in the case
> where the event channel delivery is *complete*, as opposed to
> the -EWOULDBLOCK deferred path.
>
> Drop the 'pending' variable in kvm_xen_vcpu_get_attr() and
> restart the hrtimer if (kvm->arch.xen.timer_expires), which
> ought to be exactly the same thing (that's the *point* in
> cancelling the timer, to make it truthful as we return its
> value to userspace).
>
> Improve comments.
>
> arch/x86/kvm/xen.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
Reviewed-by: Paul Durrant <paul@xen.org>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2023-09-30 13:58 [PATCH v3] KVM: x86: Use fast path for Xen timer delivery David Woodhouse
2023-10-02 10:35 ` Paul Durrant
@ 2023-10-02 17:00 ` Sean Christopherson
2023-10-02 17:05 ` David Woodhouse
2023-10-05 1:29 ` Sean Christopherson
2024-02-06 18:41 ` Sean Christopherson
3 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2023-10-02 17:00 UTC (permalink / raw)
To: David Woodhouse
Cc: kvm, Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
On Sat, Sep 30, 2023, David Woodhouse wrote:
> @@ -146,6 +160,14 @@ static enum hrtimer_restart xen_timer_callback(struct hrtimer *timer)
>
> static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, u64 guest_abs, s64 delta_ns)
> {
> + /*
> + * Avoid races with the old timer firing. Checking timer_expires
> + * to avoid calling hrtimer_cancel() will only have false positives
> + * so is fine.
> + */
> + if (vcpu->arch.xen.timer_expires)
> + hrtimer_cancel(&vcpu->arch.xen.timer);
> +
> atomic_set(&vcpu->arch.xen.timer_pending, 0);
> vcpu->arch.xen.timer_expires = guest_abs;
>
> @@ -1019,9 +1041,36 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> break;
>
> case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
> + /*
> + * Ensure a consistent snapshot of state is captured, with a
> + * timer either being pending, or the event channel delivered
> + * to the corresponding bit in the shared_info. Not still
> + * lurking in the timer_pending flag for deferred delivery.
> + * Purely as an optimisation, if the timer_expires field is
> + * zero, that means the timer isn't active (or even in the
> + * timer_pending flag) and there is no need to cancel it.
> + */
Ah, kvm_xen_start_timer() zeros timer_pending.
Given that, shouldn't it be impossible for xen_timer_callback() to observe a
non-zero timer_pending value? E.g. couldn't this code WARN?
if (atomic_read(&vcpu->arch.xen.timer_pending))
return HRTIMER_NORESTART;
Obviously not a blocker for this patch, I'm mostly just curious to know if I'm
missing something.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2023-10-02 17:00 ` Sean Christopherson
@ 2023-10-02 17:05 ` David Woodhouse
0 siblings, 0 replies; 11+ messages in thread
From: David Woodhouse @ 2023-10-02 17:05 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]
On Mon, 2023-10-02 at 10:00 -0700, Sean Christopherson wrote:
>
> > case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
> > + /*
> > + * Ensure a consistent snapshot of state is captured, with a
> > + * timer either being pending, or the event channel delivered
> > + * to the corresponding bit in the shared_info. Not still
> > + * lurking in the timer_pending flag for deferred delivery.
> > + * Purely as an optimisation, if the timer_expires field is
> > + * zero, that means the timer isn't active (or even in the
> > + * timer_pending flag) and there is no need to cancel it.
> > + */
>
> Ah, kvm_xen_start_timer() zeros timer_pending.
>
> Given that, shouldn't it be impossible for xen_timer_callback() to observe a
> non-zero timer_pending value? E.g. couldn't this code WARN?
>
> if (atomic_read(&vcpu->arch.xen.timer_pending))
> return HRTIMER_NORESTART;
>
> Obviously not a blocker for this patch, I'm mostly just curious to know if I'm
> missing something.
Yes, I believe that is true.
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2023-09-30 13:58 [PATCH v3] KVM: x86: Use fast path for Xen timer delivery David Woodhouse
2023-10-02 10:35 ` Paul Durrant
2023-10-02 17:00 ` Sean Christopherson
@ 2023-10-05 1:29 ` Sean Christopherson
2024-02-06 18:41 ` Sean Christopherson
3 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2023-10-05 1:29 UTC (permalink / raw)
To: Sean Christopherson, kvm, David Woodhouse
Cc: Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
On Sat, 30 Sep 2023 14:58:35 +0100, David Woodhouse wrote:
> Most of the time there's no need to kick the vCPU and deliver the timer
> event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> directly from the timer callback, and only fall back to the slow path
> when it's necessary to do so.
>
> This gives a significant improvement in timer latency testing (using
> nanosleep() for various periods and then measuring the actual time
> elapsed).
>
> [...]
Applied to kvm-x86 xen, thanks!
[1/1] KVM: x86: Use fast path for Xen timer delivery
https://github.com/kvm-x86/linux/commit/77c9b9dea4fb
--
https://github.com/kvm-x86/linux/tree/next
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2023-09-30 13:58 [PATCH v3] KVM: x86: Use fast path for Xen timer delivery David Woodhouse
` (2 preceding siblings ...)
2023-10-05 1:29 ` Sean Christopherson
@ 2024-02-06 18:41 ` Sean Christopherson
2024-02-06 18:51 ` David Woodhouse
3 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-02-06 18:41 UTC (permalink / raw)
To: David Woodhouse
Cc: kvm, Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
On Sat, Sep 30, 2023, David Woodhouse wrote:
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 40edf4d1974c..75586da134b3 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -134,9 +134,23 @@ static enum hrtimer_restart xen_timer_callback(struct hrtimer *timer)
> {
> struct kvm_vcpu *vcpu = container_of(timer, struct kvm_vcpu,
> arch.xen.timer);
> + struct kvm_xen_evtchn e;
> + int rc;
> +
> if (atomic_read(&vcpu->arch.xen.timer_pending))
> return HRTIMER_NORESTART;
>
> + e.vcpu_id = vcpu->vcpu_id;
> + e.vcpu_idx = vcpu->vcpu_idx;
> + e.port = vcpu->arch.xen.timer_virq;
> + e.priority = KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL;
> +
> + rc = kvm_xen_set_evtchn_fast(&e, vcpu->kvm);
> + if (rc != -EWOULDBLOCK) {
> + vcpu->arch.xen.timer_expires = 0;
> + return HRTIMER_NORESTART;
> + }
> +
> atomic_inc(&vcpu->arch.xen.timer_pending);
> kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
> kvm_vcpu_kick(vcpu);
> @@ -146,6 +160,14 @@ static enum hrtimer_restart xen_timer_callback(struct hrtimer *timer)
>
> static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, u64 guest_abs, s64 delta_ns)
> {
> + /*
> + * Avoid races with the old timer firing. Checking timer_expires
> + * to avoid calling hrtimer_cancel() will only have false positives
> + * so is fine.
> + */
> + if (vcpu->arch.xen.timer_expires)
> + hrtimer_cancel(&vcpu->arch.xen.timer);
> +
> atomic_set(&vcpu->arch.xen.timer_pending, 0);
> vcpu->arch.xen.timer_expires = guest_abs;
>
> @@ -1019,9 +1041,36 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> break;
>
> case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
> + /*
> + * Ensure a consistent snapshot of state is captured, with a
> + * timer either being pending, or the event channel delivered
> + * to the corresponding bit in the shared_info. Not still
> + * lurking in the timer_pending flag for deferred delivery.
> + * Purely as an optimisation, if the timer_expires field is
> + * zero, that means the timer isn't active (or even in the
> + * timer_pending flag) and there is no need to cancel it.
> + */
> + if (vcpu->arch.xen.timer_expires) {
> + hrtimer_cancel(&vcpu->arch.xen.timer);
> + kvm_xen_inject_timer_irqs(vcpu);
This has an obvious-in-hindsight recursive deadlock bug. If KVM actually needs
to inject a timer IRQ, and the fast path fails, i.e. the gpc is invalid,
kvm_xen_set_evtchn() will attempt to acquire xen.xen_lock, which is already held.
Not sure if I sucked at testing before, or if I just got "lucky" on a random run.
============================================
WARNING: possible recursive locking detected
6.8.0-smp--5e10b4d51d77-drs #232 Tainted: G O
--------------------------------------------
xen_shinfo_test/250013 is trying to acquire lock:
ffff938c9930cc30 (&kvm->arch.xen.xen_lock){+.+.}-{3:3}, at: kvm_xen_set_evtchn+0x74/0x170 [kvm]
but task is already holding lock:
ffff938c9930cc30 (&kvm->arch.xen.xen_lock){+.+.}-{3:3}, at: kvm_xen_vcpu_get_attr+0x38/0x250 [kvm]
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&kvm->arch.xen.xen_lock);
lock(&kvm->arch.xen.xen_lock);
*** DEADLOCK ***
May be due to missing lock nesting notation
2 locks held by xen_shinfo_test/250013:
#0: ffff9228863f21a8 (&vcpu->mutex){+.+.}-{3:3}, at: kvm_vcpu_ioctl+0x8f/0x5b0 [kvm]
#1: ffff938c9930cc30 (&kvm->arch.xen.xen_lock){+.+.}-{3:3}, at: kvm_xen_vcpu_get_attr+0x38/0x250 [kvm]
stack backtrace:
CPU: 128 PID: 250013 Comm: xen_shinfo_test Tainted: G O 6.8.0-smp--5e10b4d51d77-drs #232
Hardware name: Google, Inc. Arcadia_IT_80/Arcadia_IT_80, BIOS 34.30.0 07/27/2023
Call Trace:
<TASK>
dump_stack_lvl+0x69/0xa0
dump_stack+0x14/0x20
print_deadlock_bug+0x2af/0x2c0
__lock_acquire+0x13f7/0x2e30
lock_acquire+0xd4/0x220
__mutex_lock+0x6a/0xa60
mutex_lock_nested+0x1f/0x30
kvm_xen_set_evtchn+0x74/0x170 [kvm]
kvm_xen_vcpu_get_attr+0x136/0x250 [kvm]
kvm_arch_vcpu_ioctl+0x942/0x1130 [kvm]
kvm_vcpu_ioctl+0x484/0x5b0 [kvm]
__se_sys_ioctl+0x7a/0xc0
__x64_sys_ioctl+0x21/0x30
do_syscall_64+0x82/0x160
entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x460eab
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2024-02-06 18:41 ` Sean Christopherson
@ 2024-02-06 18:51 ` David Woodhouse
2024-02-07 2:58 ` Sean Christopherson
0 siblings, 1 reply; 11+ messages in thread
From: David Woodhouse @ 2024-02-06 18:51 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 910 bytes --]
On Tue, 2024-02-06 at 10:41 -0800, Sean Christopherson wrote:
>
> This has an obvious-in-hindsight recursive deadlock bug. If KVM actually needs
> to inject a timer IRQ, and the fast path fails, i.e. the gpc is invalid,
> kvm_xen_set_evtchn() will attempt to acquire xen.xen_lock, which is already held
Hm, right. In fact, kvm_xen_set_evtchn() shouldn't actually *need* the
xen_lock in an ideal world; it's only taking it in order to work around
the fact that the gfn_to_pfn_cache doesn't have its *own* self-
sufficient locking. I have patches for that...
I think the *simplest* of the "patches for that" approaches is just to
use the gpc->refresh_lock to cover all activate, refresh and deactivate
calls. I was waiting for Paul's series to land before sending that one,
but I'll work on it today, and double-check my belief that we can then
just drop xen_lock from kvm_xen_set_evtchn().
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2024-02-06 18:51 ` David Woodhouse
@ 2024-02-07 2:58 ` Sean Christopherson
2024-02-07 3:29 ` David Woodhouse
0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-02-07 2:58 UTC (permalink / raw)
To: David Woodhouse
Cc: kvm, Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
On Tue, Feb 06, 2024, David Woodhouse wrote:
> On Tue, 2024-02-06 at 10:41 -0800, Sean Christopherson wrote:
> >
> > This has an obvious-in-hindsight recursive deadlock bug. If KVM actually needs
> > to inject a timer IRQ, and the fast path fails, i.e. the gpc is invalid,
> > kvm_xen_set_evtchn() will attempt to acquire xen.xen_lock, which is already held
>
> Hm, right. In fact, kvm_xen_set_evtchn() shouldn't actually *need* the
> xen_lock in an ideal world; it's only taking it in order to work around
> the fact that the gfn_to_pfn_cache doesn't have its *own* self-
> sufficient locking. I have patches for that...
>
> I think the *simplest* of the "patches for that" approaches is just to
> use the gpc->refresh_lock to cover all activate, refresh and deactivate
> calls. I was waiting for Paul's series to land before sending that one,
> but I'll work on it today, and double-check my belief that we can then
> just drop xen_lock from kvm_xen_set_evtchn().
While I definitely want to get rid of arch.xen.xen_lock, I don't want to address
the deadlock by relying on adding more locking to the gpc code. I want a teeny
tiny patch that is easy to review and backport. Y'all are *proably* the only
folks that care about Xen emulation, but even so, that's not a valid reason for
taking a roundabout way to fixing a deadlock.
Can't we simply not take xen_lock in kvm_xen_vcpu_get_attr() It holds vcpu->mutex
so it's mutually exclusive with kvm_xen_vcpu_set_attr(), and I don't see any other
flows other than vCPU destruction that deactivate (or change) the gpc.
And the worst case scenario is that if _userspace_ is being stupid, userspace gets
a stale GPA.
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 4b4e738c6f1b..50aa28b9ffc4 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -973,8 +973,6 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
{
int r = -ENOENT;
- mutex_lock(&vcpu->kvm->arch.xen.xen_lock);
-
switch (data->type) {
case KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO:
if (vcpu->arch.xen.vcpu_info_cache.active)
@@ -1083,7 +1081,6 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
break;
}
- mutex_unlock(&vcpu->kvm->arch.xen.xen_lock);
return r;
}
If that seems to risky, we could go with an ugly and hacky, but conservative:
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 4b4e738c6f1b..456d05c5b18a 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1052,7 +1052,9 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
*/
if (vcpu->arch.xen.timer_expires) {
hrtimer_cancel(&vcpu->arch.xen.timer);
+ mutex_unlock(&vcpu->kvm->arch.xen.xen_lock);
kvm_xen_inject_timer_irqs(vcpu);
+ mutex_lock(&vcpu->kvm->arch.xen.xen_lock);
}
data->u.timer.port = vcpu->arch.xen.timer_virq;
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2024-02-07 2:58 ` Sean Christopherson
@ 2024-02-07 3:29 ` David Woodhouse
2024-02-07 4:28 ` Sean Christopherson
0 siblings, 1 reply; 11+ messages in thread
From: David Woodhouse @ 2024-02-07 3:29 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]
On Tue, 2024-02-06 at 18:58 -0800, Sean Christopherson wrote:
> On Tue, Feb 06, 2024, David Woodhouse wrote:
> > On Tue, 2024-02-06 at 10:41 -0800, Sean Christopherson wrote:
> > >
> > > This has an obvious-in-hindsight recursive deadlock bug. If KVM actually needs
> > > to inject a timer IRQ, and the fast path fails, i.e. the gpc is invalid,
> > > kvm_xen_set_evtchn() will attempt to acquire xen.xen_lock, which is already held
> >
> > Hm, right. In fact, kvm_xen_set_evtchn() shouldn't actually *need* the
> > xen_lock in an ideal world; it's only taking it in order to work around
> > the fact that the gfn_to_pfn_cache doesn't have its *own* self-
> > sufficient locking. I have patches for that...
> >
> > I think the *simplest* of the "patches for that" approaches is just to
> > use the gpc->refresh_lock to cover all activate, refresh and deactivate
> > calls. I was waiting for Paul's series to land before sending that one,
> > but I'll work on it today, and double-check my belief that we can then
> > just drop xen_lock from kvm_xen_set_evtchn().
>
> While I definitely want to get rid of arch.xen.xen_lock, I don't want to address
> the deadlock by relying on adding more locking to the gpc code. I want a teeny
> tiny patch that is easy to review and backport. Y'all are *proably* the only
> folks that care about Xen emulation, but even so, that's not a valid reason for
> taking a roundabout way to fixing a deadlock.
I strongly disagree. I get that you're reticent about fixing the gpc
locking, but what I'm proposing is absolutely *not* a 'roundabout way
to fixing a deadlock'. The kvm_xen_set_evtchn() function shouldn't
*need* that lock; it's only taking it because of the underlying problem
with the gpc itself, which needs its caller to do its locking for it.
The solution is not to do further gymnastics with the xen_lock.
> Can't we simply not take xen_lock in kvm_xen_vcpu_get_attr() It holds vcpu->mutex
> so it's mutually exclusive with kvm_xen_vcpu_set_attr(), and I don't see any other
> flows other than vCPU destruction that deactivate (or change) the gpc.
Maybe. Although with the gpc locking being incomplete, I'm extremely
concerned about something *implicitly* relying on the xen_lock. We
still need to fix the gpc to have self-contained locking.
I'll put something together and do some testing.
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2024-02-07 3:29 ` David Woodhouse
@ 2024-02-07 4:28 ` Sean Christopherson
2024-02-07 4:36 ` David Woodhouse
0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-02-07 4:28 UTC (permalink / raw)
To: David Woodhouse, g
Cc: kvm, Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
On Tue, Feb 06, 2024, David Woodhouse wrote:
> On Tue, 2024-02-06 at 18:58 -0800, Sean Christopherson wrote:
> > On Tue, Feb 06, 2024, David Woodhouse wrote:
> > > On Tue, 2024-02-06 at 10:41 -0800, Sean Christopherson wrote:
> > > >
> > > > This has an obvious-in-hindsight recursive deadlock bug. If KVM actually needs
> > > > to inject a timer IRQ, and the fast path fails, i.e. the gpc is invalid,
> > > > kvm_xen_set_evtchn() will attempt to acquire xen.xen_lock, which is already held
> > >
> > > Hm, right. In fact, kvm_xen_set_evtchn() shouldn't actually *need* the
> > > xen_lock in an ideal world; it's only taking it in order to work around
> > > the fact that the gfn_to_pfn_cache doesn't have its *own* self-
> > > sufficient locking. I have patches for that...
> > >
> > > I think the *simplest* of the "patches for that" approaches is just to
> > > use the gpc->refresh_lock to cover all activate, refresh and deactivate
> > > calls. I was waiting for Paul's series to land before sending that one,
> > > but I'll work on it today, and double-check my belief that we can then
> > > just drop xen_lock from kvm_xen_set_evtchn().
> >
> > While I definitely want to get rid of arch.xen.xen_lock, I don't want to address
> > the deadlock by relying on adding more locking to the gpc code. I want a teeny
> > tiny patch that is easy to review and backport. Y'all are *proably* the only
> > folks that care about Xen emulation, but even so, that's not a valid reason for
> > taking a roundabout way to fixing a deadlock.
>
> I strongly disagree. I get that you're reticent about fixing the gpc
> locking, but what I'm proposing is absolutely *not* a 'roundabout way
> to fixing a deadlock'. The kvm_xen_set_evtchn() function shouldn't
> *need* that lock; it's only taking it because of the underlying problem
> with the gpc itself, which needs its caller to do its locking for it.
>
> The solution is not to do further gymnastics with the xen_lock.
I agree that's the long term solution, but I am not entirely confident that a big
overhaul is 6.9 material at this point. Squeezing an overhaul into 6.8 (and if
we're being nitpicky, backporting to 6.7) is out of the question.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
2024-02-07 4:28 ` Sean Christopherson
@ 2024-02-07 4:36 ` David Woodhouse
0 siblings, 0 replies; 11+ messages in thread
From: David Woodhouse @ 2024-02-07 4:36 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, Paul Durrant, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 656 bytes --]
On Tue, 2024-02-06 at 20:28 -0800, Sean Christopherson wrote:
>
> I agree that's the long term solution, but I am not entirely confident that a big
> overhaul is 6.9 material at this point. Squeezing an overhaul into 6.8 (and if
> we're being nitpicky, backporting to 6.7) is out of the question.
It actually ends up being really simple. We just lift ->refresh_lock up
and use it to protect all the activate/deactivate/refresh paths.
I have a patch in my tree; just need to put it through some testing.
(Currently stuck in a hotel room with a bunch of positive COVID tests,
unstable wifi and no decent screen, getting a bit fractious :)
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread