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.zh...@intel.com>
> >>>>>>
> >>>>>> 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.zh...@intel.com>
> >>>>>> ---
> >>>>>> 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.
--
Gleb.
--
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
