Gleb Natapov wrote on 2013-04-07:
> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
>> 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.zh...@intel.com>
>>>>
>>>> Signed-off-by: Yang Zhang <yang.z.zh...@intel.com>
>>>> ---
>>>> 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?
>>
> First of all userspace should not care when it calls kvm_set_ioapic()
> the kernel need to do the right thing. Second, believe it or not,
> kvm_ioapic_reset() is not called during system reset. Instead userspace
> reset it by calling kvm_set_ioapic() with ioapic state after reset.
Ok. I see. As the logic you suggested, it will clear dest_map if no pending eoi
in vcpu, so we don't need to do it again.
>
>>>
>>>> + }
>>>> + }
>>>> + 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.
> Unit test is a guest code. This has nothing to do with a guest code.
> Unit test is not the one who creates lapic.
Ok. Then !ioapic is unnecessary.
>>
>>> 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();
> OK.
>
>>
>>>
>>>> + 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():
> __rtc_irq_eoi_tracking_restore_one();
> Since caller will have the lock already.
>
>> }
>>
>> 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();
>> }
>>
> Yes.
>
> --
> Gleb.
Best regards,
Yang
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
