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

[Alexander Graf <agraf@suse.de>]

On 07.10.2013, at 17:15, Bhushan Bharat-R65777 <R65777@freescale.com> wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Friday, October 04, 2013 4:46 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 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@vger.kernel.org;
>>>> kvm@vger.kernel.org; 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