Re: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Gleb Natapov <gleb@redhat.com>]

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@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.zhang@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()

> 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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> Best regards,
> Yang
> 

--
			Gleb.