Re: [RFC][PATCH]: kdump on KVM

[Avi Kivity <avi@redhat.com>]

On 06/04/2010 11:11 PM, Chris Lalancette wrote:
> 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.c code when the hrtimer representing the PIT expires.  In
> this case, when we get the callback, we kick only the BSP.
>
> 2)  In the i8254.c code 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.c code when calling kvm_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 in DM_LOWEST mode.  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.
>
> 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.

>   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 the set_irq logic 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.

> Taking a step back, it seems to me that something along the lines of my
> previous patchset (where we do set_irq directly 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.

> 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.

> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 3c4f8f6..bc40143 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -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;
> +
> +	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.

> +
> +	/*
> +	 * 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

> +	}
> +
> +	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.

> +
> +	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.

> @@ -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&&
> +	    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.

>
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 9fd5b3e..c492bae 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -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.

-- 
error compiling committee.c: too many arguments to function