On 07.10.2013, at 17:15, Bhushan Bharat-R65777 <R65777@xxxxxxxxxxxxx> wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@xxxxxxx] >> Sent: Friday, October 04, 2013 4:46 PM >> To: Bhushan Bharat-R65777 >> Cc: Wood Scott-B07421; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to >> epapr_hypercall() >> >> >> On 04.10.2013, at 06:26, Bhushan Bharat-R65777 wrote: >> >>> >>> >>>> -----Original Message----- >>>> From: Wood Scott-B07421 >>>> Sent: Thursday, October 03, 2013 12:04 AM >>>> To: Alexander Graf >>>> Cc: Bhushan Bharat-R65777; kvm-ppc@xxxxxxxxxxxxxxx; >>>> kvm@xxxxxxxxxxxxxxx; Bhushan >>>> Bharat-R65777 >>>> Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to >>>> epapr_hypercall() >>>> >>>> On Wed, 2013-10-02 at 19:54 +0200, Alexander Graf wrote: >>>>> On 02.10.2013, at 19:49, Scott Wood wrote: >>>>> >>>>>> On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote: >>>>>>> On 02.10.2013, at 19:42, Scott Wood wrote: >>>>>>> >>>>>>>> On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote: >>>>>>>>> On 02.10.2013, at 19:04, Scott Wood wrote: >>>>>>>>> >>>>>>>>>> On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote: >>>>>>>>>>> On 02.10.2013, at 18:40, Scott Wood wrote: >>>>>>>>>>> >>>>>>>>>>>> On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote: >>>>>>>>>>>>> Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't >>>>>>>>>>>>> have >>>> epapr_hcalls.S compiled into the code base then and the bl above >>>> would reference an unknown function. >>>>>>>>>>>> >>>>>>>>>>>> KVM_GUEST selects EPAPR_PARAVIRT. >>>>>>>>>>> >>>>>>>>>>> But you can not select KVM_GUEST and still call these inline >>>>>>>>>>> functions, >>>> no? >>>>>>>>>> >>>>>>>>>> No. >>>>>>>>>> >>>>>>>>>>> Like kvm_arch_para_features(). >>>>>>>>>> >>>>>>>>>> Where does that get called without KVM_GUEST? >>>>>>>>>> >>>>>>>>>> How would that work currently, with the call to kvm_hypercall() >>>>>>>>>> in arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)? >>>>>>>>> >>>>>>>>> It wouldn't ever get called because kvm_hypercall() ends up >>>>>>>>> always >>>> returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST. >>>>>>>> >>>>>>>> OK, so the objection is to removing that stub? Where would we >>>>>>>> actually want to call this without knowing that KVM_GUEST or >>>>>>>> EPAPR_PARAVIRT are enabled? >>>>>>> >>>>>>> In probing code. I usually prefer >>>>>>> >>>>>>> if (kvm_feature_available(X)) { >>>>>>> ... >>>>>>> } >>>>>>> >>>>>>> over >>>>>>> >>>>>>> #ifdef CONFIG_KVM_GUEST >>>>>>> if (kvm_feature_available(X)) { >>>>>>> ... >>>>>>> } >>>>>>> #endif >>>>>>> >>>>>>> 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. Alex --- #ifdef CONFIG_KVM_GUEST #include <linux/of.h> static inline int kvm_para_available(void) { struct device_node *hyper_node; hyper_node = of_find_node_by_path("/hypervisor"); if (!hyper_node) return 0; if (!of_device_is_compatible(hyper_node, "linux,kvm")) return 0; return 1; } extern unsigned long kvm_hypercall(unsigned long *in, unsigned long *out, unsigned long nr); #else static inline int kvm_para_available(void) { return 0; } static unsigned long kvm_hypercall(unsigned long *in, unsigned long *out, unsigned long nr) { return EV_UNIMPLEMENTED; } #endif -- 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
