> -----Original Message----- > From: Alexander Graf [mailto:agraf@xxxxxxx] > Sent: Monday, October 07, 2013 9:43 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 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 Yes, I was getting confused why only KVM_GUEST as this not specific to KVM-GUEST. Thank you > > 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
