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

Avi Kivity
2010-06-07T15:53:12Z

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