Gleb Natapov wrote on 2013-03-21:
> On Thu, Mar 21, 2013 at 05:39:46AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-03-21:
>>> On Thu, Mar 21, 2013 at 05:30:32AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-03-21:
>>>>> On Thu, Mar 21, 2013 at 03:42:46AM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-03-20:
>>>>>>> On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
>>>>>>>> From: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>>>>>>>>
>>>>>>>> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
>>>>>>>> or apic register (id, ldr, dfr) is changed.
>>>>>>>>
>>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>>>>>>>> ---
>>>>>>>> virt/kvm/ioapic.c | 9 +++++++--
>>>>>>>> 1 files changed, 7 insertions(+), 2 deletions(-)
>>>>>>>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>>>>>>>> index ddf9414..91b4c08 100644
>>>>>>>> --- a/virt/kvm/ioapic.c
>>>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>>>> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu
>>> *vcpu,
>>>>>>>> { struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; union
>>>>>>>> kvm_ioapic_redirect_entry *e; + unsigned long *rtc_map =
>>>>>>>> ioapic->rtc_status.vcpu_map; struct kvm_lapic_irq irqe; int
> index;
>>>>>>>> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct
> kvm_vcpu
>>>>> *vcpu,
>>>>>>>> if (!e->fields.mask &&
>>>>>>>> (e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
>>>>>>>> kvm_irq_has_notifier(ioapic->kvm,
> KVM_IRQCHIP_IOAPIC,
>>>>>>>> - index))) {
>>>>>>>> + index) || index == 8)) {
>>>>>>>> irqe.dest_id = e->fields.dest_id;
>>>>>>>> irqe.vector = e->fields.vector;
>>>>>>>> irqe.dest_mode = e->fields.dest_mode;
>>>>>>>> irqe.shorthand = 0;
>>>>>>>>
>>>>>>>> if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
>>>>>>>> - irqe.dest_id, irqe.dest_mode))
>>>>>>>> + irqe.dest_id, irqe.dest_mode)) {
>>>>>>>> __set_bit(irqe.vector, eoi_exit_bitmap);
>>>>>>>> + if (index == 8)
>>>>>>>> + __set_bit(vcpu->vcpu_id, rtc_map);
>>>>>>>> + } else if (index == 8)
>>>>>>>> + __clear_bit(vcpu->vcpu_id, rtc_map);
>>>>>>> rtc_map bitmap is accessed from different vcpus simultaneously so
>>>>>>> access has to be atomic. We also have a race:
>>>>>>>
>>>>>>> vcpu0 iothread
>>>>>>> ioapic config changes
>>>>>>> request scan ioapic
>>>>>>> inject rtc interrupt
>>>>>>> use old vcpu mask
>>>>>>> scan_ioapic()
>>>>>>> recalculate vcpu mask
>>>>>>>
>>>>>>> So this approach (suggested by me :() will not work.
>>>>>>>
>>>>>>> Need to think about it some more. May be your idea of building a
>>>>>>> bitmap while injecting the interrupt is the way to go indeed: pass
>>>>>>> a pointer to a bitmap to kvm_irq_delivery_to_apic() and build it
>>>>>>> there. Pass NULL pointer if caller does not need to track vcpus.
>>>>>> Or, we can block inject rtc interrupt during recalculate vcpu map.
>>>>>>
>>>>>> if(need_eoi > 0 && in_recalculating)
>>>>>> return coalesced
>>>>>>
>>>>> This should be ||. Then you need to maintain in_recalculating and
>>>>> recalculations requests may overlap. Too complex and fragile.
>>>> It should not be too complex. How about the following logic?
>>>>
>>>> when make scan ioapic request:
>>>> kvm_vcpu_scan_ioapic()
>>>> {
>>>> kvm_for_each_vcpu()
>>>> in_recalculating++;
>>>> }
>>>>
>>>> Then on each vcpu's request handler:
>>>> vcpu_scan_ioapic()
>>>> {
>>>> in_recalculating--;
>>>> }
>>>>
>>> kvm_vcpu_scan_ioapic() can be called more often then vcpu_scan_ioapic()
>> Ok. I see your point. Maybe we need to rollback to old idea.
>>
>> Can you pick the first two patches? If rollback to old way, it will not
>> touch those code.
>>
> First patch is great, but drop no longer needed irqe there. I do not see
> the point of the second patch if the map will be built during injection.
Sure. I will resend the first patch.
And we need to rebuild TMR when ioapic entry changed. So the second patch will be used at that time. But it's ok to send it with APICv patch.
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
