> -----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 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 > > I prefer b, Scott prefers b. > > > Alex > > -- > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body > of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > http://vger.kernel.org/majordomo-info.html -- 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
