RE: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()

[Bhushan Bharat-R65777 <R65777@freescale.com>]

> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On
> Behalf Of Alexander Graf
> Sent: Monday, October 07, 2013 9:16 PM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to
> epapr_hypercall()
> 
> 
> On 07.10.2013, at 17:43, Bhushan Bharat-R65777 <R65777@freescale.com> 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@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html