: kdump on KVM
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 31 lines · Chris Lalancette
Hello,
I've gone back to look at the problem with kdump while running as a KVM
guest. I'll preface this discussion by noting that all of the problems I describe below only happen when the HPET emulation is shutoff. However, since there still seem to be some lingering issues with the HPET emulation, running without the HPET is a reasonable mode of operation, and one in which it would be nice to have kexec/kdump working.
The basic problem with getting kdump working is that currently KVM
delivers the timer interrupts only to vcpu 0, so kexecing onto another processor will never see timer interrupts. In the KVM code, there are a number of places where we make incorrect assumptions regarding the timer and which vcpu to deliver it to:1) In the
i8254.ccode when the hrtimer representing the PIT expires. In this case, when we get the callback, we kick only the BSP.2) In the
i8254.ccode when a vcpu is migrated from one processor to another. In this case we only migrate the PIT timer if the vcpu to be migrated is the BSP.3) In the lapic code when deciding whether to accept a PIC interrupt, we only accept interrupts on the BSP.
4) In the
irq_comm.ccode when callingkvm_irq_delivery_to_apic(). The problem here is that we don't take into account the fact that an LAPIC might be disabled when trying to deliver an interrupt inDM_LOWESTmode. Further, on a kexec, the processor that we are kexec'ing *to* gets it's APIC ID re-written to the BSP APIC ID. What it means in the end is that we are currently still matching against the BSP even though vcpu 1 (where the kexec is happening) would match if we let it.
Quoted 10 lines
The attached patch takes care of 1), 3), and 4), and works in my testing (it needs to be cleaned up a bit to not be so inefficient, but it should work). However, problem 2) is pretty sticky. The reason we are currently migrating the PIT timer around with the BSP is pretty well explained in commit
2f5997140f22f68f6390c49941150d3fa8a95cb7. With my new patch, though, we are no longer guaranteeing that we are going to inject onto CPU 0. I think we can do something where when the hrtimer expires, we figure out which vcpu will get the timer interrupt and IPI to that physical processor to cause the VMEXIT. Unfortunately it's racy because the expiration of the hrtimer is de-coupled from the setting of the IRQ for the interrupt.
It's only racy because of (perhaps not immature) optimization. Logically, the flow of events is as follows:
1. the httimer fires
2. irq0 is toggled (perhaps via a workqueue since the hrtimer is in interrupt context)
3. the PIC and IOAPIC state machines run and decide which local APIC(s), if any, need to be notified
4. the local APICs are IPIed.
Perhaps we should start by deoptimizing the code to work like the above.
In light of the below, I'm not sure the race actually exists.
Quoted 6 lines
That means that
the hrtimer could expire, we could choose vcpu 2 (say), IPI to cause a VMEXIT, but by the time it goes to VMENTER the guest has changed something in the (IO)APIC(s) so that theset_irqlogic chooses vcpu 3 to do the injection. This would result in a delayed injection that 2f5997 is trying to avoid.
Not sure that's the race here. Between the vmexit and vmenter we have the following trace:
kvm_inject_pending_timer_irqs()
kvm_inject_pit_timer_irqs()
__inject_pit_timer_intr()
kvm_set_irq()
So, even if we end up on the wrong vcpu, we will IPI the right one later.
Quoted 5 lines
Taking a step back, it seems to me that something along the lines of my previous patchset (where we do
set_irqdirectly from the hrtimer callback) is the right way to go. We would still need to IPI to the appropriate physical cpu to cause a VMEXIT on the cpu we care about, but we would avoid the race I describe in the previous paragraph.
I agree. But we also want the vcpu that accepts the interrupt to pull the hrtimer after it (like it does now) to minimize cross-cpu talk.
Quoted 3 lines
Unfortunately that patchset is also much
more risky.
I'd like a minimal, backportable patchset first, then bigger changes later. We have at least two choices, irq-safe pit and ioapic, and using a workqueue for to match impedances. The latter is a lot safer since we already do it for assigned devices.
Quoted 33 lines
arch/x86/kvm/i8254.c +17
-1@@ -231,7 +231,13 @@ int pit_has_pending_timer(struct kvm_vcpu *vcpu) {struct kvm_pit *pit = vcpu->kvm->arch.vpit; -if (pit&&kvm_vcpu_is_bsp (vcpu)&& pit->pit_state.irq_ack) +/* We only want to inject a single IRQ for each PIT timer that goes +* off. If 2 vcpus happen to come in here at the same time, we may +* erroneously tell one of them that it has a timer pending. That's +* OK, though; in kvm_inject_pit_timer_irqs we will make sure to only +* let one of them inject. +*/ +if (pit&& pit->pit_state.irq_ack)return atomic_read (&pit->pit_state.pit_timer.pending);return 0 ; }@@ -277,6 +283,51 @@ static struct kvm_timer_ops kpit_ops = { .is_periodic = kpit_is_periodic, }; +static enum hrtimer_restart pit_timer_fn (struct hrtimer *data) +{ +int restart_timer =0 ; +struct kvm_vcpu *vcpu, *this; +int i; + struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer); + +if (!ktimer->vcpu) +return HRTIMER_NORESTART ; +
arch/x86/kvm/i8254.c static struct kvm_timer_ops kpit_ops = { · line 305
Quoted 10 lines
arch/x86/kvm/i8254.c +9
-0+ this =NULL ; +kvm_for_each_vcpu (i, vcpu, ktimer->kvm) { +if (kvm_lapic_enabled (vcpu)) { + this = vcpu; +break ; + } + } +if (this ==NULL ) + this = ktimer->kvm->bsp_vcpu;
If lapics are active, 'this' is chosen as the highest numbered lapic.
arch/x86/kvm/i8254.c static struct kvm_timer_ops kpit_ops = { · line 316
Quoted 11 lines
arch/x86/kvm/i8254.c +10
-0+ +/* +* There is a race window between reading and incrementing, but we do +* not care about potentially loosing timer events in the !reinject +* case anyway. +*/ +if (ktimer->reinject || !atomic_read (&ktimer->pending)) { +atomic_inc (&ktimer->pending); +/* FIXME: this code should not know anything about vcpus */ +set_bit (KVM_REQ_PENDING_TIMER ,&this->requests);
The comment is correct...
Possible fix:
- audit the code to make sure that it will work regardless of which vcpu it is tied to
- install an irq ack notifier for the pit interrupt
- pull the pit hrtimer towards the vcpu that last acked it
arch/x86/kvm/i8254.c static struct kvm_timer_ops kpit_ops = { · line 321
Quoted 5 lines
arch/x86/kvm/i8254.c +4
-0+ } + +if (waitqueue_active (&this->wq)) +wake_up_interruptible (&this->wq);
Need an IPI if the vcpu is in guest mode and we set KVM_REQ_PENDING_TIMER earlier.
arch/x86/kvm/i8254.c static struct kvm_timer_ops kpit_ops = { · line 331
Quoted 11 lines
arch/x86/kvm/i8254.c +10
-0+ +if (ktimer->t_ops->is_periodic (ktimer)) { +hrtimer_add_expires_ns (&ktimer->timer, ktimer->period); + restart_timer =1 ; + } + +if (restart_timer) +return HRTIMER_RESTART ; +else +return HRTIMER_NORESTART ;
Unrelated: as soon as ->pending starts to accumulate, we should return HRTIMER_NORESTART and calculate pending from the period and current time. This will reduce hrtimers if the system is overloaded, or if the guest masked the pit and uses some other time source.
Quoted 9 lines
arch/x86/kvm/i8254.c +0
-5@@ -1106,13 +1112,11 @@ int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)u32 lvt0 =apic_get_reg (vcpu->arch.apic,APIC_LVT0 );int r =0 ; -if (kvm_vcpu_is_bsp (vcpu)) { -if (!apic_hw_enabled (vcpu->arch.apic)) - r =1 ; -if ((lvt0&APIC_LVT_MASKED ) ==0 && -GET_APIC_DELIVERY_MODE (lvt0) ==APIC_MODE_EXTINT )
arch/x86/kvm/i8254.c int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu) · line 1122
Quoted 10 lines
arch/x86/kvm/i8254.c +5
-2- r =1 ; - } +if (!apic_hw_enabled (vcpu->arch.apic)) + r =1 ; +if ((lvt0&APIC_LVT_MASKED ) ==0 && +GET_APIC_DELIVERY_MODE (lvt0) ==APIC_MODE_EXTINT ) + r =1 ;return r; }
This is the crux, yes? I think that if we pull the vcpu calculation out of pit_timer_fn into an ack notifier, we can end up with the original kvm_timer_fn.
Quoted 7 lines
virt/kvm/irq_comm.c +0
-0
virt/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 104
Quoted 8 lines
virt/kvm/irq_comm.c +1
-1@@ -98,7 +98,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,if (r<0 ) r =0 ; r +=kvm_apic_set_irq (vcpu, irq); - }else { + }else if (kvm_lapic_enabled (vcpu)){if (!lowest) lowest = vcpu;else if (kvm_apic_compare_prio (vcpu, lowest)<0 )
Unrelated fix?
The patch is large for such a ticklish subject. I'd be more than happy to remove the vcpu affinity optimization, fix the code, and reinstate the optimization later, since it will make the patch(es) much easier to review.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 194 lines · Avi Kivity
Quoted 31 lines · Chris Lalancette
Hello,
I've gone back to look at the problem with kdump while running as a KVM
guest. I'll preface this discussion by noting that all of the problems I describe below only happen when the HPET emulation is shutoff. However, since there still seem to be some lingering issues with the HPET emulation, running without the HPET is a reasonable mode of operation, and one in which it would be nice to have kexec/kdump working.
The basic problem with getting kdump working is that currently KVM
delivers the timer interrupts only to vcpu 0, so kexecing onto another processor will never see timer interrupts. In the KVM code, there are a number of places where we make incorrect assumptions regarding the timer and which vcpu to deliver it to:1) In the
i8254.ccode when the hrtimer representing the PIT expires. In this case, when we get the callback, we kick only the BSP.2) In the
i8254.ccode when a vcpu is migrated from one processor to another. In this case we only migrate the PIT timer if the vcpu to be migrated is the BSP.3) In the lapic code when deciding whether to accept a PIC interrupt, we only accept interrupts on the BSP.
4) In the
irq_comm.ccode when callingkvm_irq_delivery_to_apic(). The problem here is that we don't take into account the fact that an LAPIC might be disabled when trying to deliver an interrupt inDM_LOWESTmode. Further, on a kexec, the processor that we are kexec'ing *to* gets it's APIC ID re-written to the BSP APIC ID. What it means in the end is that we are currently still matching against the BSP even though vcpu 1 (where the kexec is happening) would match if we let it.Quoted 10 lines
The attached patch takes care of 1), 3), and 4), and works in my testing (it needs to be cleaned up a bit to not be so inefficient, but it should work). However, problem 2) is pretty sticky. The reason we are currently migrating the PIT timer around with the BSP is pretty well explained in commit
2f5997140f22f68f6390c49941150d3fa8a95cb7. With my new patch, though, we are no longer guaranteeing that we are going to inject onto CPU 0. I think we can do something where when the hrtimer expires, we figure out which vcpu will get the timer interrupt and IPI to that physical processor to cause the VMEXIT. Unfortunately it's racy because the expiration of the hrtimer is de-coupled from the setting of the IRQ for the interrupt.It's only racy because of (perhaps not immature) optimization. Logically, the flow of events is as follows:
1. the httimer fires
2. irq0 is toggled (perhaps via a workqueue since the hrtimer is in interrupt context)
3. the PIC and IOAPIC state machines run and decide which local APIC(s), if any, need to be notified
4. the local APICs are IPIed.Perhaps we should start by deoptimizing the code to work like the above.
In light of the below, I'm not sure the race actually exists.
Quoted 5 lines
That means that
the hrtimer could expire, we could choose vcpu 2 (say), IPI to cause a VMEXIT, but by the time it goes to VMENTER the guest has changed something in the (IO)APIC(s) so that theset_irqlogic chooses vcpu 3 to do the injection. This would result in a delayed injection that 2f5997 is trying to avoid.Not sure that's the race here. Between the vmexit and vmenter we have the following trace:
kvm_inject_pending_timer_irqs()
kvm_inject_pit_timer_irqs()
__inject_pit_timer_intr()
kvm_set_irq()So, even if we end up on the wrong vcpu, we will IPI the right one later.
Quoted 5 lines
Taking a step back, it seems to me that something along the lines of my previous patchset (where we do
set_irqdirectly from the hrtimer callback) is the right way to go. We would still need to IPI to the appropriate physical cpu to cause a VMEXIT on the cpu we care about, but we would avoid the race I describe in the previous paragraph.I agree. But we also want the vcpu that accepts the interrupt to pull the hrtimer after it (like it does now) to minimize cross-cpu talk.
Quoted 2 lines
Unfortunately that patchset is also much
more risky.I'd like a minimal, backportable patchset first, then bigger changes later. We have at least two choices, irq-safe pit and ioapic, and using a workqueue for to match impedances. The latter is a lot safer since we already do it for assigned devices.
Quoted 32 lines
arch/x86/kvm/i8254.c +16
-1@@ -231,7 +231,13 @@ int pit_has_pending_timer(struct kvm_vcpu *vcpu) {struct kvm_pit *pit = vcpu->kvm->arch.vpit; -if (pit&&kvm_vcpu_is_bsp (vcpu)&& pit->pit_state.irq_ack) +/* We only want to inject a single IRQ for each PIT timer that goes +* off. If 2 vcpus happen to come in here at the same time, we may +* erroneously tell one of them that it has a timer pending. That's +* OK, though; in kvm_inject_pit_timer_irqs we will make sure to only +* let one of them inject. +*/ +if (pit&& pit->pit_state.irq_ack)return atomic_read (&pit->pit_state.pit_timer.pending);return 0 ; }@@ -277,6 +283,51 @@ static struct kvm_timer_ops kpit_ops = { .is_periodic = kpit_is_periodic, }; +static enum hrtimer_restart pit_timer_fn (struct hrtimer *data) +{ +int restart_timer =0 ; +struct kvm_vcpu *vcpu, *this; +int i; + struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer); + +if (!ktimer->vcpu) +return HRTIMER_NORESTART ;arch/x86/kvm/i8254.c static struct kvm_timer_ops kpit_ops = { · line 304
Quoted 10 lines
arch/x86/kvm/i8254.c +10
-0+ + this =NULL ; +kvm_for_each_vcpu (i, vcpu, ktimer->kvm) { +if (kvm_lapic_enabled (vcpu)) { + this = vcpu; +break ; + } + } +if (this ==NULL ) + this = ktimer->kvm->bsp_vcpu;If lapics are active, 'this' is chosen as the highest numbered lapic.
arch/x86/kvm/i8254.c static struct kvm_timer_ops kpit_ops = { · line 314
Quoted 10 lines
arch/x86/kvm/i8254.c +10
-0+ +/* +* There is a race window between reading and incrementing, but we do +* not care about potentially loosing timer events in the !reinject +* case anyway. +*/ +if (ktimer->reinject || !atomic_read (&ktimer->pending)) { +atomic_inc (&ktimer->pending); +/* FIXME: this code should not know anything about vcpus */ +set_bit (KVM_REQ_PENDING_TIMER ,&this->requests);The comment is correct...
Possible fix:
- audit the code to make sure that it will work regardless of which vcpu it is tied to
- install an irq ack notifier for the pit interrupt
- pull the pit hrtimer towards the vcpu that last acked itarch/x86/kvm/i8254.c static struct kvm_timer_ops kpit_ops = { · line 318
Quoted 4 lines
arch/x86/kvm/i8254.c +4
-0+ } + +if (waitqueue_active (&this->wq)) +wake_up_interruptible (&this->wq);Need an IPI if the vcpu is in guest mode and we set
KVM_REQ_PENDING_TIMERearlier.arch/x86/kvm/i8254.c static struct kvm_timer_ops kpit_ops = { · line 328
Quoted 10 lines
arch/x86/kvm/i8254.c +10
-0+ +if (ktimer->t_ops->is_periodic (ktimer)) { +hrtimer_add_expires_ns (&ktimer->timer, ktimer->period); + restart_timer =1 ; + } + +if (restart_timer) +return HRTIMER_RESTART ; +else +return HRTIMER_NORESTART ;Unrelated: as soon as ->pending starts to accumulate, we should return
HRTIMER_NORESTARTand calculate pending from the period and current time. This will reduce hrtimers if the system is overloaded, or if the guest masked the pit and uses some other time source.Quoted 14 lines
arch/x86/kvm/i8254.c +3
-7@@ -1106,13 +1112,11 @@ int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)u32 lvt0 =apic_get_reg (vcpu->arch.apic,APIC_LVT0 );int r =0 ; -if (kvm_vcpu_is_bsp (vcpu)) { -if (!apic_hw_enabled (vcpu->arch.apic)) - r =1 ; -if ((lvt0&APIC_LVT_MASKED ) ==0 && -GET_APIC_DELIVERY_MODE (lvt0) ==APIC_MODE_EXTINT ) - r =1 ; - } +if (!apic_hw_enabled (vcpu->arch.apic)) + r =1 ; + if ((lvt0& APIC_LVT_MASKED) == 0&&
Quoted 8 lines
arch/x86/kvm/i8254.c int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu) · line 1121
Quoted 4 lines
arch/x86/kvm/i8254.c +2
-0+ GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT) + r =1 ;return r; }This is the crux, yes? I think that if we pull the vcpu calculation out of
pit_timer_fninto an ack notifier, we can end up with the originalkvm_timer_fn.
I see disable_IO_APIC puts the IOAPIC in virtual wire mode with dest_id == bsp apic id, yes?
So the above should be enough?
As Avi mentioned having the hrtimer tick on vcpu0 should be fine since the entry path will inject the interrupt to the proper LAPIC.
Quoted 25 lines
Quoted 7 lines
virt/kvm/irq_comm.c +0
-0virt/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 104
Quoted 8 lines
virt/kvm/irq_comm.c +1
-1@@ -98,7 +98,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,if (r<0 ) r =0 ; r +=kvm_apic_set_irq (vcpu, irq); - }else { + }else if (kvm_lapic_enabled (vcpu)){if (!lowest) lowest = vcpu;else if (kvm_apic_compare_prio (vcpu, lowest)<0 )Unrelated fix?
The patch is large for such a ticklish subject. I'd be more than happy to remove the vcpu affinity optimization, fix the code, and reinstate the optimization later, since it will make the patch(es) much easier to review.
Signature · 1 line
error compiling committee.c: too many arguments to function
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 23 lines · Chris Lalancette
Quoted 14 lines
Quoted 14 lines
(quoted diff) +13
-0+static enum hrtimer_restart pit_timer_fn(struct hrtimer *data) +{ + int restart_timer = 0; + struct kvm_vcpu *vcpu, *this; + int i; + struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer); + + if (!ktimer->vcpu) + return HRTIMER_NORESTART; + + this = NULL; + kvm_for_each_vcpu(i, vcpu, ktimer->kvm) { + if (kvm_lapic_enabled(vcpu)) {Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ this = vcpu; + break; + } + } + if (this == NULL) + this = ktimer->kvm->bsp_vcpu;If lapics are active, 'this' is chosen as the highest numbered lapic.
Quoted 5 lines
Hm, I'm not sure I understand this comment. From my reading "
kvm_for_each_vcpu" starts at vcpu 0 and iterates up from there. So in the normal case with all LAPICs enabled, we choose vcpu 0 exclusively unless it's lapic is disabled. What am I missing?
Sorry, I misread the code. It will select vcpu 0.
Maybe we should check whether the bsp's lapic is enabled, and if not, choose the first enabled vcpu.
Dealing with a related problem, that code is still wrong. It should be possible to have multiple local APICs enabled for LINT0 delivery (say with DM_NMI). But one thing at a time, let's start with getting one thing fixed.
Quoted 17 lines
Quoted 17 lines
Quoted 7 lines
virt/kvm/irq_comm.c +0
-0virt/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 104
Quoted 8 lines
virt/kvm/irq_comm.c +1
-1@@ -98,7 +98,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,if (r<0 ) r =0 ; r +=kvm_apic_set_irq (vcpu, irq); - }else { + }else if (kvm_lapic_enabled (vcpu)){if (!lowest) lowest = vcpu;else if (kvm_apic_compare_prio (vcpu, lowest)<0 )Unrelated fix?
Quoted 2 lines
Actually, no, this is also required for kdump to work (and I should have put a comment here explaining).
I don't want a patch that fixes kdump. I want a series of patches that each fix a single problem with the emulated hardware. If the effect of the set is to fix kdump, that's a pleasant side effect.
So from my POV this hunk fixes a different problem from the timer-delivery-to-non-bsp problem.
Quoted 3 lines
What happens duing a kdump is that the apic of
the crashing cpu (say vcpu 1) is re-written to have the same APIC id as vcpu 0.
Wow, doesn't that confuse the APIC bus on real hardware?
Quoted 5 lines
So what happens during this loop is that we still always deliver the interrupt to vcpu 0 because we are
DM_LOWESTmode and it's the first one that matches. By gating it onkvm_lapic_enabled()we ensure that we skip vcpu 0 and choose vcpu 1 as the one to deliver to.
Perhaps having the local APIC disabled prevents it from talking on the APIC bus. Will need to rereread the docs.
Signature · 1 line
error compiling committee.c: too many arguments to function