Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts
2015-12-23 02:12+, Wu, Feng:
>> From: rkrc...@redhat.com [mailto:rkrc...@redhat.com]
>> 2015-12-22 07:19+, Wu, Feng:
>> >> From: Yang Zhang [mailto:yang.zhang...@gmail.com]
>> >> On 2015/12/22 14:59, Wu, Feng wrote:
>> >> >> From: Yang Zhang [mailto:yang.zhang...@gmail.com]
>> >> >>>>>> On 2015/12/16 9:37, Feng Wu wrote:
>> 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.
>>
>> 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"))
>
> Software enabled LAPIC is also checked in patch 1e08ec4a130e
> ("KVM: optimize apic interrupt delivery"), however, it was removed
> in patch 3b5a5ffa928a3f875b0d5dd284eeb7c322e1688a.
Right, thanks. (The software check was actually removed in 173beedc1601
("KVM: x86: Software disabled APIC should still deliver NMIs"), which
introduced a two pass mechanism that was later simplified.)
>Now I am
> a little confused about the policy, when and where should we do
> the software/hardware enabled check?
It's a mess, I think we'd like both checks to be done early and ideally
only in one place.
The fast path would like to precompute as much as possible, but only
hardware enabled affects all interrupts (like non-present LAPIC);
software disabled still needs an extra condition for every interrupt.
>> 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 :)
>
> From the change log, It seems to me this patch was fixing a bug.
Yeah, I found the original discussion
RFC: http://www.spinics.net/lists/kvm/msg36190.html
v1: http://www.spinics.net/lists/kvm/msg36395.html
v2: http://www.spinics.net/lists/kvm/msg36651.html
that led to some explanation in bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=596223 (a clone of
https://bugzilla.redhat.com/show_bug.cgi?id=505527)
It seems that kexec on VCPU != 0 did something with BSP APIC ID that
resulted in a wrong delivery -- I didn't look where the bug was, but the
solution we adopted is probably just a lucky workaround.
Makes sense to look deeper into it.
--
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 v2 2/2] KVM: x86: Add lowest-priority support for vt-d posted-interrupts
2015-12-22 14:42+0800, Yang Zhang:
> On 2015/12/22 12:36, Wu, Feng wrote:
>>>From: Yang Zhang [mailto:yang.zhang...@gmail.com]
>>>On 2015/12/21 9:55, Wu, Feng wrote:
>From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
>On 2015/12/16 9:37, Feng Wu wrote:
>>diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>@@ -10702,8 +10702,16 @@ static int vmx_update_pi_irte(struct kvm
>>>*kvm,
>unsigned int host_irq,
>> */
>>
>> kvm_set_msi_irq(e, );
>>- if (!kvm_intr_is_single_vcpu(kvm, , ))
>>- continue;
>>+
>>+ if (!kvm_intr_is_single_vcpu(kvm, , )) {
>>+ if (!kvm_vector_hashing_enabled() ||
>>+ irq.delivery_mode !=
>APIC_DM_LOWEST)
>>+ continue;
>>+
>>+ vcpu = kvm_intr_vector_hashing_dest(kvm, );
>>+ if (!vcpu)
>>+ continue;
>>+ }
>
>I am a little confused with the 'continue'. If the destination is not
>single vcpu, shouldn't we rollback to use non-PI mode?
Here is the logic:
- If it is single destination, we will use PI no matter it is fixed or
lowest-priority.
- If it is not single destination:
a) It is fixed, we will use non-PI
b) It is lowest-priority and vector-hashing is enabled, we will use PI
c) otherwise, use non-PI
>>>
>>>If it is single destination previously, then change to no-single mode.
>>>Can current code cover this case?
>>
>>In my test, before setting irq affinity (change single vcpu to non-single vcpu
>>in this case), the guest will mask the interrupt first, so before getting
>>here, IRTE
>>has been changed back to remapped mode already(when guest masks the MSIx,
>>we will change back to remapped mode), hence nothing needed here.
>>
>>Digging into the linux code (guest) a bit more, I found that if interrupt
>>remapping
>>is not enabled in the guest (IR is not supported for guest anyway), it will
>>always
>>mask the MSI/MSIx before setting the irq affinity. So the code should work
>>well currently.
>
> We should not rely on guest's behavior. From code level, it need be fixed.
>
>>However, for robustness, I think explicitly changing IRTE back to remapped
>>mode for the 'continue' case should be a good idea.
>
> This is what i am looking for.
I agree, that would be a nice addition.
IIRC, the masking is optional -- if the guest can handle interrupts that
are generated while the device is half-configured, it doesn't need to
disable MSIs.
--
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 v2 1/2] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts
2015-12-22 07:19+, Wu, Feng:
>> From: Yang Zhang [mailto:yang.zhang...@gmail.com]
>> On 2015/12/22 14:59, Wu, Feng wrote:
>> >> From: Yang Zhang [mailto:yang.zhang...@gmail.com]
>> >> On 2015/12/16 9:37, Feng Wu wrote:
>> >>> +for_each_set_bit(i, , 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.
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.
--
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
