| Message ID | 1363779379-20212-6-git-send-email-yang.z.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- 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
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 Best regards, Yang -- 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
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. -- 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
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--; } 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 -- 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
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. -- 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
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@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() 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. > >> 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. Best regards, Yang -- 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
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@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() > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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() >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. How about build it in kvm_apic_set_irq()? It should be more straightforward. Best regards, Yang -- 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
On Thu, Mar 21, 2013 at 07:02:39AM +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. > How about build it in kvm_apic_set_irq()? It should be more straightforward. > Sure, pass a pointer there. Just do not access ioapic directly. -- 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
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); } } spin_unlock(&ioapic->lock);