On 07.10.2013, at 17:43, Bhushan Bharat-R65777 <R65777@xxxxxxxxxxxxx> wrote: >>>>>>>>> at least when I can avoid it. With the current code the compiler >>>>>>>>> would be >>>>>> smart enough to just optimize out the complete branch. >>>>>>>> >>>>>>>> Sure. My point is, where would you be calling that where the >>>>>>>> entire file isn't predicated on (or selecting) CONFIG_KVM_GUEST or >> similar? >>>>>>>> >>>>>>>> We don't do these stubs for every single function in the kernel >>>>>>>> -- only ones where the above is a reasonable use case. >>>>>>> >>>>>>> Yeah, I'm fine on dropping it, but we need to make that a >>>>>>> conscious decision >>>>>> and verify that no caller relies on it. >>>>>> >>>>>> kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c, >>>>>> arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which >>>>>> are enabled by CONFIG_KVM_GUEST. >>>>>> >>>>>> I did find one example of kvm_para_available() being used in an >>>>>> unexpected place >>>>>> -- sound/pci/intel8x0.c. It defines its own non-CONFIG_KVM_GUEST >>>>>> stub, even though x86 defines kvm_para_available() using inline >>>>>> CPUID stuff which should work without CONFIG_KVM_GUEST. >>>>>> I'm not sure why it even needs to do that, though -- shouldn't the >>>>>> subsequent PCI subsystem vendor/device check should be sufficient? >>>>>> No hypercalls are involved. >>>>>> >>>>>> That said, the possibility that some random driver might want to >>>>>> make use of paravirt features is a decent argument for keeping the stub. >>>>>> >>>>> >>>>> I am not sure where we are agreeing on? >>>>> Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h >>>>> ? as >>>> there is no caller without KVM_GUEST and in future caller ensure this >>>> to be called only from code selected by KVM_GUEST? >>>>> >>>>> Or let this stub stay to avoid any random driver calling this ? >>>> >>>> I think the most reasonable way forward is to add a stub for >>>> non-CONFIG_EPAPR to the epapr code, then replace the kvm bits with >>>> generic epapr bits (which your patches already do). >>> >>> Please describe which stub you are talking about. >> >> kvm_hypercall is always available, regardless of the config option, which makes >> all its subfunctions always available as well. > > This patch renames kvm_hypercall() to epapr_hypercall() and which is always available. And the kvm_hypercall() friends now directly calls epapr_hypercall(). > IIUC, So what you are trying to say is let the kvm_hypercall() friends keep on calling kvm_hypercall() itself and a sub something like this: No, what I'm saying is that we either a) drop the whole #ifndef code path consciously. This would have to be a separate patch with a separate discussion. It's orthogonal to combining kvm_hypercall() and epapr_hypercall() b) add the #ifndef path to epapr_hypercall() I prefer b, Scott prefers b. Alex -- 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
