> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Tuesday, November 17, 2015 5:41 PM
> To: Radim Krčmář <rkrc...@redhat.com>; Wu, Feng <feng...@intel.com>
> Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] KVM: x86: Add lowest-priority support for vt-d posted-
> interrupts
>
>
>
> On 16/11/2015 20:03, Radim Krčmář wrote:
> > 2015-11-09 10:46+0800, Feng Wu:
> >> Use vector-hashing to handle lowest-priority interrupts for
> >> posted-interrupts. As an example, modern Intel CPUs use this
> >> method to handle lowest-priority interrupts.
> >
> > (I don't think it's a good idea that the algorithm differs from non-PI
> > lowest priority delivery. I'd make them both vector-hashing, which
> > would be "fun" to explain to people expecting round robin ...)
>
> Yup, I would make it a module option. Thanks very much Radim for
> helping with the review.
Thanks for your guys' review. Yes, we can introduce a module option
for it. According to Radim's comments above, we need use the
same policy for PI and non-PI lowest-priority interrupts, so here is the
question: for vector hashing, it is easy to apply it for both non-PI and PI
case, however, for Round-Robin, in non-PI case, the round robin counter
is used and updated when the interrupt is injected to guest, but for
PI case, the interrupt is injected to guest totally by hardware, software
cannot control it while interrupt delivery, we can only decide the
destination vCPU for the PI interrupt in the initial configuration
time (guest update vMSI -> QEMU -> KVM). Do you guys have any good
suggestion to do round robin for PI lowest-priority? Seems Round robin
is not a good way for PI lowest-priority interrupts. Any comments
are appreciated!
Thanks,
Feng
>
> Paolo
>
> >> Signed-off-by: Feng Wu <feng...@intel.com>
> >> ---
> >> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> >> +/*
> >> + * This routine handles lowest-priority interrupts using vector-hashing
> >> + * mechanism. As an example, modern Intel CPUs use this method to
> handle
> >> + * lowest-priority interrupts.
> >> + *
> >> + * Here is the details about the vector-hashing mechanism:
> >> + * 1. For lowest-priority interrupts, store all the possible destination
> >> + * vCPUs in an array.
> >> + * 2. Use "guest vector % max number of destination vCPUs" to find the
> right
> >> + * destination vCPU in the array for the lowest-priority interrupt.
> >> + */
> >
> > (Is Skylake i7-6700 a modern Intel CPU?
> > I didn't manage to get hashing ... all interrupts always went to the
> > lowest APIC ID in the set :/
> > Is there a simple way to verify the algorithm?)
> >
> >> +struct kvm_vcpu *kvm_intr_vector_hashing_dest(struct kvm *kvm,
> >> + struct kvm_lapic_irq *irq)
> >> +
> >> +{
> >> + unsigned long
> dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
> >> + unsigned int dest_vcpus = 0;
> >> + struct kvm_vcpu *vcpu;
> >> + unsigned int i, mod, idx = 0;
> >> +
> >> + vcpu = kvm_intr_vector_hashing_dest_fast(kvm, irq);
> >> + if (vcpu)
> >> + return vcpu;
> >
> > I think the rest of this function shouldn't be implemented:
> > - Shorthands are only for IPIs and hence don't need to be handled,
> > - Lowest priority physical broadcast is not supported,
> > - Lowest priority cluster logical broadcast is not supported,
> > - No point in optimizing mixed xAPIC and x2APIC mode,
> > - The rest is handled by kvm_intr_vector_hashing_dest_fast().
> > (Even lowest priority flat logical "broadcast".)
> > - We do the work twice when vcpu == NULL means that there is no
> > matching destination.
> >
> > Is there a valid case that can be resolved by going through all vcpus?
> >
> >> +
> >> + memset(dest_vcpu_bitmap, 0, sizeof(dest_vcpu_bitmap));
> >> +
> >> + kvm_for_each_vcpu(i, vcpu, kvm) {
> >> + if (!kvm_apic_present(vcpu))
> >> + continue;
> >> +
> >> + if (!kvm_apic_match_dest(vcpu, NULL, irq->shorthand,
> >> + irq->dest_id, irq->dest_mode))
> >> + continue;
> >> +
> >> + __set_bit(vcpu->vcpu_id, dest_vcpu_bitmap);
> >> + dest_vcpus++;
> >> + }
> >> +
> >> + if (dest_vcpus == 0)
> >> + return NULL;
> >> +
> >> + mod = irq->vector % dest_vcpus;
> >> +
> >> + for (i = 0; i <= mod; i++) {
> >> + idx = find_next_bit(dest_vcpu_bitmap, KVM_MAX_VCPUS,
> idx) + 1;
> >> + BUG_ON(idx >= KVM_MAX_VCPUS);
> >> + }
> >> +
> >> + return kvm_get_vcpu(kvm, idx - 1);
> >> +}
> >> +EXPORT_SYMBOL_GPL(kvm_intr_vector_hashing_dest);
> >> +
> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >> @@ -816,6 +816,63 @@ out:
> >> +struct kvm_vcpu *kvm_intr_vector_hashing_dest_fast(struct kvm *kvm,
> >> + struct kvm_lapic_irq *irq)
> >
> > We now have three very similar functions :(
> >
> > kvm_irq_delivery_to_apic_fast
> > kvm_intr_is_single_vcpu_fast
> > kvm_intr_vector_hashing_dest_fast
> >
> > By utilizing the gcc optimizer, they can be merged without introducing
> > many instructions to the hot path, kvm_irq_delivery_to_apic_fast.
> > (I would eventually do it, so you can save time by ignoring this.)
> >
> > Thanks.
> >
