Gleb Natapov wrote on 2013-04-04: > On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote: >> From: Yang Zhang <yang.z.zhang@xxxxxxxxx> >> >> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx> >> --- >> arch/x86/kvm/lapic.c | 9 +++++++++ arch/x86/kvm/lapic.h | 2 ++ >> virt/kvm/ioapic.c | 43 >> +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h | >> 1 + 4 files changed, 55 insertions(+), 0 deletions(-) >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index 96ab160..9c041fa 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -94,6 +94,14 @@ static inline int apic_test_vector(int vec, void *bitmap) >> return test_bit(VEC_POS(vec), (bitmap) + REG_POS(vec)); >> } >> +bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector) >> +{ >> + struct kvm_lapic *apic = vcpu->arch.apic; >> + >> + return apic_test_vector(vector, apic->regs + APIC_ISR) || >> + apic_test_vector(vector, apic->regs + APIC_IRR); >> +} >> + >> static inline void apic_set_vector(int vec, void *bitmap) >> { >> set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec)); >> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu > *vcpu, >> apic->highest_isr_cache = -1; >> kvm_x86_ops->hwapic_isr_update(vcpu->kvm, >> apic_find_highest_isr(apic)); kvm_make_request(KVM_REQ_EVENT, vcpu); >> + kvm_rtc_irq_restore(vcpu); } >> >> void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu) >> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h >> index 967519c..004d2ad 100644 >> --- a/arch/x86/kvm/lapic.h >> +++ b/arch/x86/kvm/lapic.h >> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct > kvm_vcpu *vcpu) >> return vcpu->arch.apic->pending_events; >> } >> +bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector); >> + >> #endif >> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c >> index 8664812..0b12b17 100644 >> --- a/virt/kvm/ioapic.c >> +++ b/virt/kvm/ioapic.c >> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct > kvm_ioapic *ioapic, >> return result; >> } >> +static void rtc_irq_reset(struct kvm_ioapic *ioapic) >> +{ >> + ioapic->rtc_status.pending_eoi = 0; >> + bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS); >> +} >> + >> +static void rtc_irq_restore(struct kvm_ioapic *ioapic) >> +{ >> + struct kvm_vcpu *vcpu; >> + int vector, i, pending_eoi = 0; >> + >> + if (RTC_GSI >= IOAPIC_NUM_PINS) >> + return; >> + >> + vector = ioapic->redirtbl[RTC_GSI].fields.vector; >> + kvm_for_each_vcpu(i, vcpu, ioapic->kvm) { >> + if (kvm_apic_pending_eoi(vcpu, vector)) { >> + pending_eoi++; >> + __set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map); > You should cleat dest_map at the beginning to get rid of stale bits. I thought kvm_set_ioapic is called only after save/restore or migration. And the ioapic should be reset successfully before call it. So the dest_map is empty before call rtc_irq_restore(). But it is possible kvm_set_ioapic is called beside save/restore or migration. Right? > >> + } >> + } >> + ioapic->rtc_status.pending_eoi = pending_eoi; >> +} >> + >> +void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu) >> +{ >> + struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; >> + int vector; >> + >> + if (!ioapic) >> + return; >> + > Can this be called if ioapic == NULL? Yes. IIRC, unit test will test lapic function without ioapic. > Should check for if (RTC_GSI >= IOAPIC_NUM_PINS) here too. Not necessary. kvm_rtc_irq_restore is called from "arch/x86/" and we have the defination: #ifdef CONFIG_X86 #define RTC_GSI 8 The check will be false always. As the logic you suggested below, this check is necessary for _all() not _one(); > >> + spin_lock(&ioapic->lock); >> + vector = ioapic->redirtbl[RTC_GSI].fields.vector; >> + if (kvm_apic_pending_eoi(vcpu, vector)) { >> + __set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map); >> + ioapic->rtc_status.pending_eoi++; > The bit may have been set already. The logic should be: Right. > > > new_val = kvm_apic_pending_eoi(vcpu, vector) > old_val = set_bit(vcpu_id, dest_map) > > if (new_val == old_val) > return; > > if (new_val) { > __set_bit(vcpu_id, dest_map); > pending_eoi++; > } else { > __clear_bit(vcpu_id, dest_map); > pending_eoi--; > } > > The naming of above two functions are not good either. Call > them something like kvm_rtc_eoi_tracking_restore_all() and > kvm_rtc_eoi_tracking_restore_one(). And _all should call _one() for > each vcpu. Make __rtc_irq_eoi_tracking_restore_one() that does not > take ioapic lock and call it from kvm_rtc_eoi_tracking_restore_one() > surrounded by locks. Ok. Just confirm whether I am understanding correct: kvm_rtc_eoi_tracking_restore_all(): { for_each_vcpu: kvm_rtc_eoi_tracking_restore_one(): } kvm_rtc_eoi_tracking_restore_one(): { lock(); __rtc_irq_eoi_tracking_restore_one(): unlock(); } kvm_set_ioapic() { kvm_rtc_eoi_tracking_restore_all(); } kvm_apic_post_state_restore() { kvm_rtc_eoi_tracking_restore_one(); } > > -- > Gleb. > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html