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--;
}
And when delivering RTC interrupt:
if(need_eoi > 0 || in_recalculating)
return coalesced
>
> --
> 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
