From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [RFC][PATCH]: kdump on KVM Date: Mon, 07 Jun 2010 18:53:10 +0300 Message-ID: <4C0D15E6.8010104@redhat.com> References: <20100604201124.GD2831@localhost.localdomain> <4C0B55D3.2080100@redhat.com> <20100607153527.GD2760@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Marcelo Tosatti To: Chris Lalancette Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34085 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751966Ab0FGPxM (ORCPT ); Mon, 7 Jun 2010 11:53:12 -0400 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o57FrC0C024818 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 7 Jun 2010 11:53:12 -0400 Received: from cleopatra.tlv.redhat.com (cleopatra.tlv.redhat.com [10.35.255.11]) by int-mx04.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o57FrAo9028268 for ; Mon, 7 Jun 2010 11:53:11 -0400 In-Reply-To: <20100607153527.GD2760@localhost.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: 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