On Wed, Dec 05, 2012 at 06:02:59AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2012-12-05:
> > On Wed, Dec 05, 2012 at 01:55:17AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2012-12-04:
> >>> On Tue, Dec 04, 2012 at 06:39:50AM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2012-12-03:
> >>>>> On Mon, Dec 03, 2012 at 03:01:03PM +0800, Yang Zhang wrote:
> >>>>>> Virtual interrupt delivery avoids KVM to inject vAPIC interrupts
> >>>>>> manually, which is fully taken care of by the hardware. This needs
> >>>>>> some special awareness into existing interrupr injection path:
> >>>>>>
> >>>>>> - for pending interrupt, instead of direct injection, we may need
> >>>>>> update architecture specific indicators before resuming to guest. -
> >>>>>> A pending interrupt, which is masked by ISR, should be also
> >>>>>> considered in above update action, since hardware will decide when
> >>>>>> to inject it at right time. Current has_interrupt and get_interrupt
> >>>>>> only returns a valid vector from injection p.o.v.
> >>>>> Most of my previous comments still apply.
> >>>>>
> >>>>>> +void kvm_set_eoi_exitmap(struct kvm_vcpu *vcpu, int vector,
> >>>>>> + int trig_mode, int always_set)
> >>>>>> +{
> >>>>>> + if (kvm_x86_ops->set_eoi_exitmap)
> >>>>>> + kvm_x86_ops->set_eoi_exitmap(vcpu, vector,
> >>>>>> + trig_mode, always_set);
> >>>>>> +}
> >>>>>> +
> >>>>>> /*
> >>>>>> * Add a pending IRQ into lapic.
> >>>>>> * Return 1 if successfully added and 0 if discarded.
> >>>>>> @@ -661,6 +669,7 @@ static int __apic_accept_irq(struct kvm_lapic
> > *apic,
> >>> int
> >>>>> delivery_mode,
> >>>>>> if (unlikely(!apic_enabled(apic)))
> >>>>>> break;
> >>>>>> + kvm_set_eoi_exitmap(vcpu, vector, trig_mode, 0);
> >>>>> As I said in the last review rebuild the bitmap when ioapic or irq
> >>>>> notifier configuration changes, user request bit to notify vcpus to
> >>>>> reload the bitmap.
> >>>> It is too complicated. When program ioapic entry, we cannot get the target
> > vcpu
> >>> easily. We need to read destination format register and logical destination
> >>> register to find out target vcpu if using logical mode. Also, we must trap every
> >>> modification to the two registers to update eoi bitmap.
> >>> No need to check target vcpu. Enable exit on all vcpus for the vector
> >> This is wrong. As we known, modern OS uses per VCPU vector. We cannot
> > ensure all vectors have same trigger mode. And what's worse, the vector in
> > another vcpu is used to handle high frequency interrupts(like 10G NIC), then it
> > will hurt performance.
> >>
> > I never saw OSes reuse vector used by ioapic, as far as I see this
> Could you point out which code does this check in Linux kernel? I don't see any special checks when Linux kernel allocates a vector.
>
arch/x86/kernel/apic/io_apic.c:create_irq_nr(). It uses
apic->target_cpus() to get cpu mask. target_cpus() return mask of all
online cpus. Actually you wrote arch_pi_create_irq() in PI patches to
workaround this behaviour and allocated vector per cpu, no?
Are you aware of any guest that I can run, examine ioapic/apic
configuration and see that the same vector is used on different vcpus
for different devices? Can you point me to it?
> > is not how Linux code works. Furthermore it will not work with KVM
> > currently since apic eoi redirected to ioapic based on vector alone,
> > not vector/vcpu pair and as far as I am aware this is how real HW works.
> yes, real HW works in this way. But why it is helpful in this case?
It makes it impossible to use the same vector for different devices on
different cpus if the vector is delivered to at least one cpu through ioapic.
It may cause spurious interrupts, it will bring havoc to our ack
notifiers (albeit this is KVM's implementation problem). Also look at
various comment in arch/x86/kernel/apic/io_apic.c, it looks like ioapics
tend to misbehave if you look at them funny. Who knows what troubles EOIing
the same vector twice on real HW may bring.
>
> >>> programmed into ioapic. Which two registers? All accesses to ioapic are
> >>> trapped and reconfiguration is rare.
> >> In logical mode, the destination VCPU is depend on each CPU's destination
> > format register and logical destination register. So we must also trap the two
> > registers.
> >> And if it uses lowest priority delivery mode, the PPR need to be trapped too.
> > Since PPR will change on each interrupt injection, the cost should be higher than
> > current approach.
> > No need for all of that if bitmask it global.
> No, the bitmask is per VCPU. Also, why it will work if bitmask is global?
Make in global. Why what will work?
And we need to trap format/logical destination/id registers anyway since
we need to build kvm->arch.apic_map table that is used to deliver
interrupts. BTW you can use this table to build per VCPU eoi bitmask
too, but I am not convinced it is needed in practice.
--
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
