diff mbox

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

Message ID 1379913839-11347-2-git-send-email-Bharat.Bhushan@freescale.com
State New, archived
Headers show

Commit Message

Bharat Bhushan Sept. 23, 2013, 5:23 a.m. UTC
kvm_hypercall() have nothing KVM specific, so renamed to epapr_hypercall().
Also this in moved to arch/powerpc/include/asm/epapr_hcalls.h

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/epapr_hcalls.h |   36 +++++++++++++++++++++++++++
 arch/powerpc/include/asm/kvm_para.h     |   23 ++++-------------
 arch/powerpc/kernel/kvm.c               |   41 +-----------------------------
 3 files changed, 44 insertions(+), 56 deletions(-)

Comments

Scott Wood Sept. 23, 2013, 10:45 p.m. UTC | #1
On Mon, 2013-09-23 at 10:53 +0530, Bharat Bhushan wrote:
> kvm_hypercall() have nothing KVM specific, so renamed to epapr_hypercall().
> Also this in moved to arch/powerpc/include/asm/epapr_hcalls.h
[snip]
> +	out[0] = r4;
> +	out[1] = r5;
> +	out[2] = r6;
> +	out[3] = r7;
> +	out[4] = r8;
> +	out[5] = r9;
> +	out[6] = r10;
> +	out[7] = r11;
> +
> +	return r3;
> +}
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _EPAPR_HCALLS_H */
[snip]
> -	out[0] = r4;
> -	out[1] = r5;
> -	out[2] = r6;
> -	out[3] = r7;
> -	out[4] = r8;
> -	out[5] = r9;
> -	out[6] = r10;
> -	out[7] = r11;
> -
> -	return r3;
> -}
> -EXPORT_SYMBOL_GPL(kvm_hypercall);

Where did the export go?

I wish git could detect copies/moves of chunks of code between existing
files rather than just when a new file is created...

-Scott



--
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
Scott Wood Sept. 23, 2013, 10:57 p.m. UTC | #2
On Mon, 2013-09-23 at 17:45 -0500, Scott Wood wrote:
> On Mon, 2013-09-23 at 10:53 +0530, Bharat Bhushan wrote:
> > kvm_hypercall() have nothing KVM specific, so renamed to epapr_hypercall().
> > Also this in moved to arch/powerpc/include/asm/epapr_hcalls.h
> [snip]
> > +	out[0] = r4;
> > +	out[1] = r5;
> > +	out[2] = r6;
> > +	out[3] = r7;
> > +	out[4] = r8;
> > +	out[5] = r9;
> > +	out[6] = r10;
> > +	out[7] = r11;
> > +
> > +	return r3;
> > +}
> >  #endif /* !__ASSEMBLY__ */
> >  #endif /* _EPAPR_HCALLS_H */
> [snip]
> > -	out[0] = r4;
> > -	out[1] = r5;
> > -	out[2] = r6;
> > -	out[3] = r7;
> > -	out[4] = r8;
> > -	out[5] = r9;
> > -	out[6] = r10;
> > -	out[7] = r11;
> > -
> > -	return r3;
> > -}
> > -EXPORT_SYMBOL_GPL(kvm_hypercall);
> 
> Where did the export go?

Never mind -- it's been converted to inline.

-Scott



--
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
Alexander Graf Oct. 2, 2013, 2:19 p.m. UTC | #3
On 23.09.2013, at 07:23, Bharat Bhushan wrote:

> kvm_hypercall() have nothing KVM specific, so renamed to epapr_hypercall().
> Also this in moved to arch/powerpc/include/asm/epapr_hcalls.h
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> arch/powerpc/include/asm/epapr_hcalls.h |   36 +++++++++++++++++++++++++++
> arch/powerpc/include/asm/kvm_para.h     |   23 ++++-------------
> arch/powerpc/kernel/kvm.c               |   41 +-----------------------------
> 3 files changed, 44 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/epapr_hcalls.h b/arch/powerpc/include/asm/epapr_hcalls.h
> index d3d6342..8a85f6f 100644
> --- a/arch/powerpc/include/asm/epapr_hcalls.h
> +++ b/arch/powerpc/include/asm/epapr_hcalls.h
> @@ -454,5 +454,41 @@ static inline unsigned int ev_idle(void)
> 
> 	return r3;
> }
> +
> +static inline unsigned long epapr_hypercall(unsigned long *in,
> +			    unsigned long *out,
> +			    unsigned long nr)
> +{
> +	unsigned long register r0 asm("r0");
> +	unsigned long register r3 asm("r3") = in[0];
> +	unsigned long register r4 asm("r4") = in[1];
> +	unsigned long register r5 asm("r5") = in[2];
> +	unsigned long register r6 asm("r6") = in[3];
> +	unsigned long register r7 asm("r7") = in[4];
> +	unsigned long register r8 asm("r8") = in[5];
> +	unsigned long register r9 asm("r9") = in[6];
> +	unsigned long register r10 asm("r10") = in[7];
> +	unsigned long register r11 asm("r11") = nr;
> +	unsigned long register r12 asm("r12");
> +
> +	asm volatile("bl	epapr_hypercall_start"
> +		     : "=r"(r0), "=r"(r3), "=r"(r4), "=r"(r5), "=r"(r6),
> +		       "=r"(r7), "=r"(r8), "=r"(r9), "=r"(r10), "=r"(r11),
> +		       "=r"(r12)
> +		     : "r"(r3), "r"(r4), "r"(r5), "r"(r6), "r"(r7), "r"(r8),
> +		       "r"(r9), "r"(r10), "r"(r11)
> +		     : "memory", "cc", "xer", "ctr", "lr");
> +
> +	out[0] = r4;
> +	out[1] = r5;
> +	out[2] = r6;
> +	out[3] = r7;
> +	out[4] = r8;
> +	out[5] = r9;
> +	out[6] = r10;
> +	out[7] = r11;
> +
> +	return r3;
> +}
> #endif /* !__ASSEMBLY__ */
> #endif /* _EPAPR_HCALLS_H */
> diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
> index 2b11965..c18660e 100644
> --- a/arch/powerpc/include/asm/kvm_para.h
> +++ b/arch/powerpc/include/asm/kvm_para.h
> @@ -39,10 +39,6 @@ static inline int kvm_para_available(void)
> 	return 1;
> }
> 
> -extern unsigned long kvm_hypercall(unsigned long *in,
> -				   unsigned long *out,
> -				   unsigned long nr);
> -
> #else
> 
> static inline int kvm_para_available(void)
> @@ -50,13 +46,6 @@ 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
> 
> static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
> @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
> 	unsigned long out[8];
> 	unsigned long r;
> 
> -	r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
> +	r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));

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.


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
Scott Wood Oct. 2, 2013, 4:40 p.m. UTC | #4
On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
> On 23.09.2013, at 07:23, Bharat Bhushan wrote:
> > static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
> > @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
> > 	unsigned long out[8];
> > 	unsigned long r;
> > 
> > -	r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
> > +	r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
> 
> 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.

-Scott



--
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
Alexander Graf Oct. 2, 2013, 4:53 p.m. UTC | #5
On 02.10.2013, at 18:40, Scott Wood wrote:

> On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
>> On 23.09.2013, at 07:23, Bharat Bhushan wrote:
>>> static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
>>> @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
>>> 	unsigned long out[8];
>>> 	unsigned long r;
>>> 
>>> -	r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
>>> +	r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
>> 
>> 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? Like kvm_arch_para_features().


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
Scott Wood Oct. 2, 2013, 5:04 p.m. UTC | #6
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:
> >> On 23.09.2013, at 07:23, Bharat Bhushan wrote:
> >>> static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
> >>> @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
> >>> 	unsigned long out[8];
> >>> 	unsigned long r;
> >>> 
> >>> -	r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
> >>> +	r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
> >> 
> >> 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)?

-Scott



--
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
Alexander Graf Oct. 2, 2013, 5:17 p.m. UTC | #7
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:
>>>> On 23.09.2013, at 07:23, Bharat Bhushan wrote:
>>>>> static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
>>>>> @@ -65,7 +54,7 @@ static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
>>>>> 	unsigned long out[8];
>>>>> 	unsigned long r;
>>>>> 
>>>>> -	r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
>>>>> +	r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
>>>> 
>>>> 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.


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
Scott Wood Oct. 2, 2013, 5:42 p.m. UTC | #8
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?

-SCott



--
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
Alexander Graf Oct. 2, 2013, 5:46 p.m. UTC | #9
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.


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
Scott Wood Oct. 2, 2013, 5:49 p.m. UTC | #10
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.

-Scott



--
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
Alexander Graf Oct. 2, 2013, 5:54 p.m. UTC | #11
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.


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
Scott Wood Oct. 2, 2013, 6:33 p.m. UTC | #12
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.

-Scott



--
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
Bharat Bhushan Oct. 4, 2013, 4:26 a.m. UTC | #13
> -----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 ?

Thanks
-Bharat
Alexander Graf Oct. 4, 2013, 11:16 a.m. UTC | #14
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).

With that we should be 100% equivalent to today's code, just with a lot less lines of code :).


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
Bharat Bhushan Oct. 7, 2013, 3:15 p.m. UTC | #15
> -----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.

Thanks
-Bharat

> 
> With that we should be 100% equivalent to today's code, just with a lot less
> lines of code :).
> 
> 
> 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
Alexander Graf Oct. 7, 2013, 3:20 p.m. UTC | #16
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

--
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
Bharat Bhushan Oct. 7, 2013, 3:43 p.m. UTC | #17
> >>>>>>> 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:
--------
#ifdef CONFIG_KVM_GUEST
 
static unsigned long kvm_hypercall(unsigned long *in,
                                    unsigned long *out,
                                    unsigned long nr)
{
	return epapr_hypercall(in, out. nr);
}
 
 #else
static unsigned long kvm_hypercall(unsigned long *in,
                                    unsigned long *out,
                                    unsigned long nr) {
         return EV_UNIMPLEMENTED;
}
---------

I am still not really convinced about why we want to keep this stub where we know this is not called outside KVM_GUEST and calling this without KVM_GUEST is debatable.

Thanks
-Bharat

Thanks
-Bharat

> 
> 
> 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
> 


--
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
Alexander Graf Oct. 7, 2013, 3:46 p.m. UTC | #18
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()

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
Bharat Bhushan Oct. 7, 2013, 4:04 p.m. UTC | #19
> -----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


--
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
Alexander Graf Oct. 7, 2013, 4:12 p.m. UTC | #20
On 07.10.2013, at 18:04, Bhushan Bharat-R65777 <R65777@freescale.com> wrote:

> 
> 
>> -----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

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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan Oct. 7, 2013, 4:15 p.m. UTC | #21
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Monday, October 07, 2013 9:43 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 18:04, Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
> 
> >
> >
> >> -----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
> 
> 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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/powerpc/include/asm/epapr_hcalls.h b/arch/powerpc/include/asm/epapr_hcalls.h
index d3d6342..8a85f6f 100644
--- a/arch/powerpc/include/asm/epapr_hcalls.h
+++ b/arch/powerpc/include/asm/epapr_hcalls.h
@@ -454,5 +454,41 @@  static inline unsigned int ev_idle(void)
 
 	return r3;
 }
+
+static inline unsigned long epapr_hypercall(unsigned long *in,
+			    unsigned long *out,
+			    unsigned long nr)
+{
+	unsigned long register r0 asm("r0");
+	unsigned long register r3 asm("r3") = in[0];
+	unsigned long register r4 asm("r4") = in[1];
+	unsigned long register r5 asm("r5") = in[2];
+	unsigned long register r6 asm("r6") = in[3];
+	unsigned long register r7 asm("r7") = in[4];
+	unsigned long register r8 asm("r8") = in[5];
+	unsigned long register r9 asm("r9") = in[6];
+	unsigned long register r10 asm("r10") = in[7];
+	unsigned long register r11 asm("r11") = nr;
+	unsigned long register r12 asm("r12");
+
+	asm volatile("bl	epapr_hypercall_start"
+		     : "=r"(r0), "=r"(r3), "=r"(r4), "=r"(r5), "=r"(r6),
+		       "=r"(r7), "=r"(r8), "=r"(r9), "=r"(r10), "=r"(r11),
+		       "=r"(r12)
+		     : "r"(r3), "r"(r4), "r"(r5), "r"(r6), "r"(r7), "r"(r8),
+		       "r"(r9), "r"(r10), "r"(r11)
+		     : "memory", "cc", "xer", "ctr", "lr");
+
+	out[0] = r4;
+	out[1] = r5;
+	out[2] = r6;
+	out[3] = r7;
+	out[4] = r8;
+	out[5] = r9;
+	out[6] = r10;
+	out[7] = r11;
+
+	return r3;
+}
 #endif /* !__ASSEMBLY__ */
 #endif /* _EPAPR_HCALLS_H */
diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
index 2b11965..c18660e 100644
--- a/arch/powerpc/include/asm/kvm_para.h
+++ b/arch/powerpc/include/asm/kvm_para.h
@@ -39,10 +39,6 @@  static inline int kvm_para_available(void)
 	return 1;
 }
 
-extern unsigned long kvm_hypercall(unsigned long *in,
-				   unsigned long *out,
-				   unsigned long nr);
-
 #else
 
 static inline int kvm_para_available(void)
@@ -50,13 +46,6 @@  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
 
 static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
@@ -65,7 +54,7 @@  static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
 	unsigned long out[8];
 	unsigned long r;
 
-	r = kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
+	r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 	*r2 = out[0];
 
 	return r;
@@ -76,7 +65,7 @@  static inline long kvm_hypercall0(unsigned int nr)
 	unsigned long in[8];
 	unsigned long out[8];
 
-	return kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
+	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 }
 
 static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
@@ -85,7 +74,7 @@  static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
 	unsigned long out[8];
 
 	in[0] = p1;
-	return kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
+	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 }
 
 static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
@@ -96,7 +85,7 @@  static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
 
 	in[0] = p1;
 	in[1] = p2;
-	return kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
+	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 }
 
 static inline long kvm_hypercall3(unsigned int nr, unsigned long p1,
@@ -108,7 +97,7 @@  static inline long kvm_hypercall3(unsigned int nr, unsigned long p1,
 	in[0] = p1;
 	in[1] = p2;
 	in[2] = p3;
-	return kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
+	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 }
 
 static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
@@ -122,7 +111,7 @@  static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
 	in[1] = p2;
 	in[2] = p3;
 	in[3] = p4;
-	return kvm_hypercall(in, out, KVM_HCALL_TOKEN(nr));
+	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
 }
 
 
diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
index db28032..6a01752 100644
--- a/arch/powerpc/kernel/kvm.c
+++ b/arch/powerpc/kernel/kvm.c
@@ -413,13 +413,13 @@  static void kvm_map_magic_page(void *data)
 {
 	u32 *features = data;
 
-	ulong in[8];
+	ulong in[8] = {0};
 	ulong out[8];
 
 	in[0] = KVM_MAGIC_PAGE;
 	in[1] = KVM_MAGIC_PAGE;
 
-	kvm_hypercall(in, out, KVM_HCALL_TOKEN(KVM_HC_PPC_MAP_MAGIC_PAGE));
+	epapr_hypercall(in, out, KVM_HCALL_TOKEN(KVM_HC_PPC_MAP_MAGIC_PAGE));
 
 	*features = out[0];
 }
@@ -711,43 +711,6 @@  static void kvm_use_magic_page(void)
 			 kvm_patching_worked ? "worked" : "failed");
 }
 
-unsigned long kvm_hypercall(unsigned long *in,
-			    unsigned long *out,
-			    unsigned long nr)
-{
-	unsigned long register r0 asm("r0");
-	unsigned long register r3 asm("r3") = in[0];
-	unsigned long register r4 asm("r4") = in[1];
-	unsigned long register r5 asm("r5") = in[2];
-	unsigned long register r6 asm("r6") = in[3];
-	unsigned long register r7 asm("r7") = in[4];
-	unsigned long register r8 asm("r8") = in[5];
-	unsigned long register r9 asm("r9") = in[6];
-	unsigned long register r10 asm("r10") = in[7];
-	unsigned long register r11 asm("r11") = nr;
-	unsigned long register r12 asm("r12");
-
-	asm volatile("bl	epapr_hypercall_start"
-		     : "=r"(r0), "=r"(r3), "=r"(r4), "=r"(r5), "=r"(r6),
-		       "=r"(r7), "=r"(r8), "=r"(r9), "=r"(r10), "=r"(r11),
-		       "=r"(r12)
-		     : "r"(r3), "r"(r4), "r"(r5), "r"(r6), "r"(r7), "r"(r8),
-		       "r"(r9), "r"(r10), "r"(r11)
-		     : "memory", "cc", "xer", "ctr", "lr");
-
-	out[0] = r4;
-	out[1] = r5;
-	out[2] = r6;
-	out[3] = r7;
-	out[4] = r8;
-	out[5] = r9;
-	out[6] = r10;
-	out[7] = r11;
-
-	return r3;
-}
-EXPORT_SYMBOL_GPL(kvm_hypercall);
-
 static __init void kvm_free_tmp(void)
 {
 	free_reserved_area(&kvm_tmp[kvm_tmp_index],