RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Tuesday, January 26, 2016 12:15 AM
> To: Radim Krcmár 
> Cc: Wu, Feng ; linux-kernel@vger.kernel.org;
> k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> 
> 
> On 25/01/2016 16:20, Radim Krcmár wrote:
> > 2016-01-25 13:25+0100, Paolo Bonzini:
> >> On 22/01/2016 15:01, Radim Krcmár wrote:
> >>>> for (i = 0; i <= mod; i++) {
> >>>> idx = find_next_bit(bitmap, bitmap_size, idx + 1);
> >>>> BUG_ON(idx == bitmap_size);
> >>>> }
> >>
> >> WARN_ON, not BUG_ON.
> >
> > Callers don't check the return value for an error, because every error
> > is a BUG now.  I think that we should check if we return bitmap_size.
> > (Current paths could dereference NULL or throw unrelated warnings.)
> 
> You can probably just return a random number (e.g. zero or
> find_first_bit) if the bug is hit.  But really, the bug is easy enough
> to verify that BUG_ON might even be okay...

Thanks a lot for your comments, Radim and Paolo! Any other comments
to other patches in this series. If this is the only comments, do I need to
send v5?

Thanks,
Feng

> 
> Paolo

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Paolo Bonzini]

On 25/01/2016 16:20, Radim Krcmár wrote:
> 2016-01-25 13:25+0100, Paolo Bonzini:
>> On 22/01/2016 15:01, Radim Krcmár wrote:
 for (i = 0; i <= mod; i++) {
 idx = find_next_bit(bitmap, bitmap_size, idx + 1);
 BUG_ON(idx == bitmap_size);
 }
>>
>> WARN_ON, not BUG_ON.
> 
> Callers don't check the return value for an error, because every error
> is a BUG now.  I think that we should check if we return bitmap_size.
> (Current paths could dereference NULL or throw unrelated warnings.)

You can probably just return a random number (e.g. zero or
find_first_bit) if the bug is hit.  But really, the bug is easy enough
to verify that BUG_ON might even be okay...

Paolo

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krcmár]

2016-01-25 13:25+0100, Paolo Bonzini:
> On 22/01/2016 15:01, Radim Krcmár wrote:
>>> for (i = 0; i <= mod; i++) {
>>> idx = find_next_bit(bitmap, bitmap_size, idx + 1);
>>> BUG_ON(idx == bitmap_size);
>>> }
> 
> WARN_ON, not BUG_ON.

Callers don't check the return value for an error, because every error
is a BUG now.  I think that we should check if we return bitmap_size.
(Current paths could dereference NULL or throw unrelated warnings.)

Also, WARN_ON_ONCE?

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Paolo Bonzini]

On 22/01/2016 15:01, Radim Krcmár wrote:
>> for (i = 0; i <= mod; i++) {
>> idx = find_next_bit(bitmap, bitmap_size, idx + 1);
>> BUG_ON(idx == bitmap_size);
>> }

WARN_ON, not BUG_ON.

Paolo

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krcmár]

2016-01-25 13:25+0100, Paolo Bonzini:
> On 22/01/2016 15:01, Radim Krcmár wrote:
>>> for (i = 0; i <= mod; i++) {
>>> idx = find_next_bit(bitmap, bitmap_size, idx + 1);
>>> BUG_ON(idx == bitmap_size);
>>> }
> 
> WARN_ON, not BUG_ON.

Callers don't check the return value for an error, because every error
is a BUG now.  I think that we should check if we return bitmap_size.
(Current paths could dereference NULL or throw unrelated warnings.)

Also, WARN_ON_ONCE?

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Paolo Bonzini]

On 25/01/2016 16:20, Radim Krcmár wrote:
> 2016-01-25 13:25+0100, Paolo Bonzini:
>> On 22/01/2016 15:01, Radim Krcmár wrote:
 for (i = 0; i <= mod; i++) {
 idx = find_next_bit(bitmap, bitmap_size, idx + 1);
 BUG_ON(idx == bitmap_size);
 }
>>
>> WARN_ON, not BUG_ON.
> 
> Callers don't check the return value for an error, because every error
> is a BUG now.  I think that we should check if we return bitmap_size.
> (Current paths could dereference NULL or throw unrelated warnings.)

You can probably just return a random number (e.g. zero or
find_first_bit) if the bug is hit.  But really, the bug is easy enough
to verify that BUG_ON might even be okay...

Paolo

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Tuesday, January 26, 2016 12:15 AM
> To: Radim Krcmár <rkrc...@redhat.com>
> Cc: Wu, Feng <feng...@intel.com>; linux-kernel@vger.kernel.org;
> k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> 
> 
> On 25/01/2016 16:20, Radim Krcmár wrote:
> > 2016-01-25 13:25+0100, Paolo Bonzini:
> >> On 22/01/2016 15:01, Radim Krcmár wrote:
> >>>> for (i = 0; i <= mod; i++) {
> >>>> idx = find_next_bit(bitmap, bitmap_size, idx + 1);
> >>>> BUG_ON(idx == bitmap_size);
> >>>> }
> >>
> >> WARN_ON, not BUG_ON.
> >
> > Callers don't check the return value for an error, because every error
> > is a BUG now.  I think that we should check if we return bitmap_size.
> > (Current paths could dereference NULL or throw unrelated warnings.)
> 
> You can probably just return a random number (e.g. zero or
> find_first_bit) if the bug is hit.  But really, the bug is easy enough
> to verify that BUG_ON might even be okay...

Thanks a lot for your comments, Radim and Paolo! Any other comments
to other patches in this series. If this is the only comments, do I need to
send v5?

Thanks,
Feng

> 
> Paolo

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Paolo Bonzini]

On 22/01/2016 15:01, Radim Krcmár wrote:
>> for (i = 0; i <= mod; i++) {
>> idx = find_next_bit(bitmap, bitmap_size, idx + 1);
>> BUG_ON(idx == bitmap_size);
>> }

WARN_ON, not BUG_ON.

Paolo

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krcmár]

2016-01-22 05:12+, Wu, Feng:
>> From: Radim Krčmář [mailto:rkrc...@redhat.com]
>> 2016-01-20 09:42+0800, Feng Wu:
>> > +{
>> > +  u32 mod;
>> > +  int i, idx = 0;
>> > +
>> > +  mod = vector % dest_vcpus;
>> > +
>> > +  for (i = 0; i <= mod; i++) {
>> > +  idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
>> 
>> I'd remove this "+ 1".  Current users don't check for errors and always
>> do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
>> return type.
>> 
> 
> Does the following code look good to you:
> 
> u32 mod;
> int i, idx = -1;
> 
> mod = vector % dest_vcpus;
> 
> for (i = 0; i <= mod; i++) {
> idx = find_next_bit(bitmap, bitmap_size, idx + 1);
> BUG_ON(idx == bitmap_size);
> }
> 
> return idx;

It's ok, thanks.

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[rkrc...@redhat.com]

2016-01-22 12:00+0800, Yang Zhang:
> On 2016/1/22 1:21, rkrc...@redhat.com wrote:
>>(I think there isn't a practical difference between *r=-1 and *r=0.)
> 
> Currently, if *r == -1, the remote_irr may get set. But it seems wrong. I

Yeah ...

> need to have a double check to see whether it is a bug in current code.

Looking forward to the patch!

Thanks.

>>'ret = true' is the better one.  We know that the interrupt is not
>>deliverable [1], so there's no point in trying to deliver with the slow
>>path.  We behave similarly when the interrupt targets a single disabled
>>APIC.
>>
>>---
>>1: Well ... it's possible that slowpath would deliver it thanks to
>>different handling of disabled APICs, but it's undefined behavior,
> 
> why it is undefined behavior? Besides, why we will keep two different
> handling logic for the fast path and slow path? It looks weird.

It does look very weird ... the slow path would require refactoring,
though, so we save effort without a considerable drawback.
(I would love if it behaved identically, but I don't want to force it on
 someone and likely won't do it myself ...)

I consider it undefined because SMD says that an OS musn't configure
this behavior and doesn't say what should happen if the OS does => we
could do anything.  (Killing the guest would be great for debugging OS
issues, but ours behavior is fairly conservative.)

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krcmár]

2016-01-22 05:12+, Wu, Feng:
>> From: Radim Krčmář [mailto:rkrc...@redhat.com]
>> 2016-01-20 09:42+0800, Feng Wu:
>> > +{
>> > +  u32 mod;
>> > +  int i, idx = 0;
>> > +
>> > +  mod = vector % dest_vcpus;
>> > +
>> > +  for (i = 0; i <= mod; i++) {
>> > +  idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
>> 
>> I'd remove this "+ 1".  Current users don't check for errors and always
>> do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
>> return type.
>> 
> 
> Does the following code look good to you:
> 
> u32 mod;
> int i, idx = -1;
> 
> mod = vector % dest_vcpus;
> 
> for (i = 0; i <= mod; i++) {
> idx = find_next_bit(bitmap, bitmap_size, idx + 1);
> BUG_ON(idx == bitmap_size);
> }
> 
> return idx;

It's ok, thanks.

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[rkrc...@redhat.com]

2016-01-22 12:00+0800, Yang Zhang:
> On 2016/1/22 1:21, rkrc...@redhat.com wrote:
>>(I think there isn't a practical difference between *r=-1 and *r=0.)
> 
> Currently, if *r == -1, the remote_irr may get set. But it seems wrong. I

Yeah ...

> need to have a double check to see whether it is a bug in current code.

Looking forward to the patch!

Thanks.

>>'ret = true' is the better one.  We know that the interrupt is not
>>deliverable [1], so there's no point in trying to deliver with the slow
>>path.  We behave similarly when the interrupt targets a single disabled
>>APIC.
>>
>>---
>>1: Well ... it's possible that slowpath would deliver it thanks to
>>different handling of disabled APICs, but it's undefined behavior,
> 
> why it is undefined behavior? Besides, why we will keep two different
> handling logic for the fast path and slow path? It looks weird.

It does look very weird ... the slow path would require refactoring,
though, so we save effort without a considerable drawback.
(I would love if it behaved identically, but I don't want to force it on
 someone and likely won't do it myself ...)

I consider it undefined because SMD says that an OS musn't configure
this behavior and doesn't say what should happen if the OS does => we
could do anything.  (Killing the guest would be great for debugging OS
issues, but ours behavior is fairly conservative.)

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: Radim Krčmář [mailto:rkrc...@redhat.com]
> Sent: Friday, January 22, 2016 3:50 AM
> To: Wu, Feng 
> Cc: pbonz...@redhat.com; linux-kernel@vger.kernel.org;
> k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> 2016-01-20 09:42+0800, Feng Wu:
> > Use vector-hashing to deliver lowest-priority interrupts, As an
> > example, modern Intel CPUs in server platform use this method to
> > handle lowest-priority interrupts.
> >
> > Signed-off-by: Feng Wu 
> > ---
> 
> Functionality looks good, so I had a lot of stylistic comments, sorry :)

Any comments are welcome! Thank you! :)

> 
> > +  const unsigned long *bitmap, u32 bitmap_size)
> > +{
> > +   u32 mod;
> > +   int i, idx = 0;
> > +
> > +   mod = vector % dest_vcpus;
> > +
> > +   for (i = 0; i <= mod; i++) {
> > +   idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
> 
> I'd remove this "+ 1".  Current users don't check for errors and always
> do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
> return type.
> 

Does the following code look good to you:

u32 mod;
int i, idx = -1;

mod = vector % dest_vcpus;

for (i = 0; i <= mod; i++) {
idx = find_next_bit(bitmap, bitmap_size, idx + 1);
BUG_ON(idx == bitmap_size);
}

return idx;

Thanks,
Feng

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/22 1:21, rkrc...@redhat.com wrote:

2016-01-21 05:33+, Wu, Feng:

From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
ow...@vger.kernel.org] On Behalf Of Yang Zhang
On 2016/1/20 9:42, Feng Wu wrote:

+   /*
+* We may find a hardware disabled LAPIC here, if

that

+* is the case, print out a error message once for each
+* guest and return.
+*/
+   if (!dst[idx-1] &&
+   (kvm->arch.disabled_lapic_found == 0)) {
+   kvm->arch.disabled_lapic_found = 1;
+   printk(KERN_ERR
+   "Disabled LAPIC found during irq

injection\n");

+   goto out;


What does "goto out" mean? Inject successfully or fail? According the
value of ret which is set to ture here, it means inject successfully but


(true actually means that fast path did the job and slow path isn't
  needed.)


i = -1.


(I think there isn't a practical difference between *r=-1 and *r=0.)


Currently, if *r == -1, the remote_irr may get set. But it seems wrong. 
I need to have a double check to see whether it is a bug in current code.





Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.


'ret = true' is the better one.  We know that the interrupt is not
deliverable [1], so there's no point in trying to deliver with the slow
path.  We behave similarly when the interrupt targets a single disabled
APIC.

---
1: Well ... it's possible that slowpath would deliver it thanks to
different handling of disabled APICs, but it's undefined behavior,


why it is undefined behavior? Besides, why we will keep two different 
handling logic for the fast path and slow path? It looks weird.



so it doesn't matter matter if we don't try.




--
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: rkrc...@redhat.com [mailto:rkrc...@redhat.com]
> Sent: Friday, January 22, 2016 1:21 AM
> To: Wu, Feng 
> Cc: Yang Zhang ; pbonz...@redhat.com; linux-
> ker...@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts



> > Oh, I didn't notice 'ret' is initialized to true, I thought it was 
> > initialized
> > to false like another function, I should add a "ret = false' here. We should
> > failed to inject the interrupt since hardware disabled LAPIC is found.
> 
> 'ret = true' is the better one.  We know that the interrupt is not
> deliverable [1], so there's no point in trying to deliver with the slow
> path.  We behave similarly when the interrupt targets a single disabled
> APIC.

Oh, yes, you are right, Thanks a lot!

Thanks,
Feng

> 
> ---
> 1: Well ... it's possible that slowpath would deliver it thanks to
>different handling of disabled APICs, but it's undefined behavior,
>so it doesn't matter matter if we don't try.

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krčmář]

2016-01-20 09:42+0800, Feng Wu:
> Use vector-hashing to deliver lowest-priority interrupts, As an
> example, modern Intel CPUs in server platform use this method to
> handle lowest-priority interrupts.
> 
> Signed-off-by: Feng Wu 
> ---

Functionality looks good, so I had a lot of stylistic comments, sorry :)

> v3:
> - Fix a bug for sparse topologies, in that case, vcpu_id is not equal
> to the return value got by kvm_get_vcpu().
> - Remove unnecessary check in fast irq delivery patch.
> - print a error message only once for each guest when we find hardware
>   disabled LAPIC during interrupt injection.
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> @@ -754,6 +754,8 @@ struct kvm_arch {
>  
>   bool irqchip_split;
>   u8 nr_reserved_ioapic_pins;
> +
> + int disabled_lapic_found;

Fits into "bool".

>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -34,6 +34,7 @@
>  #include "lapic.h"
>  
>  #include "hyperv.h"
> +#include "x86.h"
>  
>  static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>  struct kvm *kvm, int irq_source_id, int level,
> @@ -55,8 +56,10 @@ static int kvm_set_ioapic_irq(struct 
> kvm_kernel_irq_routing_entry *e,
>  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>   struct kvm_lapic_irq *irq, unsigned long *dest_map)
>  {
> - int i, r = -1;
> + int i, r = -1, idx = 0;

(No need to initialize idx.)

>   struct kvm_vcpu *vcpu, *lowest = NULL;
> + unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
> + unsigned int dest_vcpus = 0;
>  
>   if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
>   kvm_lowest_prio_delivery(irq)) {
> @@ -80,13 +85,25 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct 
> kvm_lapic *src,
>   r = 0;
>   r += kvm_apic_set_irq(vcpu, irq, dest_map);
>   } else if (kvm_lapic_enabled(vcpu)) {
> - if (!lowest)
> - lowest = vcpu;
> - else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> - lowest = vcpu;
> + if (!kvm_vector_hashing_enabled()) {
> + if (!lowest)
> + lowest = vcpu;
> + else if (kvm_apic_compare_prio(vcpu, lowest) < 
> 0)
> + lowest = vcpu;
> + } else {
> + __set_bit(i, dest_vcpu_bitmap);
> + dest_vcpus++;
> + }
>   }
>   }
>  
> + if (dest_vcpus != 0) {

(I think it's ok to do 'int idx = kvm...')

> + idx = kvm_vector_2_index(irq->vector, dest_vcpus,
> +  dest_vcpu_bitmap, KVM_MAX_VCPUS);
> +
> + lowest = kvm_get_vcpu(kvm, idx - 1);
> + }
> +
>   if (lowest)
>   r = kvm_apic_set_irq(lowest, irq, dest_map);
>  
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> @@ -675,6 +675,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct 
> kvm_lapic *source,
>   }
>  }
>  
> +int kvm_vector_2_index(u32 vector, u32 dest_vcpus,

(The "2" in name is inconsistent, other functions use "to".)

> +const unsigned long *bitmap, u32 bitmap_size)
> +{
> + u32 mod;
> + int i, idx = 0;
> +
> + mod = vector % dest_vcpus;
> +
> + for (i = 0; i <= mod; i++) {
> + idx = find_next_bit(bitmap, bitmap_size, idx) + 1;

I'd remove this "+ 1".  Current users don't check for errors and always
do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
return type.

> + BUG_ON(idx > bitmap_size);
> + }
> +
> + return idx;
> +}
> +
>  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>   struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
>  {
> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
> struct kvm_lapic *src,
>  
>   dst = map->logical_map[cid];
>  
> - if (kvm_lowest_prio_delivery(irq)) {
> + if (!kvm_lowest_prio_delivery(irq))
> + goto set_irq;
> +
> + if (!kvm_vector_hashing_enabled()) {
>   int l = -1;
>   for_each_set_bit(i, , 16) {
>   if (!dst[i])
>   continue;
>   if (l < 0)
>   l = i;
> - else if (kvm_apic_compare_prio(dst[i]->vcpu, 
> dst[l]->vcpu) < 0)
> + else if (kvm_apic_compare_prio(dst[i]->vcpu,
> + dst[l]->vcpu) < 0)
>

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[rkrc...@redhat.com]

2016-01-21 05:33+, Wu, Feng:
>> From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
>> ow...@vger.kernel.org] On Behalf Of Yang Zhang
>> On 2016/1/20 9:42, Feng Wu wrote:
>> > +  /*
>> > +   * We may find a hardware disabled LAPIC here, if
>> that
>> > +   * is the case, print out a error message once for each
>> > +   * guest and return.
>> > +   */
>> > +  if (!dst[idx-1] &&
>> > +  (kvm->arch.disabled_lapic_found == 0)) {
>> > +  kvm->arch.disabled_lapic_found = 1;
>> > +  printk(KERN_ERR
>> > +  "Disabled LAPIC found during irq
>> injection\n");
>> > +  goto out;
>> 
>> What does "goto out" mean? Inject successfully or fail? According the
>> value of ret which is set to ture here, it means inject successfully but

(true actually means that fast path did the job and slow path isn't
 needed.)

>> i = -1.

(I think there isn't a practical difference between *r=-1 and *r=0.)

> Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
> to false like another function, I should add a "ret = false' here. We should
> failed to inject the interrupt since hardware disabled LAPIC is found.

'ret = true' is the better one.  We know that the interrupt is not
deliverable [1], so there's no point in trying to deliver with the slow
path.  We behave similarly when the interrupt targets a single disabled
APIC.

---
1: Well ... it's possible that slowpath would deliver it thanks to
   different handling of disabled APICs, but it's undefined behavior,
   so it doesn't matter matter if we don't try.

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[rkrc...@redhat.com]

2016-01-21 05:33+, Wu, Feng:
>> From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
>> ow...@vger.kernel.org] On Behalf Of Yang Zhang
>> On 2016/1/20 9:42, Feng Wu wrote:
>> > +  /*
>> > +   * We may find a hardware disabled LAPIC here, if
>> that
>> > +   * is the case, print out a error message once for each
>> > +   * guest and return.
>> > +   */
>> > +  if (!dst[idx-1] &&
>> > +  (kvm->arch.disabled_lapic_found == 0)) {
>> > +  kvm->arch.disabled_lapic_found = 1;
>> > +  printk(KERN_ERR
>> > +  "Disabled LAPIC found during irq
>> injection\n");
>> > +  goto out;
>> 
>> What does "goto out" mean? Inject successfully or fail? According the
>> value of ret which is set to ture here, it means inject successfully but

(true actually means that fast path did the job and slow path isn't
 needed.)

>> i = -1.

(I think there isn't a practical difference between *r=-1 and *r=0.)

> Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
> to false like another function, I should add a "ret = false' here. We should
> failed to inject the interrupt since hardware disabled LAPIC is found.

'ret = true' is the better one.  We know that the interrupt is not
deliverable [1], so there's no point in trying to deliver with the slow
path.  We behave similarly when the interrupt targets a single disabled
APIC.

---
1: Well ... it's possible that slowpath would deliver it thanks to
   different handling of disabled APICs, but it's undefined behavior,
   so it doesn't matter matter if we don't try.

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krčmář]

2016-01-20 09:42+0800, Feng Wu:
> Use vector-hashing to deliver lowest-priority interrupts, As an
> example, modern Intel CPUs in server platform use this method to
> handle lowest-priority interrupts.
> 
> Signed-off-by: Feng Wu 
> ---

Functionality looks good, so I had a lot of stylistic comments, sorry :)

> v3:
> - Fix a bug for sparse topologies, in that case, vcpu_id is not equal
> to the return value got by kvm_get_vcpu().
> - Remove unnecessary check in fast irq delivery patch.
> - print a error message only once for each guest when we find hardware
>   disabled LAPIC during interrupt injection.
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> @@ -754,6 +754,8 @@ struct kvm_arch {
>  
>   bool irqchip_split;
>   u8 nr_reserved_ioapic_pins;
> +
> + int disabled_lapic_found;

Fits into "bool".

>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -34,6 +34,7 @@
>  #include "lapic.h"
>  
>  #include "hyperv.h"
> +#include "x86.h"
>  
>  static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>  struct kvm *kvm, int irq_source_id, int level,
> @@ -55,8 +56,10 @@ static int kvm_set_ioapic_irq(struct 
> kvm_kernel_irq_routing_entry *e,
>  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>   struct kvm_lapic_irq *irq, unsigned long *dest_map)
>  {
> - int i, r = -1;
> + int i, r = -1, idx = 0;

(No need to initialize idx.)

>   struct kvm_vcpu *vcpu, *lowest = NULL;
> + unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
> + unsigned int dest_vcpus = 0;
>  
>   if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
>   kvm_lowest_prio_delivery(irq)) {
> @@ -80,13 +85,25 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct 
> kvm_lapic *src,
>   r = 0;
>   r += kvm_apic_set_irq(vcpu, irq, dest_map);
>   } else if (kvm_lapic_enabled(vcpu)) {
> - if (!lowest)
> - lowest = vcpu;
> - else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> - lowest = vcpu;
> + if (!kvm_vector_hashing_enabled()) {
> + if (!lowest)
> + lowest = vcpu;
> + else if (kvm_apic_compare_prio(vcpu, lowest) < 
> 0)
> + lowest = vcpu;
> + } else {
> + __set_bit(i, dest_vcpu_bitmap);
> + dest_vcpus++;
> + }
>   }
>   }
>  
> + if (dest_vcpus != 0) {

(I think it's ok to do 'int idx = kvm...')

> + idx = kvm_vector_2_index(irq->vector, dest_vcpus,
> +  dest_vcpu_bitmap, KVM_MAX_VCPUS);
> +
> + lowest = kvm_get_vcpu(kvm, idx - 1);
> + }
> +
>   if (lowest)
>   r = kvm_apic_set_irq(lowest, irq, dest_map);
>  
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> @@ -675,6 +675,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct 
> kvm_lapic *source,
>   }
>  }
>  
> +int kvm_vector_2_index(u32 vector, u32 dest_vcpus,

(The "2" in name is inconsistent, other functions use "to".)

> +const unsigned long *bitmap, u32 bitmap_size)
> +{
> + u32 mod;
> + int i, idx = 0;
> +
> + mod = vector % dest_vcpus;
> +
> + for (i = 0; i <= mod; i++) {
> + idx = find_next_bit(bitmap, bitmap_size, idx) + 1;

I'd remove this "+ 1".  Current users don't check for errors and always
do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
return type.

> + BUG_ON(idx > bitmap_size);
> + }
> +
> + return idx;
> +}
> +
>  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>   struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
>  {
> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
> struct kvm_lapic *src,
>  
>   dst = map->logical_map[cid];
>  
> - if (kvm_lowest_prio_delivery(irq)) {
> + if (!kvm_lowest_prio_delivery(irq))
> + goto set_irq;
> +
> + if (!kvm_vector_hashing_enabled()) {
>   int l = -1;
>   for_each_set_bit(i, , 16) {
>   if (!dst[i])
>   continue;
>   if (l < 0)
>   l = i;
> - else if (kvm_apic_compare_prio(dst[i]->vcpu, 
> dst[l]->vcpu) < 0)
> + else if (kvm_apic_compare_prio(dst[i]->vcpu,
> + dst[l]->vcpu) < 0)
> 

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/22 1:21, rkrc...@redhat.com wrote:

2016-01-21 05:33+, Wu, Feng:

From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
ow...@vger.kernel.org] On Behalf Of Yang Zhang
On 2016/1/20 9:42, Feng Wu wrote:

+   /*
+* We may find a hardware disabled LAPIC here, if

that

+* is the case, print out a error message once for each
+* guest and return.
+*/
+   if (!dst[idx-1] &&
+   (kvm->arch.disabled_lapic_found == 0)) {
+   kvm->arch.disabled_lapic_found = 1;
+   printk(KERN_ERR
+   "Disabled LAPIC found during irq

injection\n");

+   goto out;


What does "goto out" mean? Inject successfully or fail? According the
value of ret which is set to ture here, it means inject successfully but


(true actually means that fast path did the job and slow path isn't
  needed.)


i = -1.


(I think there isn't a practical difference between *r=-1 and *r=0.)


Currently, if *r == -1, the remote_irr may get set. But it seems wrong. 
I need to have a double check to see whether it is a bug in current code.





Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.


'ret = true' is the better one.  We know that the interrupt is not
deliverable [1], so there's no point in trying to deliver with the slow
path.  We behave similarly when the interrupt targets a single disabled
APIC.

---
1: Well ... it's possible that slowpath would deliver it thanks to
different handling of disabled APICs, but it's undefined behavior,


why it is undefined behavior? Besides, why we will keep two different 
handling logic for the fast path and slow path? It looks weird.



so it doesn't matter matter if we don't try.




--
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: rkrc...@redhat.com [mailto:rkrc...@redhat.com]
> Sent: Friday, January 22, 2016 1:21 AM
> To: Wu, Feng <feng...@intel.com>
> Cc: Yang Zhang <yang.zhang...@gmail.com>; pbonz...@redhat.com; linux-
> ker...@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts



> > Oh, I didn't notice 'ret' is initialized to true, I thought it was 
> > initialized
> > to false like another function, I should add a "ret = false' here. We should
> > failed to inject the interrupt since hardware disabled LAPIC is found.
> 
> 'ret = true' is the better one.  We know that the interrupt is not
> deliverable [1], so there's no point in trying to deliver with the slow
> path.  We behave similarly when the interrupt targets a single disabled
> APIC.

Oh, yes, you are right, Thanks a lot!

Thanks,
Feng

> 
> ---
> 1: Well ... it's possible that slowpath would deliver it thanks to
>different handling of disabled APICs, but it's undefined behavior,
>so it doesn't matter matter if we don't try.

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: Radim Krčmář [mailto:rkrc...@redhat.com]
> Sent: Friday, January 22, 2016 3:50 AM
> To: Wu, Feng <feng...@intel.com>
> Cc: pbonz...@redhat.com; linux-kernel@vger.kernel.org;
> k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> 2016-01-20 09:42+0800, Feng Wu:
> > Use vector-hashing to deliver lowest-priority interrupts, As an
> > example, modern Intel CPUs in server platform use this method to
> > handle lowest-priority interrupts.
> >
> > Signed-off-by: Feng Wu <feng...@intel.com>
> > ---
> 
> Functionality looks good, so I had a lot of stylistic comments, sorry :)

Any comments are welcome! Thank you! :)

> 
> > +  const unsigned long *bitmap, u32 bitmap_size)
> > +{
> > +   u32 mod;
> > +   int i, idx = 0;
> > +
> > +   mod = vector % dest_vcpus;
> > +
> > +   for (i = 0; i <= mod; i++) {
> > +   idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
> 
> I'd remove this "+ 1".  Current users don't check for errors and always
> do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
> return type.
> 

Does the following code look good to you:

u32 mod;
int i, idx = -1;

mod = vector % dest_vcpus;

for (i = 0; i <= mod; i++) {
idx = find_next_bit(bitmap, bitmap_size, idx + 1);
BUG_ON(idx == bitmap_size);
}

return idx;

Thanks,
Feng

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 14:02, Wu, Feng wrote:




-Original Message-
From: Yang Zhang [mailto:yang.zhang...@gmail.com]
Sent: Thursday, January 21, 2016 1:58 PM
To: Wu, Feng ; pbonz...@redhat.com;
rkrc...@redhat.com
Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
priority interrupts



I remember we have discussed that even the LAPIC is software disabled,
it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
messages. Isn't current logic still problematically?


I don't think there are problems, here we only cover lowest-priority mode.


Does Intel SDM said those interrupts cannot be delivered on
lowest-priority mode?


Fixed, Lowest-priority, SMI, NMI, INIT are all "Delivery Mode", once it is
Lowest-priority, it cannot be other type, afaik.


You are correct, I missed it with physical and logical mode. Also, i 
noticed you have the check at the beginning:


+   if (!kvm_lowest_prio_delivery(irq))
+   goto set_irq;

--
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: Yang Zhang [mailto:yang.zhang...@gmail.com]
> Sent: Thursday, January 21, 2016 1:58 PM
> To: Wu, Feng ; pbonz...@redhat.com;
> rkrc...@redhat.com
> Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> >>
> >> I remember we have discussed that even the LAPIC is software disabled,
> >> it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
> >> messages. Isn't current logic still problematically?
> >
> > I don't think there are problems, here we only cover lowest-priority mode.
> 
> Does Intel SDM said those interrupts cannot be delivered on
> lowest-priority mode?

Fixed, Lowest-priority, SMI, NMI, INIT are all "Delivery Mode", once it is
Lowest-priority, it cannot be other type, afaik.

Thanks,
Feng

> 
> CC Jun.
> 
> Hi Jun,
> 
> Do you know whether INIT, NMI, SMI, and SIPI can be delivered through
> lowest-priority mode? I didn't find SDM says no.
> 
> --
> best regards
> yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 13:46, Wu, Feng wrote:




-Original Message-
From: Yang Zhang [mailto:yang.zhang...@gmail.com]
Sent: Thursday, January 21, 2016 1:43 PM
To: Wu, Feng ; pbonz...@redhat.com;
rkrc...@redhat.com
Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
priority interrupts

On 2016/1/21 13:33, Wu, Feng wrote:




-Original Message-
From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
ow...@vger.kernel.org] On Behalf Of Yang Zhang
Sent: Thursday, January 21, 2016 1:24 PM
To: Wu, Feng ; pbonz...@redhat.com;
rkrc...@redhat.com
Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver

lowest-

priority interrupts

On 2016/1/20 9:42, Feng Wu wrote:

Use vector-hashing to deliver lowest-priority interrupts, As an
example, modern Intel CPUs in server platform use this method to
handle lowest-priority interrupts.

Signed-off-by: Feng Wu 
---
bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic

*src,

struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
{
@@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm

*kvm, struct kvm_lapic *src,


dst = map->logical_map[cid];

-   if (kvm_lowest_prio_delivery(irq)) {
+   if (!kvm_lowest_prio_delivery(irq))
+   goto set_irq;
+
+   if (!kvm_vector_hashing_enabled()) {
int l = -1;
for_each_set_bit(i, , 16) {
if (!dst[i])
continue;
if (l < 0)
l = i;
-   else if (kvm_apic_compare_prio(dst[i]->vcpu,

dst[l]->vcpu) < 0)

+   else if (kvm_apic_compare_prio(dst[i]->vcpu,
+   dst[l]->vcpu) < 0)
l = i;
}
-
bitmap = (l >= 0) ? 1 << l : 0;
+   } else {
+   int idx = 0;
+   unsigned int dest_vcpus = 0;
+
+   dest_vcpus = hweight16(bitmap);
+   if (dest_vcpus == 0)
+   goto out;
+
+   idx = kvm_vector_2_index(irq->vector,
+   dest_vcpus, , 16);
+
+   /*
+* We may find a hardware disabled LAPIC here, if

that

+* is the case, print out a error message once for each
+* guest and return.
+*/
+   if (!dst[idx-1] &&
+   (kvm->arch.disabled_lapic_found == 0)) {
+   kvm->arch.disabled_lapic_found = 1;
+   printk(KERN_ERR
+   "Disabled LAPIC found during irq

injection\n");

+   goto out;


What does "goto out" mean? Inject successfully or fail? According the
value of ret which is set to ture here, it means inject successfully but
i = -1.



Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.


I remember we have discussed that even the LAPIC is software disabled,
it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
messages. Isn't current logic still problematically?


I don't think there are problems, here we only cover lowest-priority mode.


Does Intel SDM said those interrupts cannot be delivered on 
lowest-priority mode?


CC Jun.

Hi Jun,

Do you know whether INIT, NMI, SMI, and SIPI can be delivered through 
lowest-priority mode? I didn't find SDM says no.


--
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: Yang Zhang [mailto:yang.zhang...@gmail.com]
> Sent: Thursday, January 21, 2016 1:43 PM
> To: Wu, Feng ; pbonz...@redhat.com;
> rkrc...@redhat.com
> Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> On 2016/1/21 13:33, Wu, Feng wrote:
> >
> >
> >> -Original Message-
> >> From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
> >> ow...@vger.kernel.org] On Behalf Of Yang Zhang
> >> Sent: Thursday, January 21, 2016 1:24 PM
> >> To: Wu, Feng ; pbonz...@redhat.com;
> >> rkrc...@redhat.com
> >> Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> >> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver
> lowest-
> >> priority interrupts
> >>
> >> On 2016/1/20 9:42, Feng Wu wrote:
> >>> Use vector-hashing to deliver lowest-priority interrupts, As an
> >>> example, modern Intel CPUs in server platform use this method to
> >>> handle lowest-priority interrupts.
> >>>
> >>> Signed-off-by: Feng Wu 
> >>> ---
> >>>bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic
> >> *src,
> >>>   struct kvm_lapic_irq *irq, int *r, unsigned long 
> >>> *dest_map)
> >>>{
> >>> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
> >> *kvm, struct kvm_lapic *src,
> >>>
> >>>   dst = map->logical_map[cid];
> >>>
> >>> - if (kvm_lowest_prio_delivery(irq)) {
> >>> + if (!kvm_lowest_prio_delivery(irq))
> >>> + goto set_irq;
> >>> +
> >>> + if (!kvm_vector_hashing_enabled()) {
> >>>   int l = -1;
> >>>   for_each_set_bit(i, , 16) {
> >>>   if (!dst[i])
> >>>   continue;
> >>>   if (l < 0)
> >>>   l = i;
> >>> - else if (kvm_apic_compare_prio(dst[i]->vcpu,
> >> dst[l]->vcpu) < 0)
> >>> + else if (kvm_apic_compare_prio(dst[i]->vcpu,
> >>> + dst[l]->vcpu) < 0)
> >>>   l = i;
> >>>   }
> >>> -
> >>>   bitmap = (l >= 0) ? 1 << l : 0;
> >>> + } else {
> >>> + int idx = 0;
> >>> + unsigned int dest_vcpus = 0;
> >>> +
> >>> + dest_vcpus = hweight16(bitmap);
> >>> + if (dest_vcpus == 0)
> >>> + goto out;
> >>> +
> >>> + idx = kvm_vector_2_index(irq->vector,
> >>> + dest_vcpus, , 16);
> >>> +
> >>> + /*
> >>> +  * We may find a hardware disabled LAPIC here, if
> >> that
> >>> +  * is the case, print out a error message once for each
> >>> +  * guest and return.
> >>> +  */
> >>> + if (!dst[idx-1] &&
> >>> + (kvm->arch.disabled_lapic_found == 0)) {
> >>> + kvm->arch.disabled_lapic_found = 1;
> >>> + printk(KERN_ERR
> >>> + "Disabled LAPIC found during irq
> >> injection\n");
> >>> + goto out;
> >>
> >> What does "goto out" mean? Inject successfully or fail? According the
> >> value of ret which is set to ture here, it means inject successfully but
> >> i = -1.
> >>
> >
> > Oh, I didn't notice 'ret' is initialized to true, I thought it was 
> > initialized
> > to false like another function, I should add a "ret = false' here. We should
> > failed to inject the interrupt since hardware disabled LAPIC is found.
> 
> I remember we have discussed that even the LAPIC is software disabled,
> it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
> messages. Isn't current logic still problematically?

I don't think there are problems, here we only cover lowest-priority mode.

Thanks,
Feng

> 
> --
> best regards
> yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 13:33, Wu, Feng wrote:




-Original Message-
From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
ow...@vger.kernel.org] On Behalf Of Yang Zhang
Sent: Thursday, January 21, 2016 1:24 PM
To: Wu, Feng ; pbonz...@redhat.com;
rkrc...@redhat.com
Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
priority interrupts

On 2016/1/20 9:42, Feng Wu wrote:

Use vector-hashing to deliver lowest-priority interrupts, As an
example, modern Intel CPUs in server platform use this method to
handle lowest-priority interrupts.

Signed-off-by: Feng Wu 
---
   bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic

*src,

struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
   {
@@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm

*kvm, struct kvm_lapic *src,


dst = map->logical_map[cid];

-   if (kvm_lowest_prio_delivery(irq)) {
+   if (!kvm_lowest_prio_delivery(irq))
+   goto set_irq;
+
+   if (!kvm_vector_hashing_enabled()) {
int l = -1;
for_each_set_bit(i, , 16) {
if (!dst[i])
continue;
if (l < 0)
l = i;
-   else if (kvm_apic_compare_prio(dst[i]->vcpu,

dst[l]->vcpu) < 0)

+   else if (kvm_apic_compare_prio(dst[i]->vcpu,
+   dst[l]->vcpu) < 0)
l = i;
}
-
bitmap = (l >= 0) ? 1 << l : 0;
+   } else {
+   int idx = 0;
+   unsigned int dest_vcpus = 0;
+
+   dest_vcpus = hweight16(bitmap);
+   if (dest_vcpus == 0)
+   goto out;
+
+   idx = kvm_vector_2_index(irq->vector,
+   dest_vcpus, , 16);
+
+   /*
+* We may find a hardware disabled LAPIC here, if

that

+* is the case, print out a error message once for each
+* guest and return.
+*/
+   if (!dst[idx-1] &&
+   (kvm->arch.disabled_lapic_found == 0)) {
+   kvm->arch.disabled_lapic_found = 1;
+   printk(KERN_ERR
+   "Disabled LAPIC found during irq

injection\n");

+   goto out;


What does "goto out" mean? Inject successfully or fail? According the
value of ret which is set to ture here, it means inject successfully but
i = -1.



Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.


I remember we have discussed that even the LAPIC is software disabled, 
it still can respond to some interrupts like INIT, NMI, SMI, and SIPI 
messages. Isn't current logic still problematically?


--
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
> ow...@vger.kernel.org] On Behalf Of Yang Zhang
> Sent: Thursday, January 21, 2016 1:24 PM
> To: Wu, Feng ; pbonz...@redhat.com;
> rkrc...@redhat.com
> Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> On 2016/1/20 9:42, Feng Wu wrote:
> > Use vector-hashing to deliver lowest-priority interrupts, As an
> > example, modern Intel CPUs in server platform use this method to
> > handle lowest-priority interrupts.
> >
> > Signed-off-by: Feng Wu 
> > ---
> >   bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic
> *src,
> > struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
> >   {
> > @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
> *kvm, struct kvm_lapic *src,
> >
> > dst = map->logical_map[cid];
> >
> > -   if (kvm_lowest_prio_delivery(irq)) {
> > +   if (!kvm_lowest_prio_delivery(irq))
> > +   goto set_irq;
> > +
> > +   if (!kvm_vector_hashing_enabled()) {
> > int l = -1;
> > for_each_set_bit(i, , 16) {
> > if (!dst[i])
> > continue;
> > if (l < 0)
> > l = i;
> > -   else if (kvm_apic_compare_prio(dst[i]->vcpu,
> dst[l]->vcpu) < 0)
> > +   else if (kvm_apic_compare_prio(dst[i]->vcpu,
> > +   dst[l]->vcpu) < 0)
> > l = i;
> > }
> > -
> > bitmap = (l >= 0) ? 1 << l : 0;
> > +   } else {
> > +   int idx = 0;
> > +   unsigned int dest_vcpus = 0;
> > +
> > +   dest_vcpus = hweight16(bitmap);
> > +   if (dest_vcpus == 0)
> > +   goto out;
> > +
> > +   idx = kvm_vector_2_index(irq->vector,
> > +   dest_vcpus, , 16);
> > +
> > +   /*
> > +* We may find a hardware disabled LAPIC here, if
> that
> > +* is the case, print out a error message once for each
> > +* guest and return.
> > +*/
> > +   if (!dst[idx-1] &&
> > +   (kvm->arch.disabled_lapic_found == 0)) {
> > +   kvm->arch.disabled_lapic_found = 1;
> > +   printk(KERN_ERR
> > +   "Disabled LAPIC found during irq
> injection\n");
> > +   goto out;
> 
> What does "goto out" mean? Inject successfully or fail? According the
> value of ret which is set to ture here, it means inject successfully but
> i = -1.
> 

Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.

Thanks,
Feng

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/20 9:42, Feng Wu wrote:

Use vector-hashing to deliver lowest-priority interrupts, As an
example, modern Intel CPUs in server platform use this method to
handle lowest-priority interrupts.

Signed-off-by: Feng Wu 
---
v3:
- Fix a bug for sparse topologies, in that case, vcpu_id is not equal
to the return value got by kvm_get_vcpu().
- Remove unnecessary check in fast irq delivery patch.
- print a error message only once for each guest when we find hardware
   disabled LAPIC during interrupt injection.

  arch/x86/include/asm/kvm_host.h |  2 ++
  arch/x86/kvm/irq_comm.c | 27 +
  arch/x86/kvm/lapic.c| 52 ++---
  arch/x86/kvm/lapic.h|  2 ++
  arch/x86/kvm/x86.c  |  9 +++
  arch/x86/kvm/x86.h  |  1 +
  6 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 44adbb8..5054810 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -754,6 +754,8 @@ struct kvm_arch {

bool irqchip_split;
u8 nr_reserved_ioapic_pins;
+
+   int disabled_lapic_found;
  };

  struct kvm_vm_stat {
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 8fc89ef..062e907 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -34,6 +34,7 @@
  #include "lapic.h"

  #include "hyperv.h"
+#include "x86.h"

  static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
   struct kvm *kvm, int irq_source_id, int level,
@@ -55,8 +56,10 @@ static int kvm_set_ioapic_irq(struct 
kvm_kernel_irq_routing_entry *e,
  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq, unsigned long *dest_map)
  {
-   int i, r = -1;
+   int i, r = -1, idx = 0;
struct kvm_vcpu *vcpu, *lowest = NULL;
+   unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
+   unsigned int dest_vcpus = 0;

if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
kvm_lowest_prio_delivery(irq)) {
@@ -67,6 +70,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct 
kvm_lapic *src,
if (kvm_irq_delivery_to_apic_fast(kvm, src, irq, , dest_map))
return r;

+   memset(dest_vcpu_bitmap, 0, sizeof(dest_vcpu_bitmap));
+
kvm_for_each_vcpu(i, vcpu, kvm) {
if (!kvm_apic_present(vcpu))
continue;
@@ -80,13 +85,25 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct 
kvm_lapic *src,
r = 0;
r += kvm_apic_set_irq(vcpu, irq, dest_map);
} else if (kvm_lapic_enabled(vcpu)) {
-   if (!lowest)
-   lowest = vcpu;
-   else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
-   lowest = vcpu;
+   if (!kvm_vector_hashing_enabled()) {
+   if (!lowest)
+   lowest = vcpu;
+   else if (kvm_apic_compare_prio(vcpu, lowest) < 
0)
+   lowest = vcpu;
+   } else {
+   __set_bit(i, dest_vcpu_bitmap);
+   dest_vcpus++;
+   }
}
}

+   if (dest_vcpus != 0) {
+   idx = kvm_vector_2_index(irq->vector, dest_vcpus,
+dest_vcpu_bitmap, KVM_MAX_VCPUS);
+
+   lowest = kvm_get_vcpu(kvm, idx - 1);
+   }
+
if (lowest)
r = kvm_apic_set_irq(lowest, irq, dest_map);

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 36591fa..e1a449da 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -675,6 +675,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct 
kvm_lapic *source,
}
  }

+int kvm_vector_2_index(u32 vector, u32 dest_vcpus,
+  const unsigned long *bitmap, u32 bitmap_size)
+{
+   u32 mod;
+   int i, idx = 0;
+
+   mod = vector % dest_vcpus;
+
+   for (i = 0; i <= mod; i++) {
+   idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
+   BUG_ON(idx > bitmap_size);
+   }
+
+   return idx;
+}
+
  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
  {
@@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
struct kvm_lapic *src,

dst = map->logical_map[cid];

-   if (kvm_lowest_prio_delivery(irq)) {
+   if (!kvm_lowest_prio_delivery(irq))
+   goto set_irq;
+
+   if (!kvm_vector_hashing_enabled()) {
int l = -1;
  

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
> ow...@vger.kernel.org] On Behalf Of Yang Zhang
> Sent: Thursday, January 21, 2016 1:24 PM
> To: Wu, Feng <feng...@intel.com>; pbonz...@redhat.com;
> rkrc...@redhat.com
> Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> On 2016/1/20 9:42, Feng Wu wrote:
> > Use vector-hashing to deliver lowest-priority interrupts, As an
> > example, modern Intel CPUs in server platform use this method to
> > handle lowest-priority interrupts.
> >
> > Signed-off-by: Feng Wu <feng...@intel.com>
> > ---
> >   bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic
> *src,
> > struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
> >   {
> > @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
> *kvm, struct kvm_lapic *src,
> >
> > dst = map->logical_map[cid];
> >
> > -   if (kvm_lowest_prio_delivery(irq)) {
> > +   if (!kvm_lowest_prio_delivery(irq))
> > +   goto set_irq;
> > +
> > +   if (!kvm_vector_hashing_enabled()) {
> > int l = -1;
> > for_each_set_bit(i, , 16) {
> > if (!dst[i])
> > continue;
> > if (l < 0)
> > l = i;
> > -   else if (kvm_apic_compare_prio(dst[i]->vcpu,
> dst[l]->vcpu) < 0)
> > +   else if (kvm_apic_compare_prio(dst[i]->vcpu,
> > +   dst[l]->vcpu) < 0)
> > l = i;
> > }
> > -
> > bitmap = (l >= 0) ? 1 << l : 0;
> > +   } else {
> > +   int idx = 0;
> > +   unsigned int dest_vcpus = 0;
> > +
> > +   dest_vcpus = hweight16(bitmap);
> > +   if (dest_vcpus == 0)
> > +   goto out;
> > +
> > +   idx = kvm_vector_2_index(irq->vector,
> > +   dest_vcpus, , 16);
> > +
> > +   /*
> > +* We may find a hardware disabled LAPIC here, if
> that
> > +* is the case, print out a error message once for each
> > +* guest and return.
> > +*/
> > +   if (!dst[idx-1] &&
> > +   (kvm->arch.disabled_lapic_found == 0)) {
> > +   kvm->arch.disabled_lapic_found = 1;
> > +   printk(KERN_ERR
> > +   "Disabled LAPIC found during irq
> injection\n");
> > +   goto out;
> 
> What does "goto out" mean? Inject successfully or fail? According the
> value of ret which is set to ture here, it means inject successfully but
> i = -1.
> 

Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.

Thanks,
Feng

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 13:33, Wu, Feng wrote:




-Original Message-
From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
ow...@vger.kernel.org] On Behalf Of Yang Zhang
Sent: Thursday, January 21, 2016 1:24 PM
To: Wu, Feng <feng...@intel.com>; pbonz...@redhat.com;
rkrc...@redhat.com
Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
priority interrupts

On 2016/1/20 9:42, Feng Wu wrote:

Use vector-hashing to deliver lowest-priority interrupts, As an
example, modern Intel CPUs in server platform use this method to
handle lowest-priority interrupts.

Signed-off-by: Feng Wu <feng...@intel.com>
---
   bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic

*src,

struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
   {
@@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm

*kvm, struct kvm_lapic *src,


dst = map->logical_map[cid];

-   if (kvm_lowest_prio_delivery(irq)) {
+   if (!kvm_lowest_prio_delivery(irq))
+   goto set_irq;
+
+   if (!kvm_vector_hashing_enabled()) {
int l = -1;
for_each_set_bit(i, , 16) {
if (!dst[i])
continue;
if (l < 0)
l = i;
-   else if (kvm_apic_compare_prio(dst[i]->vcpu,

dst[l]->vcpu) < 0)

+   else if (kvm_apic_compare_prio(dst[i]->vcpu,
+   dst[l]->vcpu) < 0)
l = i;
}
-
bitmap = (l >= 0) ? 1 << l : 0;
+   } else {
+   int idx = 0;
+   unsigned int dest_vcpus = 0;
+
+   dest_vcpus = hweight16(bitmap);
+   if (dest_vcpus == 0)
+   goto out;
+
+   idx = kvm_vector_2_index(irq->vector,
+   dest_vcpus, , 16);
+
+   /*
+* We may find a hardware disabled LAPIC here, if

that

+* is the case, print out a error message once for each
+* guest and return.
+*/
+   if (!dst[idx-1] &&
+   (kvm->arch.disabled_lapic_found == 0)) {
+   kvm->arch.disabled_lapic_found = 1;
+   printk(KERN_ERR
+   "Disabled LAPIC found during irq

injection\n");

+   goto out;


What does "goto out" mean? Inject successfully or fail? According the
value of ret which is set to ture here, it means inject successfully but
i = -1.



Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.


I remember we have discussed that even the LAPIC is software disabled, 
it still can respond to some interrupts like INIT, NMI, SMI, and SIPI 
messages. Isn't current logic still problematically?


--
best regards
yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 13:46, Wu, Feng wrote:




-Original Message-
From: Yang Zhang [mailto:yang.zhang...@gmail.com]
Sent: Thursday, January 21, 2016 1:43 PM
To: Wu, Feng <feng...@intel.com>; pbonz...@redhat.com;
rkrc...@redhat.com
Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
priority interrupts

On 2016/1/21 13:33, Wu, Feng wrote:




-Original Message-
From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
ow...@vger.kernel.org] On Behalf Of Yang Zhang
Sent: Thursday, January 21, 2016 1:24 PM
To: Wu, Feng <feng...@intel.com>; pbonz...@redhat.com;
rkrc...@redhat.com
Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver

lowest-

priority interrupts

On 2016/1/20 9:42, Feng Wu wrote:

Use vector-hashing to deliver lowest-priority interrupts, As an
example, modern Intel CPUs in server platform use this method to
handle lowest-priority interrupts.

Signed-off-by: Feng Wu <feng...@intel.com>
---
bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic

*src,

struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
{
@@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm

*kvm, struct kvm_lapic *src,


dst = map->logical_map[cid];

-   if (kvm_lowest_prio_delivery(irq)) {
+   if (!kvm_lowest_prio_delivery(irq))
+   goto set_irq;
+
+   if (!kvm_vector_hashing_enabled()) {
int l = -1;
for_each_set_bit(i, , 16) {
if (!dst[i])
continue;
if (l < 0)
l = i;
-   else if (kvm_apic_compare_prio(dst[i]->vcpu,

dst[l]->vcpu) < 0)

+   else if (kvm_apic_compare_prio(dst[i]->vcpu,
+   dst[l]->vcpu) < 0)
l = i;
}
-
bitmap = (l >= 0) ? 1 << l : 0;
+   } else {
+   int idx = 0;
+   unsigned int dest_vcpus = 0;
+
+   dest_vcpus = hweight16(bitmap);
+   if (dest_vcpus == 0)
+   goto out;
+
+   idx = kvm_vector_2_index(irq->vector,
+   dest_vcpus, , 16);
+
+   /*
+* We may find a hardware disabled LAPIC here, if

that

+* is the case, print out a error message once for each
+* guest and return.
+*/
+   if (!dst[idx-1] &&
+   (kvm->arch.disabled_lapic_found == 0)) {
+   kvm->arch.disabled_lapic_found = 1;
+   printk(KERN_ERR
+   "Disabled LAPIC found during irq

injection\n");

+   goto out;


What does "goto out" mean? Inject successfully or fail? According the
value of ret which is set to ture here, it means inject successfully but
i = -1.



Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.


I remember we have discussed that even the LAPIC is software disabled,
it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
messages. Isn't current logic still problematically?


I don't think there are problems, here we only cover lowest-priority mode.


Does Intel SDM said those interrupts cannot be delivered on 
lowest-priority mode?


CC Jun.

Hi Jun,

Do you know whether INIT, NMI, SMI, and SIPI can be delivered through 
lowest-priority mode? I didn't find SDM says no.


--
best regards
yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 14:02, Wu, Feng wrote:




-Original Message-
From: Yang Zhang [mailto:yang.zhang...@gmail.com]
Sent: Thursday, January 21, 2016 1:58 PM
To: Wu, Feng <feng...@intel.com>; pbonz...@redhat.com;
rkrc...@redhat.com
Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
priority interrupts



I remember we have discussed that even the LAPIC is software disabled,
it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
messages. Isn't current logic still problematically?


I don't think there are problems, here we only cover lowest-priority mode.


Does Intel SDM said those interrupts cannot be delivered on
lowest-priority mode?


Fixed, Lowest-priority, SMI, NMI, INIT are all "Delivery Mode", once it is
Lowest-priority, it cannot be other type, afaik.


You are correct, I missed it with physical and logical mode. Also, i 
noticed you have the check at the beginning:


+   if (!kvm_lowest_prio_delivery(irq))
+   goto set_irq;

--
best regards
yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/20 9:42, Feng Wu wrote:

Use vector-hashing to deliver lowest-priority interrupts, As an
example, modern Intel CPUs in server platform use this method to
handle lowest-priority interrupts.

Signed-off-by: Feng Wu 
---
v3:
- Fix a bug for sparse topologies, in that case, vcpu_id is not equal
to the return value got by kvm_get_vcpu().
- Remove unnecessary check in fast irq delivery patch.
- print a error message only once for each guest when we find hardware
   disabled LAPIC during interrupt injection.

  arch/x86/include/asm/kvm_host.h |  2 ++
  arch/x86/kvm/irq_comm.c | 27 +
  arch/x86/kvm/lapic.c| 52 ++---
  arch/x86/kvm/lapic.h|  2 ++
  arch/x86/kvm/x86.c  |  9 +++
  arch/x86/kvm/x86.h  |  1 +
  6 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 44adbb8..5054810 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -754,6 +754,8 @@ struct kvm_arch {

bool irqchip_split;
u8 nr_reserved_ioapic_pins;
+
+   int disabled_lapic_found;
  };

  struct kvm_vm_stat {
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 8fc89ef..062e907 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -34,6 +34,7 @@
  #include "lapic.h"

  #include "hyperv.h"
+#include "x86.h"

  static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
   struct kvm *kvm, int irq_source_id, int level,
@@ -55,8 +56,10 @@ static int kvm_set_ioapic_irq(struct 
kvm_kernel_irq_routing_entry *e,
  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq, unsigned long *dest_map)
  {
-   int i, r = -1;
+   int i, r = -1, idx = 0;
struct kvm_vcpu *vcpu, *lowest = NULL;
+   unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
+   unsigned int dest_vcpus = 0;

if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
kvm_lowest_prio_delivery(irq)) {
@@ -67,6 +70,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct 
kvm_lapic *src,
if (kvm_irq_delivery_to_apic_fast(kvm, src, irq, , dest_map))
return r;

+   memset(dest_vcpu_bitmap, 0, sizeof(dest_vcpu_bitmap));
+
kvm_for_each_vcpu(i, vcpu, kvm) {
if (!kvm_apic_present(vcpu))
continue;
@@ -80,13 +85,25 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct 
kvm_lapic *src,
r = 0;
r += kvm_apic_set_irq(vcpu, irq, dest_map);
} else if (kvm_lapic_enabled(vcpu)) {
-   if (!lowest)
-   lowest = vcpu;
-   else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
-   lowest = vcpu;
+   if (!kvm_vector_hashing_enabled()) {
+   if (!lowest)
+   lowest = vcpu;
+   else if (kvm_apic_compare_prio(vcpu, lowest) < 
0)
+   lowest = vcpu;
+   } else {
+   __set_bit(i, dest_vcpu_bitmap);
+   dest_vcpus++;
+   }
}
}

+   if (dest_vcpus != 0) {
+   idx = kvm_vector_2_index(irq->vector, dest_vcpus,
+dest_vcpu_bitmap, KVM_MAX_VCPUS);
+
+   lowest = kvm_get_vcpu(kvm, idx - 1);
+   }
+
if (lowest)
r = kvm_apic_set_irq(lowest, irq, dest_map);

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 36591fa..e1a449da 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -675,6 +675,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct 
kvm_lapic *source,
}
  }

+int kvm_vector_2_index(u32 vector, u32 dest_vcpus,
+  const unsigned long *bitmap, u32 bitmap_size)
+{
+   u32 mod;
+   int i, idx = 0;
+
+   mod = vector % dest_vcpus;
+
+   for (i = 0; i <= mod; i++) {
+   idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
+   BUG_ON(idx > bitmap_size);
+   }
+
+   return idx;
+}
+
  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
  {
@@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, 
struct kvm_lapic *src,

dst = map->logical_map[cid];

-   if (kvm_lowest_prio_delivery(irq)) {
+   if (!kvm_lowest_prio_delivery(irq))
+   goto set_irq;
+
+   if (!kvm_vector_hashing_enabled()) {
   

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: Yang Zhang [mailto:yang.zhang...@gmail.com]
> Sent: Thursday, January 21, 2016 1:43 PM
> To: Wu, Feng <feng...@intel.com>; pbonz...@redhat.com;
> rkrc...@redhat.com
> Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> On 2016/1/21 13:33, Wu, Feng wrote:
> >
> >
> >> -Original Message-
> >> From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
> >> ow...@vger.kernel.org] On Behalf Of Yang Zhang
> >> Sent: Thursday, January 21, 2016 1:24 PM
> >> To: Wu, Feng <feng...@intel.com>; pbonz...@redhat.com;
> >> rkrc...@redhat.com
> >> Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> >> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver
> lowest-
> >> priority interrupts
> >>
> >> On 2016/1/20 9:42, Feng Wu wrote:
> >>> Use vector-hashing to deliver lowest-priority interrupts, As an
> >>> example, modern Intel CPUs in server platform use this method to
> >>> handle lowest-priority interrupts.
> >>>
> >>> Signed-off-by: Feng Wu <feng...@intel.com>
> >>> ---
> >>>bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic
> >> *src,
> >>>   struct kvm_lapic_irq *irq, int *r, unsigned long 
> >>> *dest_map)
> >>>{
> >>> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
> >> *kvm, struct kvm_lapic *src,
> >>>
> >>>   dst = map->logical_map[cid];
> >>>
> >>> - if (kvm_lowest_prio_delivery(irq)) {
> >>> + if (!kvm_lowest_prio_delivery(irq))
> >>> + goto set_irq;
> >>> +
> >>> + if (!kvm_vector_hashing_enabled()) {
> >>>   int l = -1;
> >>>   for_each_set_bit(i, , 16) {
> >>>   if (!dst[i])
> >>>   continue;
> >>>   if (l < 0)
> >>>   l = i;
> >>> - else if (kvm_apic_compare_prio(dst[i]->vcpu,
> >> dst[l]->vcpu) < 0)
> >>> + else if (kvm_apic_compare_prio(dst[i]->vcpu,
> >>> + dst[l]->vcpu) < 0)
> >>>   l = i;
> >>>   }
> >>> -
> >>>   bitmap = (l >= 0) ? 1 << l : 0;
> >>> + } else {
> >>> + int idx = 0;
> >>> + unsigned int dest_vcpus = 0;
> >>> +
> >>> + dest_vcpus = hweight16(bitmap);
> >>> + if (dest_vcpus == 0)
> >>> + goto out;
> >>> +
> >>> + idx = kvm_vector_2_index(irq->vector,
> >>> + dest_vcpus, , 16);
> >>> +
> >>> + /*
> >>> +  * We may find a hardware disabled LAPIC here, if
> >> that
> >>> +  * is the case, print out a error message once for each
> >>> +  * guest and return.
> >>> +  */
> >>> + if (!dst[idx-1] &&
> >>> + (kvm->arch.disabled_lapic_found == 0)) {
> >>> + kvm->arch.disabled_lapic_found = 1;
> >>> + printk(KERN_ERR
> >>> + "Disabled LAPIC found during irq
> >> injection\n");
> >>> + goto out;
> >>
> >> What does "goto out" mean? Inject successfully or fail? According the
> >> value of ret which is set to ture here, it means inject successfully but
> >> i = -1.
> >>
> >
> > Oh, I didn't notice 'ret' is initialized to true, I thought it was 
> > initialized
> > to false like another function, I should add a "ret = false' here. We should
> > failed to inject the interrupt since hardware disabled LAPIC is found.
> 
> I remember we have discussed that even the LAPIC is software disabled,
> it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
> messages. Isn't current logic still problematically?

I don't think there are problems, here we only cover lowest-priority mode.

Thanks,
Feng

> 
> --
> best regards
> yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -Original Message-
> From: Yang Zhang [mailto:yang.zhang...@gmail.com]
> Sent: Thursday, January 21, 2016 1:58 PM
> To: Wu, Feng <feng...@intel.com>; pbonz...@redhat.com;
> rkrc...@redhat.com
> Cc: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> >>
> >> I remember we have discussed that even the LAPIC is software disabled,
> >> it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
> >> messages. Isn't current logic still problematically?
> >
> > I don't think there are problems, here we only cover lowest-priority mode.
> 
> Does Intel SDM said those interrupts cannot be delivered on
> lowest-priority mode?

Fixed, Lowest-priority, SMI, NMI, INIT are all "Delivery Mode", once it is
Lowest-priority, it cannot be other type, afaik.

Thanks,
Feng

> 
> CC Jun.
> 
> Hi Jun,
> 
> Do you know whether INIT, NMI, SMI, and SIPI can be delivered through
> lowest-priority mode? I didn't find SDM says no.
> 
> --
> best regards
> yang