linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall
@ 2018-07-20 16:28 Wanpeng Li
  2018-07-23  5:52 ` Paolo Bonzini
  0 siblings, 1 reply; 7+ messages in thread
From: Wanpeng Li @ 2018-07-20 16:28 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Radim Krčmář, Wanpeng Li, Vitaly Kuznetsov

From: Wanpeng Li <wanpengli@tencent.com>

Using hypercall to send IPIs by one vmexit instead of one by one for
xAPIC/x2APIC physical mode and one vmexit per-cluster for x2APIC cluster 
mode. Intel guest can enter x2apic cluster mode when interrupt remmaping 
is enabled in qemu, however, latest AMD EPYC still just supports xapic 
mode which can get great improvement by Exit-less IPIs. This patchset 
lets a guest which sends multicast IPIs at most can handle 128 vCPUs per 
hypercall on 64-bit machines and 64 vCPUs per hypercall on 32-bit machines.

Hardware: Xeon Skylake 2.5GHz, 2 sockets, 40 cores, 80 threads, the VM 
is 80 vCPUs, IPI microbenchmark(https://lkml.org/lkml/2017/12/19/141):

x2apic cluster mode, vanilla

 Dry-run:                         0,            2392199 ns
 Self-IPI:                  6907514,           15027589 ns
 Normal IPI:              223910476,          251301666 ns
 Broadcast IPI:                   0,         9282161150 ns
 Broadcast lock:                  0,         8812934104 ns

x2apic cluster mode, pv-ipi 

 Dry-run:                         0,            2449341 ns
 Self-IPI:                  6720360,           15028732 ns
 Normal IPI:              228643307,          255708477 ns
 Broadcast IPI:                   0,         7572293590 ns  => 22% performance boost 
 Broadcast lock:                  0,         8316124651 ns

x2apic physical mode, vanilla

 Dry-run:                         0,            3135933 ns
 Self-IPI:                  8572670,           17901757 ns
 Normal IPI:              226444334,          255421709 ns
 Broadcast IPI:                   0,        19845070887 ns
 Broadcast lock:                  0,        19827383656 ns

x2apic physical mode, pv-ipi

 Dry-run:                         0,            2446381 ns
 Self-IPI:                  6788217,           15021056 ns
 Normal IPI:              219454441,          249583458 ns
 Broadcast IPI:                   0,         7806540019 ns  => 154% performance boost 
 Broadcast lock:                  0,         9143618799 ns

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 Documentation/virtual/kvm/hypercalls.txt | 17 ++++++++++++++
 arch/x86/kvm/x86.c                       | 38 ++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/Documentation/virtual/kvm/hypercalls.txt b/Documentation/virtual/kvm/hypercalls.txt
index a890529..912b877 100644
--- a/Documentation/virtual/kvm/hypercalls.txt
+++ b/Documentation/virtual/kvm/hypercalls.txt
@@ -121,3 +121,20 @@ compute the CLOCK_REALTIME for its clock, at the same instant.
 
 Returns KVM_EOPNOTSUPP if the host does not use TSC clocksource,
 or if clock type is different than KVM_CLOCK_PAIRING_WALLCLOCK.
+
+6. KVM_HC_SEND_IPI
+------------------------
+Architecture: x86
+Status: active
+Purpose: Hypercall used to send IPIs.
+
+a0: ipi_bitmap low 64 bits
+a1: ipi_bitmap high 64 bits
+a2: the lowest APIC ID in bitmap
+a3: APIC ICR
+
+The hypercall lets a guest send multicast IPIs at most can handle
+128 vCPUs per hypercall on 64-bit machines and 64 vCPUs per hypercall
+on 32-bit machines.
+
+Returns 0 if successfully delivery the IPIs and 1 if discarded.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2b812b3..016c7e2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6691,6 +6691,41 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
 	kvm_irq_delivery_to_apic(kvm, NULL, &lapic_irq, NULL);
 }
 
+/*
+ * Return 0 if successfully added and 1 if discarded.
+ */
+static int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
+		unsigned long ipi_bitmap_high, int min, int vector, int op_64_bit)
+{
+	int i;
+	struct kvm_apic_map *map;
+	struct kvm_vcpu *vcpu;
+	struct kvm_lapic_irq irq = {
+		.delivery_mode = APIC_DM_FIXED,
+		.vector = vector,
+	};
+
+	rcu_read_lock();
+	map = rcu_dereference(kvm->arch.apic_map);
+
+	for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
+		vcpu = map->phys_map[min + i]->vcpu;
+		if (!kvm_apic_set_irq(vcpu, &irq, NULL))
+			return 1;
+	}
+
+	if (op_64_bit) {
+		for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
+			vcpu = map->phys_map[min + i + BITS_PER_LONG]->vcpu;
+			if (!kvm_apic_set_irq(vcpu, &irq, NULL))
+				return 1;
+		}
+	}
+
+	rcu_read_unlock();
+	return 0;
+}
+
 void kvm_vcpu_deactivate_apicv(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.apicv_active = false;
@@ -6739,6 +6774,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 	case KVM_HC_CLOCK_PAIRING:
 		ret = kvm_pv_clock_pairing(vcpu, a0, a1);
 		break;
+	case KVM_HC_SEND_IPI:
+		ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
+		break;
 #endif
 	default:
 		ret = -KVM_ENOSYS;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall
  2018-07-20 16:28 [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall Wanpeng Li
@ 2018-07-23  5:52 ` Paolo Bonzini
  2018-07-23  6:00   ` Wanpeng Li
  0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2018-07-23  5:52 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm
  Cc: Radim Krčmář, Wanpeng Li, Vitaly Kuznetsov

On 20/07/2018 18:28, Wanpeng Li wrote:
> +a0: ipi_bitmap low 64 bits
> +a1: ipi_bitmap high 64 bits
> +a2: the lowest APIC ID in bitmap
> +a3: APIC ICR
> +
> +The hypercall lets a guest send multicast IPIs at most can handle
> +128 vCPUs per hypercall on 64-bit machines and 64 vCPUs per hypercall
> +on 32-bit machines.
> +
> +Returns 0 if successfully delivery the IPIs and 1 if discarded.

This description does not mention what happens in 32-bit mode.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2b812b3..016c7e2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6691,6 +6691,41 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
>  	kvm_irq_delivery_to_apic(kvm, NULL, &lapic_irq, NULL);
>  }
>  
> +/*
> + * Return 0 if successfully added and 1 if discarded.
> + */
> +static int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> +		unsigned long ipi_bitmap_high, int min, int vector, int op_64_bit)
> +{
> +	int i;
> +	struct kvm_apic_map *map;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_lapic_irq irq = {
> +		.delivery_mode = APIC_DM_FIXED,
> +		.vector = vector,
> +	};
> +
> +	rcu_read_lock();
> +	map = rcu_dereference(kvm->arch.apic_map);
> +
> +	for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> +		vcpu = map->phys_map[min + i]->vcpu;
> +		if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> +			return 1;
> +	}
> +
> +	if (op_64_bit) {
> +		for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> +			vcpu = map->phys_map[min + i + BITS_PER_LONG]->vcpu;
> +			if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> +				return 1;
> +		}
> +	}

The second loop processes the second argument, and it should always run,
even in 32-bit mode.  However, the phys_map index should be min + i + 32
in 32-bit mode and min + i + 64 in 64-bit mode.  (Using BITS_PER_LONG in
the for_each_set_bit length is not a bug instead; you could write it
explicitly as 32 in 32-bit mode, and 64 in 64-bit mode, but I think it's
a little bit more efficient if it's constant).

Paolo
> +


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall
  2018-07-23  5:52 ` Paolo Bonzini
@ 2018-07-23  6:00   ` Wanpeng Li
  2018-07-23  6:09     ` Paolo Bonzini
  2018-07-23  6:10     ` Wanpeng Li
  0 siblings, 2 replies; 7+ messages in thread
From: Wanpeng Li @ 2018-07-23  6:00 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: LKML, kvm, Radim Krcmar, Wanpeng Li, Vitaly Kuznetsov

On Mon, 23 Jul 2018 at 13:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 20/07/2018 18:28, Wanpeng Li wrote:
> > +a0: ipi_bitmap low 64 bits
> > +a1: ipi_bitmap high 64 bits
> > +a2: the lowest APIC ID in bitmap
> > +a3: APIC ICR
> > +
> > +The hypercall lets a guest send multicast IPIs at most can handle
> > +128 vCPUs per hypercall on 64-bit machines and 64 vCPUs per hypercall
> > +on 32-bit machines.
> > +
> > +Returns 0 if successfully delivery the IPIs and 1 if discarded.
>
> This description does not mention what happens in 32-bit mode.

Will do in next version.

>
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2b812b3..016c7e2 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6691,6 +6691,41 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
> >       kvm_irq_delivery_to_apic(kvm, NULL, &lapic_irq, NULL);
> >  }
> >
> > +/*
> > + * Return 0 if successfully added and 1 if discarded.
> > + */
> > +static int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > +             unsigned long ipi_bitmap_high, int min, int vector, int op_64_bit)
> > +{
> > +     int i;
> > +     struct kvm_apic_map *map;
> > +     struct kvm_vcpu *vcpu;
> > +     struct kvm_lapic_irq irq = {
> > +             .delivery_mode = APIC_DM_FIXED,
> > +             .vector = vector,
> > +     };
> > +
> > +     rcu_read_lock();
> > +     map = rcu_dereference(kvm->arch.apic_map);
> > +
> > +     for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> > +             vcpu = map->phys_map[min + i]->vcpu;
> > +             if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> > +                     return 1;
> > +     }
> > +
> > +     if (op_64_bit) {
> > +             for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> > +                     vcpu = map->phys_map[min + i + BITS_PER_LONG]->vcpu;
> > +                     if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> > +                             return 1;
> > +             }
> > +     }
>
> The second loop processes the second argument, and it should always run,
> even in 32-bit mode.  However, the phys_map index should be min + i + 32
> in 32-bit mode and min + i + 64 in 64-bit mode.  (Using BITS_PER_LONG in
> the for_each_set_bit length is not a bug instead; you could write it
> explicitly as 32 in 32-bit mode, and 64 in 64-bit mode, but I think it's
> a little bit more efficient if it's constant).

Good catch, below should fix it.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c9dbc2c..c118040 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6701,6 +6701,7 @@ static int kvm_pv_send_ipi(struct kvm *kvm,
unsigned long ipi_bitmap_low,
     struct kvm_apic_map *map;
     struct kvm_vcpu *vcpu;
     struct kvm_lapic_irq irq = {0};
+    int cluster_size = op_64_bit ? 64 : 32;

     switch (icr & APIC_VECTOR_MASK) {
     default:
@@ -6714,18 +6715,16 @@ static int kvm_pv_send_ipi(struct kvm *kvm,
unsigned long ipi_bitmap_low,
     rcu_read_lock();
     map = rcu_dereference(kvm->arch.apic_map);

-    for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
+    for_each_set_bit(i, &ipi_bitmap_low, cluster_size) {
         vcpu = map->phys_map[min + i]->vcpu;
         if (!kvm_apic_set_irq(vcpu, &irq, NULL))
             return 1;
     }

-    if (op_64_bit) {
-        for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
-            vcpu = map->phys_map[min + i + BITS_PER_LONG]->vcpu;
-            if (!kvm_apic_set_irq(vcpu, &irq, NULL))
-                return 1;
-        }
+    for_each_set_bit(i, &ipi_bitmap_high, cluster_size) {
+        vcpu = map->phys_map[min + i + cluster_size]->vcpu;
+        if (!kvm_apic_set_irq(vcpu, &irq, NULL))
+            return 1;
     }

     rcu_read_unlock();

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall
  2018-07-23  6:00   ` Wanpeng Li
@ 2018-07-23  6:09     ` Paolo Bonzini
  2018-07-23  6:10     ` Wanpeng Li
  1 sibling, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2018-07-23  6:09 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: LKML, kvm, Radim Krcmar, Wanpeng Li, Vitaly Kuznetsov

On 23/07/2018 08:00, Wanpeng Li wrote:
> On Mon, 23 Jul 2018 at 13:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 20/07/2018 18:28, Wanpeng Li wrote:
>>> +a0: ipi_bitmap low 64 bits
>>> +a1: ipi_bitmap high 64 bits
>>> +a2: the lowest APIC ID in bitmap
>>> +a3: APIC ICR
>>> +
>>> +The hypercall lets a guest send multicast IPIs at most can handle
>>> +128 vCPUs per hypercall on 64-bit machines and 64 vCPUs per hypercall
>>> +on 32-bit machines.
>>> +
>>> +Returns 0 if successfully delivery the IPIs and 1 if discarded.
>>
>> This description does not mention what happens in 32-bit mode.
> 
> Will do in next version.
> 
>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 2b812b3..016c7e2 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -6691,6 +6691,41 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
>>>       kvm_irq_delivery_to_apic(kvm, NULL, &lapic_irq, NULL);
>>>  }
>>>
>>> +/*
>>> + * Return 0 if successfully added and 1 if discarded.
>>> + */
>>> +static int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
>>> +             unsigned long ipi_bitmap_high, int min, int vector, int op_64_bit)
>>> +{
>>> +     int i;
>>> +     struct kvm_apic_map *map;
>>> +     struct kvm_vcpu *vcpu;
>>> +     struct kvm_lapic_irq irq = {
>>> +             .delivery_mode = APIC_DM_FIXED,
>>> +             .vector = vector,
>>> +     };
>>> +
>>> +     rcu_read_lock();
>>> +     map = rcu_dereference(kvm->arch.apic_map);
>>> +
>>> +     for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
>>> +             vcpu = map->phys_map[min + i]->vcpu;
>>> +             if (!kvm_apic_set_irq(vcpu, &irq, NULL))
>>> +                     return 1;
>>> +     }
>>> +
>>> +     if (op_64_bit) {
>>> +             for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
>>> +                     vcpu = map->phys_map[min + i + BITS_PER_LONG]->vcpu;
>>> +                     if (!kvm_apic_set_irq(vcpu, &irq, NULL))
>>> +                             return 1;
>>> +             }
>>> +     }
>>
>> The second loop processes the second argument, and it should always run,
>> even in 32-bit mode.  However, the phys_map index should be min + i + 32
>> in 32-bit mode and min + i + 64 in 64-bit mode.  (Using BITS_PER_LONG in
>> the for_each_set_bit length is not a bug instead; you could write it
>> explicitly as 32 in 32-bit mode, and 64 in 64-bit mode, but I think it's
>> a little bit more efficient if it's constant).
> 
> Good catch, below should fix it.
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c9dbc2c..c118040 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6701,6 +6701,7 @@ static int kvm_pv_send_ipi(struct kvm *kvm,
> unsigned long ipi_bitmap_low,
>      struct kvm_apic_map *map;
>      struct kvm_vcpu *vcpu;
>      struct kvm_lapic_irq irq = {0};
> +    int cluster_size = op_64_bit ? 64 : 32;
> 
>      switch (icr & APIC_VECTOR_MASK) {
>      default:
> @@ -6714,18 +6715,16 @@ static int kvm_pv_send_ipi(struct kvm *kvm,
> unsigned long ipi_bitmap_low,
>      rcu_read_lock();
>      map = rcu_dereference(kvm->arch.apic_map);
> 
> -    for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> +    for_each_set_bit(i, &ipi_bitmap_low, cluster_size) {
>          vcpu = map->phys_map[min + i]->vcpu;
>          if (!kvm_apic_set_irq(vcpu, &irq, NULL))
>              return 1;
>      }
> 
> -    if (op_64_bit) {
> -        for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> -            vcpu = map->phys_map[min + i + BITS_PER_LONG]->vcpu;
> -            if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> -                return 1;
> -        }
> +    for_each_set_bit(i, &ipi_bitmap_high, cluster_size) {
> +        vcpu = map->phys_map[min + i + cluster_size]->vcpu;
> +        if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> +            return 1;
>      }
> 
>      rcu_read_unlock();
> 

Yes, that should work.  Thanks!

Paolo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall
  2018-07-23  6:00   ` Wanpeng Li
  2018-07-23  6:09     ` Paolo Bonzini
@ 2018-07-23  6:10     ` Wanpeng Li
  2018-07-23  6:19       ` Paolo Bonzini
  1 sibling, 1 reply; 7+ messages in thread
From: Wanpeng Li @ 2018-07-23  6:10 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: LKML, kvm, Radim Krcmar, Wanpeng Li, Vitaly Kuznetsov

On Mon, 23 Jul 2018 at 14:00, Wanpeng Li <kernellwp@gmail.com> wrote:
>
> On Mon, 23 Jul 2018 at 13:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 20/07/2018 18:28, Wanpeng Li wrote:
> > > +a0: ipi_bitmap low 64 bits
> > > +a1: ipi_bitmap high 64 bits
> > > +a2: the lowest APIC ID in bitmap
> > > +a3: APIC ICR
> > > +
> > > +The hypercall lets a guest send multicast IPIs at most can handle
> > > +128 vCPUs per hypercall on 64-bit machines and 64 vCPUs per hypercall
> > > +on 32-bit machines.
> > > +
> > > +Returns 0 if successfully delivery the IPIs and 1 if discarded.
> >
> > This description does not mention what happens in 32-bit mode.

Sorry, I think I mentioned "64 vCPUs per hypercall on 32-bit machines" above.

Regards,
Wanpeng Li

>
> Will do in next version.
>
> >
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 2b812b3..016c7e2 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -6691,6 +6691,41 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
> > >       kvm_irq_delivery_to_apic(kvm, NULL, &lapic_irq, NULL);
> > >  }
> > >
> > > +/*
> > > + * Return 0 if successfully added and 1 if discarded.
> > > + */
> > > +static int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > +             unsigned long ipi_bitmap_high, int min, int vector, int op_64_bit)
> > > +{
> > > +     int i;
> > > +     struct kvm_apic_map *map;
> > > +     struct kvm_vcpu *vcpu;
> > > +     struct kvm_lapic_irq irq = {
> > > +             .delivery_mode = APIC_DM_FIXED,
> > > +             .vector = vector,
> > > +     };
> > > +
> > > +     rcu_read_lock();
> > > +     map = rcu_dereference(kvm->arch.apic_map);
> > > +
> > > +     for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> > > +             vcpu = map->phys_map[min + i]->vcpu;
> > > +             if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> > > +                     return 1;
> > > +     }
> > > +
> > > +     if (op_64_bit) {
> > > +             for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> > > +                     vcpu = map->phys_map[min + i + BITS_PER_LONG]->vcpu;
> > > +                     if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> > > +                             return 1;
> > > +             }
> > > +     }
> >
> > The second loop processes the second argument, and it should always run,
> > even in 32-bit mode.  However, the phys_map index should be min + i + 32
> > in 32-bit mode and min + i + 64 in 64-bit mode.  (Using BITS_PER_LONG in
> > the for_each_set_bit length is not a bug instead; you could write it
> > explicitly as 32 in 32-bit mode, and 64 in 64-bit mode, but I think it's
> > a little bit more efficient if it's constant).
>
> Good catch, below should fix it.
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c9dbc2c..c118040 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6701,6 +6701,7 @@ static int kvm_pv_send_ipi(struct kvm *kvm,
> unsigned long ipi_bitmap_low,
>      struct kvm_apic_map *map;
>      struct kvm_vcpu *vcpu;
>      struct kvm_lapic_irq irq = {0};
> +    int cluster_size = op_64_bit ? 64 : 32;
>
>      switch (icr & APIC_VECTOR_MASK) {
>      default:
> @@ -6714,18 +6715,16 @@ static int kvm_pv_send_ipi(struct kvm *kvm,
> unsigned long ipi_bitmap_low,
>      rcu_read_lock();
>      map = rcu_dereference(kvm->arch.apic_map);
>
> -    for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> +    for_each_set_bit(i, &ipi_bitmap_low, cluster_size) {
>          vcpu = map->phys_map[min + i]->vcpu;
>          if (!kvm_apic_set_irq(vcpu, &irq, NULL))
>              return 1;
>      }
>
> -    if (op_64_bit) {
> -        for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> -            vcpu = map->phys_map[min + i + BITS_PER_LONG]->vcpu;
> -            if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> -                return 1;
> -        }
> +    for_each_set_bit(i, &ipi_bitmap_high, cluster_size) {
> +        vcpu = map->phys_map[min + i + cluster_size]->vcpu;
> +        if (!kvm_apic_set_irq(vcpu, &irq, NULL))
> +            return 1;
>      }
>
>      rcu_read_unlock();

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall
  2018-07-23  6:10     ` Wanpeng Li
@ 2018-07-23  6:19       ` Paolo Bonzini
  2018-07-23  6:22         ` Wanpeng Li
  0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2018-07-23  6:19 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: LKML, kvm, Radim Krcmar, Wanpeng Li, Vitaly Kuznetsov

On 23/07/2018 08:10, Wanpeng Li wrote:
>>> On 20/07/2018 18:28, Wanpeng Li wrote:
>>>> +a0: ipi_bitmap low 64 bits
>>>> +a1: ipi_bitmap high 64 bits
>>>> +a2: the lowest APIC ID in bitmap
>>>> +a3: APIC ICR
>>>> +
>>>> +The hypercall lets a guest send multicast IPIs at most can handle
>>>> +128 vCPUs per hypercall on 64-bit machines and 64 vCPUs per hypercall
>>>> +on 32-bit machines.
>>>> +
>>>> +Returns 0 if successfully delivery the IPIs and 1 if discarded.
>>> This description does not mention what happens in 32-bit mode.
> Sorry, I think I mentioned "64 vCPUs per hypercall on 32-bit machines" above.

Yes, but the description of a0 and a1 is not accurate.  Something like

a0: lower part of the bitmap of destination APIC IDs
a1: higher part of the bitmap of destination APIC IDs
....

The hypercall lets a guest send multicast IPIs, with at most 128
128 destinations per hypercall in 64-bit mode and 64 vCPUs per
hypercall in 32-bit mode.  The destinations are represented by a bitmap
contained in the first two arguments (a0 and a1).  Bit 0 of a0
corresponds to the APIC ID in the third argument (a2), bit 1 corresponds
to the APIC ID a2+1, and so on.

Thanks,

Paolo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall
  2018-07-23  6:19       ` Paolo Bonzini
@ 2018-07-23  6:22         ` Wanpeng Li
  0 siblings, 0 replies; 7+ messages in thread
From: Wanpeng Li @ 2018-07-23  6:22 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: LKML, kvm, Radim Krcmar, Wanpeng Li, Vitaly Kuznetsov

On Mon, 23 Jul 2018 at 14:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 23/07/2018 08:10, Wanpeng Li wrote:
> >>> On 20/07/2018 18:28, Wanpeng Li wrote:
> >>>> +a0: ipi_bitmap low 64 bits
> >>>> +a1: ipi_bitmap high 64 bits
> >>>> +a2: the lowest APIC ID in bitmap
> >>>> +a3: APIC ICR
> >>>> +
> >>>> +The hypercall lets a guest send multicast IPIs at most can handle
> >>>> +128 vCPUs per hypercall on 64-bit machines and 64 vCPUs per hypercall
> >>>> +on 32-bit machines.
> >>>> +
> >>>> +Returns 0 if successfully delivery the IPIs and 1 if discarded.
> >>> This description does not mention what happens in 32-bit mode.
> > Sorry, I think I mentioned "64 vCPUs per hypercall on 32-bit machines" above.
>
> Yes, but the description of a0 and a1 is not accurate.  Something like
>
> a0: lower part of the bitmap of destination APIC IDs
> a1: higher part of the bitmap of destination APIC IDs
> ....
>
> The hypercall lets a guest send multicast IPIs, with at most 128
> 128 destinations per hypercall in 64-bit mode and 64 vCPUs per
> hypercall in 32-bit mode.  The destinations are represented by a bitmap
> contained in the first two arguments (a0 and a1).  Bit 0 of a0
> corresponds to the APIC ID in the third argument (a2), bit 1 corresponds
> to the APIC ID a2+1, and so on.

Thanks to the great description, I will fold this to the next version.

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-07-23  6:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-20 16:28 [PATCH v4 4/6] KVM: X86: Implement PV IPIs send hypercall Wanpeng Li
2018-07-23  5:52 ` Paolo Bonzini
2018-07-23  6:00   ` Wanpeng Li
2018-07-23  6:09     ` Paolo Bonzini
2018-07-23  6:10     ` Wanpeng Li
2018-07-23  6:19       ` Paolo Bonzini
2018-07-23  6:22         ` Wanpeng Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).