On 07.10.2013, at 18:04, Bhushan Bharat-R65777 <R65777@xxxxxxxxxxxxx> wrote: > > >> -----Original Message----- >> From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On >> Behalf Of Alexander Graf >> Sent: Monday, October 07, 2013 9:16 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 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() > > Do you mean like this in arch/powerpc/include/asm/epapr_hcalls.h > > #ifdef CONFIG_KVM_GUEST CONFIG_EPAPR_PARAVIRT Apart from that, yes, I think that's what we want. Alex > static inline unsigned long epapr_hypercall(unsigned long *in, > unsigned long *out, > unsigned long nr) > { > // code for this function > } > #else > static inline unsigned long epapr_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
