Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Radim Krčmář]

2015-09-17 23:18+, Wu, Feng:
>> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
>> On 17/09/2015 17:58, Radim Krčmář wrote:
>>> xAPIC address are only 8 bit long so they always get delivered to x2APIC
>>> cluster 0, where first 16 bits work like xAPIC flat logical mode.
>> 
>> Ok, I was wondering whether this was the correct interpretation.  Thanks!
> 
> Paolo, I don't think Radim clarify your concern, right? Since mda is 8-bit, it
> is wrong with mda >> 16, this is your concern, right?

In case it was:  mda is u32 so the bitshift is defined by C.
(xAPIC destinations in KVM's x2APIC mode are stored in lowest 8 bits of
 mda, hence the cluster is always 0.)

Or am I still missing the point?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Paolo Bonzini]

On 18/09/2015 18:16, Radim Krčmář wrote:
>>> >> Ok, I was wondering whether this was the correct interpretation.  Thanks!
>> > 
>> > Paolo, I don't think Radim clarify your concern, right? Since mda is 
>> > 8-bit, it
>> > is wrong with mda >> 16, this is your concern, right?
> In case it was:  mda is u32 so the bitshift is defined by C.
> (xAPIC destinations in KVM's x2APIC mode are stored in lowest 8 bits of
>  mda, hence the cluster is always 0.)
> 
> Or am I still missing the point?

Yes, remembering that the cluster is always 0 solved my doubt.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Paolo Bonzini]

On 17/09/2015 05:17, Wu, Feng wrote:
>>> > > +   if (irq->dest_mode == APIC_DEST_PHYSICAL) {
>>> > > +   if (irq->dest_id == 0xFF)
>>> > > +   goto out;
>>> > > +
>>> > > +   if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) {
>> > 
>> > Warning here is wrong, the guest can trigger it.
> Could you please share more information about how the guest
> triggers these conditions (including the following two), Thanks
> a lot!

irq->dest_id is a 16-bit value, so it can be > 255.

> + if (!kvm_apic_logical_map_valid(map)) {
> + WARN_ON_ONCE(1);

Here, the guest can trigger it by setting a few APICs in flat mode and
others in cluster mode, for example.

> + if (cid >= ARRAY_SIZE(map->logical_map)) {
> + WARN_ON_ONCE(1);

In x2apic mode irq->dest_id could have bits 12..15 set.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Radim Krčmář]

2015-09-17 16:24+0200, Paolo Bonzini:
> I think kvm_apic_match_logical_addr for MSI and IOAPIC interrupts is
> buggy in x2apic mode.
> 
> It does:
> 
> if (apic_x2apic_mode(apic))
> return ((logical_id >> 16) == (mda >> 16))
>&& (logical_id & mda & 0x) != 0;
> 
> But mda is only 8-bits for MSI and IOAPIC interrupts.
> 
> Radim, should kvm_apic_mda also handle the !ipi && x2apic_mda && dest_id
> != APIC_BROADCAST case?  It never triggers with Linux because it uses
> only the physical mode (that's not super-easy to see;
> ioapic_set_affinity looks for the RTEs in irq_data->chip_data and that
> is allocated with kzalloc).

KVM handles that case, it's just convoluted.
(I wish we scrapped the IR-less x2APIC mode.)

For interrupts from MSI and IOxAPIC:
- Flat logical interrupts are delivered as if we had natural
  (CPU0<->bit0, CPU1<->bit1, ...) flat logical xAPIC for first 8 VCPUs.
- Cluster logical doesn't work much, it's interpreted like flat logical.
  I didn't care about xAPIC cluster because Linux, the sole user of our
  paravirtualized x2APIC, doesn't configure it.

I'll paste kvm_apic_mda() source for better explanation:

  static u32 kvm_apic_mda(unsigned int dest_id, struct kvm_lapic *source,
struct kvm_lapic *target)
  {
bool ipi = source != NULL;
bool x2apic_mda = apic_x2apic_mode(ipi ? source : target);
  
if (!ipi && dest_id == APIC_BROADCAST && x2apic_mda)
return X2APIC_BROADCAST;
  
return x2apic_mda ? dest_id : SET_APIC_DEST_FIELD(dest_id);
  }

MSI/IOxAPIC interrupt means that source is NULL and if the target is in
x2APIC mode, the original 'dest_id' is returned as mda => a flat logical
xAPIC to 0x0f will get interpreted as (cluster) logical x2APIC 0xf in
kvm_apic_match_logical_addr().
xAPIC address are only 8 bit long so they always get delivered to x2APIC
cluster 0, where first 16 bits work like xAPIC flat logical mode.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Paolo Bonzini]

On 17/09/2015 17:58, Radim Krčmář wrote:
> For interrupts from MSI and IOxAPIC:
> - Flat logical interrupts are delivered as if we had natural
>   (CPU0<->bit0, CPU1<->bit1, ...) flat logical xAPIC for first 8 VCPUs.
> - Cluster logical doesn't work much, it's interpreted like flat logical.
>   I didn't care about xAPIC cluster because Linux, the sole user of our
>   paravirtualized x2APIC, doesn't configure it.
> 
> I'll paste kvm_apic_mda() source for better explanation:
> 
>   static u32 kvm_apic_mda(unsigned int dest_id, struct kvm_lapic *source,
> struct kvm_lapic *target)
>   {
>   bool ipi = source != NULL;
>   bool x2apic_mda = apic_x2apic_mode(ipi ? source : target);
>   
>   if (!ipi && dest_id == APIC_BROADCAST && x2apic_mda)
>   return X2APIC_BROADCAST;
>   
>   return x2apic_mda ? dest_id : SET_APIC_DEST_FIELD(dest_id);
>   }
> 
> MSI/IOxAPIC interrupt means that source is NULL and if the target is in
> x2APIC mode, the original 'dest_id' is returned as mda => a flat logical
> xAPIC to 0x0f will get interpreted as (cluster) logical x2APIC 0xf in
> kvm_apic_match_logical_addr().
> xAPIC address are only 8 bit long so they always get delivered to x2APIC
> cluster 0, where first 16 bits work like xAPIC flat logical mode.

Ok, I was wondering whether this was the correct interpretation.  Thanks!

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Wu, Feng]

> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Thursday, September 17, 2015 5:42 PM
> To: Wu, Feng; alex.william...@redhat.com; j...@8bytes.org;
> mtosa...@redhat.com
> Cc: eric.au...@linaro.org; kvm@vger.kernel.org;
> io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH v8 03/13] KVM: Define a new interface
> kvm_intr_is_single_vcpu()
> 
> 
> 
> On 17/09/2015 05:17, Wu, Feng wrote:
> >>> > > + if (irq->dest_mode == APIC_DEST_PHYSICAL) {
> >>> > > + if (irq->dest_id == 0xFF)
> >>> > > + goto out;
> >>> > > +
> >>> > > + if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) {
> >> >
> >> > Warning here is wrong, the guest can trigger it.
> > Could you please share more information about how the guest
> > triggers these conditions (including the following two), Thanks
> > a lot!
> 
> irq->dest_id is a 16-bit value, so it can be > 255.

Yes, irq->dest_id is defined as u32, but by looking the current KVM
code, seems desst_id is used as an u8 variable, even in x2apic mode
the dest_id will not beyond 255 (except for broadcast dest in in x2apic
mode). Correct me if I am wrong. Thanks a lot!

> 
> > +   if (!kvm_apic_logical_map_valid(map)) {
> > +   WARN_ON_ONCE(1);
> 
> Here, the guest can trigger it by setting a few APICs in flat mode and
> others in cluster mode, for example.

Oh, right, the logical map works only when the destination mode of all
the vCPUs are the same.

> 
> > +   if (cid >= ARRAY_SIZE(map->logical_map)) {
> > +   WARN_ON_ONCE(1);
> 
> In x2apic mode irq->dest_id could have bits 12..15 set.

cid is gotten from bit 16 ..31 of the ldr (in apic_logical_id()), and
in x2apic mode, ldr is constructed in kvm_apic_set_x2apic_id() as
below:

u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));

So in fact, cid is (id >> 4), I cannot think of why cid can beyond 15.
Do I miss something here? Thanks!

Thanks,
Feng

> 
> Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Paolo Bonzini]

>> On 17/09/2015 05:17, Wu, Feng wrote:
>>> +   if (irq->dest_mode == APIC_DEST_PHYSICAL) {
>>> +   if (irq->dest_id == 0xFF)
>>> +   goto out;
>>> +
>>> +   if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) {
>
> Warning here is wrong, the guest can trigger it.
>>> Could you please share more information about how the guest
>>> triggers these conditions (including the following two), Thanks
>>> a lot!
>>
>> irq->dest_id is a 16-bit value, so it can be > 255.
> 
> Yes, irq->dest_id is defined as u32, but by looking the current KVM
> code, seems desst_id is used as an u8 variable, even in x2apic mode
> the dest_id will not beyond 255 (except for broadcast dest in in x2apic
> mode). Correct me if I am wrong. Thanks a lot!

Actually you're right, the MSI destination is only 8 bits.  I was
confused because of

#define  MSI_ADDR_DEST_ID_MASK  0x000

in arch/x86/include/asm/msidef.h.  But there may be a bug, see below...

>>> +   if (cid >= ARRAY_SIZE(map->logical_map)) {
>>> +   WARN_ON_ONCE(1);
>>
>> In x2apic mode irq->dest_id could have bits 12..15 set.
> 
> cid is gotten from bit 16 ..31 of the ldr (in apic_logical_id()), and
> in x2apic mode, ldr is constructed in kvm_apic_set_x2apic_id() as
> below:
> 
> u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));
> 
> So in fact, cid is (id >> 4), I cannot think of why cid can beyond 15.

I think kvm_apic_match_logical_addr for MSI and IOAPIC interrupts is
buggy in x2apic mode.

It does:

if (apic_x2apic_mode(apic))
return ((logical_id >> 16) == (mda >> 16))
   && (logical_id & mda & 0x) != 0;

But mda is only 8-bits for MSI and IOAPIC interrupts.

Radim, should kvm_apic_mda also handle the !ipi && x2apic_mda && dest_id
!= APIC_BROADCAST case?  It never triggers with Linux because it uses
only the physical mode (that's not super-easy to see;
ioapic_set_affinity looks for the RTEs in irq_data->chip_data and that
is allocated with kzalloc).

> Do I miss something here? Thanks!

No, you were right.

But still I think the WARNs are unnecessary; it is conceivable that some
future chipset adds support for more than 8-bits in the dest_id.

Paolo

> Thanks,
> Feng
> 
>>
>> Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Wu, Feng]

> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Friday, September 18, 2015 12:00 AM
> To: Radim Krčmář
> Cc: Wu, Feng; alex.william...@redhat.com; j...@8bytes.org;
> mtosa...@redhat.com; eric.au...@linaro.org; kvm@vger.kernel.org;
> io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH v8 03/13] KVM: Define a new interface
> kvm_intr_is_single_vcpu()
> 
> 
> 
> On 17/09/2015 17:58, Radim Krčmář wrote:
> > For interrupts from MSI and IOxAPIC:
> > - Flat logical interrupts are delivered as if we had natural
> >   (CPU0<->bit0, CPU1<->bit1, ...) flat logical xAPIC for first 8 VCPUs.
> > - Cluster logical doesn't work much, it's interpreted like flat logical.
> >   I didn't care about xAPIC cluster because Linux, the sole user of our
> >   paravirtualized x2APIC, doesn't configure it.
> >
> > I'll paste kvm_apic_mda() source for better explanation:
> >
> >   static u32 kvm_apic_mda(unsigned int dest_id, struct kvm_lapic *source,
> > struct kvm_lapic
> *target)
> >   {
> > bool ipi = source != NULL;
> > bool x2apic_mda = apic_x2apic_mode(ipi ? source : target);
> >
> > if (!ipi && dest_id == APIC_BROADCAST && x2apic_mda)
> > return X2APIC_BROADCAST;
> >
> > return x2apic_mda ? dest_id : SET_APIC_DEST_FIELD(dest_id);
> >   }
> >
> > MSI/IOxAPIC interrupt means that source is NULL and if the target is in
> > x2APIC mode, the original 'dest_id' is returned as mda => a flat logical
> > xAPIC to 0x0f will get interpreted as (cluster) logical x2APIC 0xf in
> > kvm_apic_match_logical_addr().
> > xAPIC address are only 8 bit long so they always get delivered to x2APIC
> > cluster 0, where first 16 bits work like xAPIC flat logical mode.
> 
> Ok, I was wondering whether this was the correct interpretation.  Thanks!

Paolo, I don't think Radim clarify your concern, right? Since mda is 8-bit, it
is wrong with mda >> 16, this is your concern, right?

Thanks,
Feng

> 
> Paolo
N�r��yb�X��ǧv�^�)޺{.n�+h����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

RE: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Wu, Feng]

> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Thursday, September 17, 2015 10:25 PM
> To: Wu, Feng; alex.william...@redhat.com; j...@8bytes.org;
> mtosa...@redhat.com
> Cc: eric.au...@linaro.org; kvm@vger.kernel.org;
> io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org; Radim
> Krčmář
> Subject: Re: [PATCH v8 03/13] KVM: Define a new interface
> kvm_intr_is_single_vcpu()
> 
> >> On 17/09/2015 05:17, Wu, Feng wrote:
> >>>>>>> + if (irq->dest_mode == APIC_DEST_PHYSICAL) {
> >>>>>>> + if (irq->dest_id == 0xFF)
> >>>>>>> + goto out;
> >>>>>>> +
> >>>>>>> + if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) {
> >>>>>
> >>>>> Warning here is wrong, the guest can trigger it.
> >>> Could you please share more information about how the guest
> >>> triggers these conditions (including the following two), Thanks
> >>> a lot!
> >>
> >> irq->dest_id is a 16-bit value, so it can be > 255.
> >
> > Yes, irq->dest_id is defined as u32, but by looking the current KVM
> > code, seems desst_id is used as an u8 variable, even in x2apic mode
> > the dest_id will not beyond 255 (except for broadcast dest in in x2apic
> > mode). Correct me if I am wrong. Thanks a lot!
> 
> Actually you're right, the MSI destination is only 8 bits.  I was
> confused because of
> 
> #defineMSI_ADDR_DEST_ID_MASK  0x000
> 
> in arch/x86/include/asm/msidef.h.  But there may be a bug, see below...
> 
> >>> + if (cid >= ARRAY_SIZE(map->logical_map)) {
> >>> + WARN_ON_ONCE(1);
> >>
> >> In x2apic mode irq->dest_id could have bits 12..15 set.
> >
> > cid is gotten from bit 16 ..31 of the ldr (in apic_logical_id()), and
> > in x2apic mode, ldr is constructed in kvm_apic_set_x2apic_id() as
> > below:
> >
> > u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));
> >
> > So in fact, cid is (id >> 4), I cannot think of why cid can beyond 15.
> 
> I think kvm_apic_match_logical_addr for MSI and IOAPIC interrupts is
> buggy in x2apic mode.
> 
> It does:
> 
> if (apic_x2apic_mode(apic))
> return ((logical_id >> 16) == (mda >> 16))
>&& (logical_id & mda & 0x) != 0;
> 
> But mda is only 8-bits for MSI and IOAPIC interrupts.
> 
> Radim, should kvm_apic_mda also handle the !ipi && x2apic_mda && dest_id
> != APIC_BROADCAST case?  It never triggers with Linux because it uses
> only the physical mode (that's not super-easy to see;
> ioapic_set_affinity looks for the RTEs in irq_data->chip_data and that
> is allocated with kzalloc).
> 
> > Do I miss something here? Thanks!
> 
> No, you were right.
> 
> But still I think the WARNs are unnecessary; it is conceivable that some
> future chipset adds support for more than 8-bits in the dest_id.

No problem, I agree with it. Here I just want clarify some questions, thanks
for the elaboration!

Thanks,
Feng

> 
> Paolo
> 
> > Thanks,
> > Feng
> >
> >>
> >> Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Feng Wu]

This patch defines a new interface kvm_intr_is_single_vcpu(),
which can returns whether the interrupt is for single-CPU or not.

It is used by VT-d PI, since now we only support single-CPU
interrupts, For lowest-priority interrupts, if user configures
it via /proc/irq or uses irqbalance to make it single-CPU, we
can use PI to deliver the interrupts to it. Full functionality
of lowest-priority support will be added later.

Signed-off-by: Feng Wu 
---
v8:
- Some optimizations in kvm_intr_is_single_vcpu().
- Expose kvm_intr_is_single_vcpu() so we can use it in vmx code.
- Add kvm_intr_is_single_vcpu_fast() as the fast path to find
  the target vCPU for the single-destination interrupt

 arch/x86/include/asm/kvm_host.h |  3 ++
 arch/x86/kvm/irq_comm.c | 94 +
 arch/x86/kvm/lapic.c|  5 +--
 arch/x86/kvm/lapic.h|  2 +
 4 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 49ec903..af11bca 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1204,4 +1204,7 @@ int __x86_set_memory_region(struct kvm *kvm,
 int x86_set_memory_region(struct kvm *kvm,
  const struct kvm_userspace_memory_region *mem);
 
+bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
+struct kvm_vcpu **dest_vcpu);
+
 #endif /* _ASM_X86_KVM_HOST_H */
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 9efff9e..97ba1d6 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -297,6 +297,100 @@ out:
return r;
 }
 
+static bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm,
+struct kvm_lapic_irq *irq,
+struct kvm_vcpu **dest_vcpu)
+{
+   struct kvm_apic_map *map;
+   bool ret = false;
+   struct kvm_lapic *dst = NULL;
+
+   if (irq->shorthand)
+   return false;
+
+   rcu_read_lock();
+   map = rcu_dereference(kvm->arch.apic_map);
+
+   if (!map)
+   goto out;
+
+   if (irq->dest_mode == APIC_DEST_PHYSICAL) {
+   if (irq->dest_id == 0xFF)
+   goto out;
+
+   if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) {
+   WARN_ON_ONCE(1);
+   goto out;
+   }
+
+   dst = map->phys_map[irq->dest_id];
+   if (dst && kvm_apic_present(dst->vcpu))
+   *dest_vcpu = dst->vcpu;
+   else
+   goto out;
+   } else {
+   u16 cid;
+   unsigned long bitmap = 1;
+   int i, r = 0;
+
+   if (!kvm_apic_logical_map_valid(map)) {
+   WARN_ON_ONCE(1);
+   goto out;
+   }
+
+   apic_logical_id(map, irq->dest_id, , (u16 *));
+
+   if (cid >= ARRAY_SIZE(map->logical_map)) {
+   WARN_ON_ONCE(1);
+   goto out;
+   }
+
+   for_each_set_bit(i, , 16) {
+   dst = map->logical_map[cid][i];
+   if (++r == 2)
+   goto out;
+   }
+
+   if (dst && kvm_apic_present(dst->vcpu))
+   *dest_vcpu = dst->vcpu;
+   else
+   goto out;
+   }
+
+   ret = true;
+out:
+   rcu_read_unlock();
+   return ret;
+}
+
+
+bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
+struct kvm_vcpu **dest_vcpu)
+{
+   int i, r = 0;
+   struct kvm_vcpu *vcpu;
+
+   if (kvm_intr_is_single_vcpu_fast(kvm, irq, dest_vcpu))
+   return true;
+
+   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;
+
+   if (++r == 2)
+   return false;
+
+   *dest_vcpu = vcpu;
+   }
+
+   return r == 1;
+}
+EXPORT_SYMBOL_GPL(kvm_intr_is_single_vcpu);
+
 #define IOAPIC_ROUTING_ENTRY(irq) \
{ .gsi = irq, .type = KVM_IRQ_ROUTING_IRQCHIP,  \
  .u.irqchip = { .irqchip = KVM_IRQCHIP_IOAPIC, .pin = (irq) } }
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 2a5ca97..9848cd50 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -136,13 +136,12 @@ static inline int kvm_apic_id(struct kvm_lapic *apic)
 /* The logical map is definitely wrong if we have multiple
  * modes at the same time.  (Physical map is always right.)
  */
-static inline bool kvm_apic_logical_map_valid(struct kvm_apic_map *map)
+bool 

Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Paolo Bonzini]

On 16/09/2015 10:49, Feng Wu wrote:
> This patch defines a new interface kvm_intr_is_single_vcpu(),
> which can returns whether the interrupt is for single-CPU or not.
> 
> It is used by VT-d PI, since now we only support single-CPU
> interrupts, For lowest-priority interrupts, if user configures
> it via /proc/irq or uses irqbalance to make it single-CPU, we
> can use PI to deliver the interrupts to it. Full functionality
> of lowest-priority support will be added later.
> 
> Signed-off-by: Feng Wu 
> ---
> v8:
> - Some optimizations in kvm_intr_is_single_vcpu().
> - Expose kvm_intr_is_single_vcpu() so we can use it in vmx code.
> - Add kvm_intr_is_single_vcpu_fast() as the fast path to find
>   the target vCPU for the single-destination interrupt
> 
>  arch/x86/include/asm/kvm_host.h |  3 ++
>  arch/x86/kvm/irq_comm.c | 94 
> +
>  arch/x86/kvm/lapic.c|  5 +--
>  arch/x86/kvm/lapic.h|  2 +
>  4 files changed, 101 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 49ec903..af11bca 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1204,4 +1204,7 @@ int __x86_set_memory_region(struct kvm *kvm,
>  int x86_set_memory_region(struct kvm *kvm,
> const struct kvm_userspace_memory_region *mem);
>  
> +bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> +  struct kvm_vcpu **dest_vcpu);
> +
>  #endif /* _ASM_X86_KVM_HOST_H */
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 9efff9e..97ba1d6 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -297,6 +297,100 @@ out:
>   return r;
>  }
>  
> +static bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm,
> +  struct kvm_lapic_irq *irq,
> +  struct kvm_vcpu **dest_vcpu)

Please put this in lapic.c, similar to kvm_irq_delivery_to_apic_fast, so
that you do not have to export other functions.

> +{
> + struct kvm_apic_map *map;
> + bool ret = false;
> + struct kvm_lapic *dst = NULL;
> +
> + if (irq->shorthand)
> + return false;
> +
> + rcu_read_lock();
> + map = rcu_dereference(kvm->arch.apic_map);
> +
> + if (!map)
> + goto out;
> +
> + if (irq->dest_mode == APIC_DEST_PHYSICAL) {
> + if (irq->dest_id == 0xFF)
> + goto out;
> +
> + if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) {

Warning here is wrong, the guest can trigger it.

> + WARN_ON_ONCE(1);
> + goto out;
> + }
> +
> + dst = map->phys_map[irq->dest_id];
> + if (dst && kvm_apic_present(dst->vcpu))
> + *dest_vcpu = dst->vcpu;
> + else
> + goto out;
> + } else {
> + u16 cid;
> + unsigned long bitmap = 1;
> + int i, r = 0;
> +
> + if (!kvm_apic_logical_map_valid(map)) {
> + WARN_ON_ONCE(1);

Same here.

> + goto out;
> + }
> +
> + apic_logical_id(map, irq->dest_id, , (u16 *));
> +
> + if (cid >= ARRAY_SIZE(map->logical_map)) {
> + WARN_ON_ONCE(1);

Same here.

Otherwise looks good.

Paolo

> + goto out;
> + }
> +
> + for_each_set_bit(i, , 16) {
> + dst = map->logical_map[cid][i];
> + if (++r == 2)
> + goto out;
> + }
> +
> + if (dst && kvm_apic_present(dst->vcpu))
> + *dest_vcpu = dst->vcpu;
> + else
> + goto out;
> + }
> +
> + ret = true;
> +out:
> + rcu_read_unlock();
> + return ret;
> +}
> +
> +
> +bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> +  struct kvm_vcpu **dest_vcpu)
> +{
> + int i, r = 0;
> + struct kvm_vcpu *vcpu;
> +
> + if (kvm_intr_is_single_vcpu_fast(kvm, irq, dest_vcpu))
> + return true;
> +
> + 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;
> +
> + if (++r == 2)
> + return false;
> +
> + *dest_vcpu = vcpu;
> + }
> +
> + return r == 1;
> +}
> +EXPORT_SYMBOL_GPL(kvm_intr_is_single_vcpu);
> +
>  #define IOAPIC_ROUTING_ENTRY(irq) \
>   { .gsi = irq, .type = KVM_IRQ_ROUTING_IRQCHIP,  \
> .u.irqchip = { .irqchip = KVM_IRQCHIP_IOAPIC, .pin = (irq) } }
> diff --git 

RE: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu()

[Wu, Feng]

> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Wednesday, September 16, 2015 5:23 PM
> To: Wu, Feng; alex.william...@redhat.com; j...@8bytes.org;
> mtosa...@redhat.com
> Cc: eric.au...@linaro.org; kvm@vger.kernel.org;
> io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH v8 03/13] KVM: Define a new interface
> kvm_intr_is_single_vcpu()
> 
> 
> 
> On 16/09/2015 10:49, Feng Wu wrote:
> > This patch defines a new interface kvm_intr_is_single_vcpu(),
> > which can returns whether the interrupt is for single-CPU or not.
> >
> > It is used by VT-d PI, since now we only support single-CPU
> > interrupts, For lowest-priority interrupts, if user configures
> > it via /proc/irq or uses irqbalance to make it single-CPU, we
> > can use PI to deliver the interrupts to it. Full functionality
> > of lowest-priority support will be added later.
> >
> > Signed-off-by: Feng Wu <feng...@intel.com>
> > ---
> > v8:
> > - Some optimizations in kvm_intr_is_single_vcpu().
> > - Expose kvm_intr_is_single_vcpu() so we can use it in vmx code.
> > - Add kvm_intr_is_single_vcpu_fast() as the fast path to find
> >   the target vCPU for the single-destination interrupt
> >
> >  arch/x86/include/asm/kvm_host.h |  3 ++
> >  arch/x86/kvm/irq_comm.c | 94
> +
> >  arch/x86/kvm/lapic.c|  5 +--
> >  arch/x86/kvm/lapic.h|  2 +
> >  4 files changed, 101 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> > index 49ec903..af11bca 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1204,4 +1204,7 @@ int __x86_set_memory_region(struct kvm *kvm,
> >  int x86_set_memory_region(struct kvm *kvm,
> >   const struct kvm_userspace_memory_region *mem);
> >
> > +bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> > +struct kvm_vcpu **dest_vcpu);
> > +
> >  #endif /* _ASM_X86_KVM_HOST_H */
> > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> > index 9efff9e..97ba1d6 100644
> > --- a/arch/x86/kvm/irq_comm.c
> > +++ b/arch/x86/kvm/irq_comm.c
> > @@ -297,6 +297,100 @@ out:
> > return r;
> >  }
> >
> > +static bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm,
> > +struct kvm_lapic_irq *irq,
> > +struct kvm_vcpu **dest_vcpu)
> 
> Please put this in lapic.c, similar to kvm_irq_delivery_to_apic_fast, so
> that you do not have to export other functions.
> 
> > +{
> > +   struct kvm_apic_map *map;
> > +   bool ret = false;
> > +   struct kvm_lapic *dst = NULL;
> > +
> > +   if (irq->shorthand)
> > +   return false;
> > +
> > +   rcu_read_lock();
> > +   map = rcu_dereference(kvm->arch.apic_map);
> > +
> > +   if (!map)
> > +   goto out;
> > +
> > +   if (irq->dest_mode == APIC_DEST_PHYSICAL) {
> > +   if (irq->dest_id == 0xFF)
> > +   goto out;
> > +
> > +   if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) {
> 
> Warning here is wrong, the guest can trigger it.

Could you please share more information about how the guest
triggers these conditions (including the following two), Thanks
a lot!

Thanks,
Feng

> 
> > +   WARN_ON_ONCE(1);
> > +   goto out;
> > +   }
> > +
> > +   dst = map->phys_map[irq->dest_id];
> > +   if (dst && kvm_apic_present(dst->vcpu))
> > +   *dest_vcpu = dst->vcpu;
> > +   else
> > +   goto out;
> > +   } else {
> > +   u16 cid;
> > +   unsigned long bitmap = 1;
> > +   int i, r = 0;
> > +
> > +   if (!kvm_apic_logical_map_valid(map)) {
> > +   WARN_ON_ONCE(1);
> 
> Same here.
> 
> > +   goto out;
> > +   }
> > +
> > +   apic_logical_id(map, irq->dest_id, , (u16 *));
> > +
> > +   if (cid >= ARRAY_SIZE(map->logical_map)) {
> > +   WARN_ON_ONCE(1);
> 
> Same here.
> 
> Otherwise looks good.
> 
> Paolo
> 
> > +   goto out;
> > +   }
> > +
> > +   for_each_set_bit(