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

[Avi Kivity <avi@redhat.com>]

On 06/07/2010 06:35 PM, Chris Lalancette wrote:
>
>>> +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.
>>      
> 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.

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

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

> So what happens during this loop is that we still always deliver the interrupt
> to vcpu 0 because we are DM_LOWEST mode and it's the first one that matches.
> By gating it on kvm_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.


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