> -----Original Message----- > From: rkrcmar@xxxxxxxxxx [mailto:rkrcmar@xxxxxxxxxx] > Sent: Wednesday, December 23, 2015 3:53 AM > To: Wu, Feng <feng.wu@xxxxxxxxx> > Cc: Yang Zhang <yang.zhang.wz@xxxxxxxxx>; pbonzini@xxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jiang Liu > (jiang.liu@xxxxxxxxxxxxxxx) <jiang.liu@xxxxxxxxxxxxxxx> > Subject: Re: [PATCH v2 1/2] KVM: x86: Use vector-hashing to deliver lowest- > priority interrupts > > 2015-12-22 07:19+0000, Wu, Feng: > >> From: Yang Zhang [mailto:yang.zhang.wz@xxxxxxxxx] > >> On 2015/12/22 14:59, Wu, Feng wrote: > >> >> From: Yang Zhang [mailto:yang.zhang.wz@xxxxxxxxx] > >> >>>>>> 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. > Radim, thanks a lot for your feedback! > 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. Now I am a little confused about the policy, when and where should we do the software/hardware enabled check? > > (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 :) >From the change log, It seems to me this patch was fixing a bug. Thanks, Feng -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
