Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1292694 > unrolled thread
| Started by | Feng Wu <feng.wu@intel.com> |
|---|---|
| First post | 2015-12-16 03:00 +0100 |
| Last post | 2015-12-23 18:20 +0100 |
| Articles | 3 on this page of 23 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH v2 0/2] Add vector-hashing support for lowest-priority interrupts delivery Feng Wu <feng.wu@intel.com> - 2015-12-16 03:00 +0100
[PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts Feng Wu <feng.wu@intel.com> - 2015-12-16 03:00 +0100
RE: [PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts "Wu, Feng" <feng.wu@intel.com> - 2015-12-21 03:00 +0100
Re: [PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2015-12-21 03:10 +0100
RE: [PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts "Wu, Feng" <feng.wu@intel.com> - 2015-12-22 05:40 +0100
Re: [PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2015-12-22 07:50 +0100
Re: [PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts "rkrcmar@redhat.com" <rkrcmar@redhat.com> - 2015-12-23 18:00 +0100
Re: [PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2015-12-21 03:00 +0100
Re: [PATCH v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts Radim Krčmář <rkrcmar@redhat.com> - 2015-12-23 18:30 +0100
[PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Feng Wu <feng.wu@intel.com> - 2015-12-16 03:00 +0100
Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2015-12-21 02:50 +0100
RE: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2015-12-21 03:00 +0100
Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2015-12-21 03:10 +0100
RE: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2015-12-22 05:40 +0100
Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2015-12-22 07:50 +0100
RE: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2015-12-22 08:00 +0100
RE: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2015-12-22 08:20 +0100
Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts "rkrcmar@redhat.com" <rkrcmar@redhat.com> - 2015-12-22 21:00 +0100
RE: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2015-12-23 03:20 +0100
Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts "rkrcmar@redhat.com" <rkrcmar@redhat.com> - 2015-12-23 17:50 +0100
Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2015-12-23 04:20 +0100
Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2015-12-22 08:20 +0100
Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Radim Krčmář <rkrcmar@redhat.com> - 2015-12-23 18:20 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Yang Zhang <yang.zhang.wz@gmail.com> |
|---|---|
| Date | 2015-12-23 04:20 +0100 |
| Subject | Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts |
| Message-ID | <qII82-4ir-3@gated-at.bofh.it> |
| In reply to | #1297037 |
On 2015/12/23 3:52, rkrcmar@redhat.com wrote:
> 2015-12-22 07:19+0000, Wu, Feng:
>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>> On 2015/12/22 14:59, Wu, Feng wrote:
>>>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>>>>>>> On 2015/12/16 9:37, Feng Wu wrote:
>>>>>>>>>> + for_each_set_bit(i, &bitmap, 16) {
>>>>>>>>>> + if (!dst[i]
>>>>>>>>> && !kvm_lapic_enabled(dst[i]->vcpu)) {
>>>>>>>>>
>>>>>>>>> It should be or(||) not and (&&).
>>>>>>>>
>>>>>>>> Oh, you are right! My negligence! Thanks for pointing this out, Yang!
>>>>>>>
>>>>>>> btw, i think the kvm_lapic_enabled check is wrong here? Why need it here?
>>>>>>
>>>>>> If the lapic is not enabled, I think we cannot recognize it as a candidate, can
>>>>> we?
>>>>>> Maybe Radim can confirm this, Radim, what is your option?
>
> SDM 10.6.2.2 Logical Destination Mode:
> For both configurations of logical destination mode, when combined
> with lowest priority delivery mode, software is responsible for
> ensuring that all of the local APICs included in or addressed by the
> IPI or I/O subsystem interrupt are present and enabled to receive the
> interrupt.
>
> The case is undefined if some targeted LAPICs weren't hardware enabled
> as no interrupts can be delivered to hardware disabled LAPIC, so we can
> check for hardware enabled.
>
> It's not obvious if "enabled to receive the interrupt" means hardware or
> software enabled, but lowest priority cannot deliver NMI/INIT/..., so
> checking for software enabled doesn't restrict any valid uses either.
Agree. My understanding is that it is software's responsibility to
ensuring this case not happen. But for hypervisor, we should not check
it for software. What we can do is just follow the SDM.
>
> so ... KVM only musn't blow up when encountering this situation :)
>
> The current code seems correct, but redundant. Just for reference, KVM
> now does:
> - check for software enabled LAPIC since patch aefd18f01ee8 ("KVM: x86:
> In DM_LOWEST, only deliver interrupts to vcpus with enabled LAPIC's")
> - check only for hardware enabled LAPIC in the fast path, since
> 1e08ec4a130e ("KVM: optimize apic interrupt delivery"))
>
> (v1 was arguable better, I pointed the need for enabled LAPIC in v1 only
> from looking at one KVM function, sorry.)
>
>>>>> Lapic can be disable by hw or sw. Here we only need to check the hw is
>>>>> enough which is already covered while injecting the interrupt into
>>>>> guest. I remember we(Glab, Macelo and me) have discussed it several ago,
>>>>> but i cannot find the mail thread.
>>>
>>>>
>>>> But if the lapic is disabled by software, we cannot still inject interrupts to
>>>> it, can we?
>>>
>>> Yes, We cannot inject the normal interrupt. But this already covered by
>>> current logic and add a check here seems meaningless. Conversely, it may
>>> do bad thing..
>>>
>>
>> Let's wait for Radim/Paolo's opinions about this.
>
> I'd pick whatever results in less code: this time it seems like checking
> for hardware enabled LAPIC in both paths (implicitly in the fast path).
> Maybe it can be done better, I haven't given it much thought.
>
> We should revert aefd18f01ee8 at the same time, so our PI/non-PI slow
> paths won't diverge -- I hope it wasn't fixing a bug :)
>
> I'll review the series tomorrow, thanks for your patience.
--
best regards
yang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Yang Zhang <yang.zhang.wz@gmail.com> |
|---|---|
| Date | 2015-12-22 08:20 +0100 |
| Subject | Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts |
| Message-ID | <qIpoJ-Py-11@gated-at.bofh.it> |
| In reply to | #1296576 |
On 2015/12/22 14:59, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>> Sent: Tuesday, December 22, 2015 2:49 PM
>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>> rkrcmar@redhat.com
>> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Jiang Liu
>> (jiang.liu@linux.intel.com) <jiang.liu@linux.intel.com>
>> Subject: Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-
>> priority interrupts
>>
>>>>>>
>>>>>> On 2015/12/16 9:37, 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.wu@intel.com>
>>>>>>> ---
>>>>>>> arch/x86/kvm/irq_comm.c | 27 ++++++++++++++++++-----
>>>>>>> arch/x86/kvm/lapic.c | 57
>>>>>> ++++++++++++++++++++++++++++++++++++++++---------
>>>>>>> arch/x86/kvm/lapic.h | 2 ++
>>>>>>> arch/x86/kvm/x86.c | 9 ++++++++
>>>>>>> arch/x86/kvm/x86.h | 1 +
>>>>>>> 5 files changed, 81 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> 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)
>>>>>>> {
>>>>>>> @@ -731,17 +747,38 @@ 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)) {
>>>>>>> - int l = -1;
>>>>>>> - for_each_set_bit(i, &bitmap, 16) {
>>>>>>> - if (!dst[i])
>>>>>>> - continue;
>>>>>>> - if (l < 0)
>>>>>>> - l = i;
>>>>>>> - else if (kvm_apic_compare_prio(dst[i]->vcpu,
>>>>>> dst[l]->vcpu) < 0)
>>>>>>> - l = i;
>>>>>>> + if (!kvm_vector_hashing_enabled()) {
>>>>>>> + int l = -1;
>>>>>>> + for_each_set_bit(i, &bitmap, 16) {
>>>>>>> + if (!dst[i])
>>>>>>> + continue;
>>>>>>> + if (l < 0)
>>>>>>> + l = i;
>>>>>>> + 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;
>>>>>>> +
>>>>>>> + for_each_set_bit(i, &bitmap, 16) {
>>>>>>> + if (!dst[i]
>>>>>> && !kvm_lapic_enabled(dst[i]->vcpu)) {
>>>>>>
>>>>>> It should be or(||) not and (&&).
>>>>>
>>>>> Oh, you are right! My negligence! Thanks for pointing this out, Yang!
>>>>
>>>> btw, i think the kvm_lapic_enabled check is wrong here? Why need it here?
>>>
>>> If the lapic is not enabled, I think we cannot recognize it as a candidate, can
>> we?
>>> Maybe Radim can confirm this, Radim, what is your option?
>>
>> Lapic can be disable by hw or sw. Here we only need to check the hw is
>> enough which is already covered while injecting the interrupt into
>> guest. I remember we(Glab, Macelo and me) have discussed it several ago,
>> but i cannot find the mail thread.
>
> But if the lapic is disabled by software, we cannot still inject interrupts to
> it, can we?
Yes, We cannot inject the normal interrupt. But this already covered by
current logic and add a check here seems meaningless. Conversely, it may
do bad thing..
--
best regards
yang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Radim Krčmář <rkrcmar@redhat.com> |
|---|---|
| Date | 2015-12-23 18:20 +0100 |
| Subject | Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts |
| Message-ID | <qIVeV-42B-9@gated-at.bofh.it> |
| In reply to | #1292697 |
2015-12-16 09:37+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.wu@intel.com>
> ---
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -78,13 +83,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(vcpu->vcpu_id, 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);
This is going to fail with sparse topologies (e.g. 3 cores per socket).
vcpu_id = initial APIC ID and kvm_get_vcpu() uses a compressed array
that has kvm->online_vcpus elements, so we could overflow.
The 'i' in kvm_for_each_vcpu() could be used for the bitmap.
(kvm_get_vcpu_by_id() instead of kvm_get_vcpu() is slightly worse.)
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> @@ -678,6 +678,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> 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)
> {
> @@ -731,17 +747,38 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> + if (!kvm_vector_hashing_enabled()) {
| [...]
> + } else {
> + int idx = 0;
> + unsigned int dest_vcpus = 0;
Now that we don't need to check for present/enabled LAPICs, I think it
would be better to solve this by assuming that all selected LAPICs are
enabled, so the n-th target is decided only based on vector and
destination.
> + for_each_set_bit(i, &bitmap, 16) {
> + if (!dst[i] && !kvm_lapic_enabled(dst[i]->vcpu)) {
> + __clear_bit(i, &bitmap);
> + continue;
> + }
> + }
=> we could skip this loop.
> +
> + dest_vcpus = hweight16(bitmap);
> +
> + if (dest_vcpus != 0) {
> + idx = kvm_vector_2_index(irq->vector,
> + dest_vcpus, &bitmap, 16);
> +
> + bitmap = 0;
> + __set_bit(idx-1, &bitmap);
And set just this bit.
The drawback is that buggy software that included hardware disabled
APICs to lowest priority destinations could stop working ...
Do you think it's too risky?
> + }
> }
(This is basically the same as converting the message to a fixed delivery
to n-th bit beforehand, so it might be reasonable to to apply something
similar to simplify the slow path as well. Mixed flat/cluster/x2APIC
mode makes me suspect that it won't be reasonable.)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web