[PATCH 0/2] kvm/powerpc: hypercall related cleanup

[PATCH 0/2] kvm/powerpc: hypercall related cleanup

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

This patchset does some code cleanup around kvm-hypercall.

Bharat Bhushan (2):
  kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()
  kvm/powerpc: move kvm_hypercall0() and friends to epapr_hypercall0()

 arch/powerpc/include/asm/epapr_hcalls.h |  103 +++++++++++++++++++++++++++++++
 arch/powerpc/include/asm/kvm_para.h     |   80 +-----------------------
 arch/powerpc/kernel/kvm.c               |   41 +------------
 3 files changed, 106 insertions(+), 118 deletions(-)

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

[Bharat Bhushan]

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));
 	*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],
-- 
1.7.0.4

[PATCH 2/2] kvm/powerpc: move kvm_hypercall0() and friends to epapr_hypercall0()

[Bharat Bhushan]

kvm_hypercall0() and friends have nothing KVM specific so moved to
epapr_hypercall0() and friendsi. Also they are moved from
arch/powerpc/include/asm/kvm_para.h to arch/powerpc/include/asm/epapr_hcalls.h

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/epapr_hcalls.h |   67 ++++++++++++++++++++++++++++++
 arch/powerpc/include/asm/kvm_para.h     |   69 +------------------------------
 2 files changed, 68 insertions(+), 68 deletions(-)

diff --git a/arch/powerpc/include/asm/epapr_hcalls.h b/arch/powerpc/include/asm/epapr_hcalls.h
index 8a85f6f..523b8ac 100644
--- a/arch/powerpc/include/asm/epapr_hcalls.h
+++ b/arch/powerpc/include/asm/epapr_hcalls.h
@@ -490,5 +490,72 @@ static inline unsigned long epapr_hypercall(unsigned long *in,
 
 	return r3;
 }
+
+static inline long epapr_hypercall0_1(unsigned int nr, unsigned long *r2)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+	unsigned long r;
+
+	r = epapr_hypercall(in, out, nr);
+	*r2 = out[0];
+
+	return r;
+}
+
+static inline long epapr_hypercall0(unsigned int nr)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	return epapr_hypercall(in, out, nr);
+}
+
+static inline long epapr_hypercall1(unsigned int nr, unsigned long p1)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	in[0] = p1;
+	return epapr_hypercall(in, out, nr);
+}
+
+static inline long epapr_hypercall2(unsigned int nr, unsigned long p1,
+				  unsigned long p2)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	in[0] = p1;
+	in[1] = p2;
+	return epapr_hypercall(in, out, nr);
+}
+
+static inline long epapr_hypercall3(unsigned int nr, unsigned long p1,
+				  unsigned long p2, unsigned long p3)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	in[0] = p1;
+	in[1] = p2;
+	in[2] = p3;
+	return epapr_hypercall(in, out, nr);
+}
+
+static inline long epapr_hypercall4(unsigned int nr, unsigned long p1,
+				  unsigned long p2, unsigned long p3,
+				  unsigned long p4)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	in[0] = p1;
+	in[1] = p2;
+	in[2] = p3;
+	in[3] = p4;
+	return epapr_hypercall(in, out, nr);
+}
+
 #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 c18660e..336a91a 100644
--- a/arch/powerpc/include/asm/kvm_para.h
+++ b/arch/powerpc/include/asm/kvm_para.h
@@ -48,73 +48,6 @@ static inline int kvm_para_available(void)
 
 #endif
 
-static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-	unsigned long r;
-
-	r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-	*r2 = out[0];
-
-	return r;
-}
-
-static inline long kvm_hypercall0(unsigned int nr)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	in[0] = p1;
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
-				  unsigned long p2)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	in[0] = p1;
-	in[1] = p2;
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-static inline long kvm_hypercall3(unsigned int nr, unsigned long p1,
-				  unsigned long p2, unsigned long p3)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	in[0] = p1;
-	in[1] = p2;
-	in[2] = p3;
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
-				  unsigned long p2, unsigned long p3,
-				  unsigned long p4)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	in[0] = p1;
-	in[1] = p2;
-	in[2] = p3;
-	in[3] = p4;
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-
 static inline unsigned int kvm_arch_para_features(void)
 {
 	unsigned long r;
@@ -122,7 +55,7 @@ static inline unsigned int kvm_arch_para_features(void)
 	if (!kvm_para_available())
 		return 0;
 
-	if(kvm_hypercall0_1(KVM_HC_FEATURES, &r))
+	if(epapr_hypercall0_1(KVM_HCALL_TOKEN(KVM_HC_FEATURES), &r))
 		return 0;
 
 	return r;
-- 
1.7.0.4

[PATCH 0/2] kvm/powerpc: hypercall related cleanup

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

This patchset does some code cleanup around kvm-hypercall.

Bharat Bhushan (2):
  kvm/powerpc: rename kvm_hypercall() to epapr_hypercall()
  kvm/powerpc: move kvm_hypercall0() and friends to epapr_hypercall0()

 arch/powerpc/include/asm/epapr_hcalls.h |  103 +++++++++++++++++++++++++++++++
 arch/powerpc/include/asm/kvm_para.h     |   80 +-----------------------
 arch/powerpc/kernel/kvm.c               |   41 +------------
 3 files changed, 106 insertions(+), 118 deletions(-)

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

[Bharat Bhushan]

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));
 	*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],
-- 
1.7.0.4

[PATCH 2/2] kvm/powerpc: move kvm_hypercall0() and friends to epapr_hypercall0()

[Bharat Bhushan]

kvm_hypercall0() and friends have nothing KVM specific so moved to
epapr_hypercall0() and friendsi. Also they are moved from
arch/powerpc/include/asm/kvm_para.h to arch/powerpc/include/asm/epapr_hcalls.h

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/epapr_hcalls.h |   67 ++++++++++++++++++++++++++++++
 arch/powerpc/include/asm/kvm_para.h     |   69 +------------------------------
 2 files changed, 68 insertions(+), 68 deletions(-)

diff --git a/arch/powerpc/include/asm/epapr_hcalls.h b/arch/powerpc/include/asm/epapr_hcalls.h
index 8a85f6f..523b8ac 100644
--- a/arch/powerpc/include/asm/epapr_hcalls.h
+++ b/arch/powerpc/include/asm/epapr_hcalls.h
@@ -490,5 +490,72 @@ static inline unsigned long epapr_hypercall(unsigned long *in,
 
 	return r3;
 }
+
+static inline long epapr_hypercall0_1(unsigned int nr, unsigned long *r2)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+	unsigned long r;
+
+	r = epapr_hypercall(in, out, nr);
+	*r2 = out[0];
+
+	return r;
+}
+
+static inline long epapr_hypercall0(unsigned int nr)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	return epapr_hypercall(in, out, nr);
+}
+
+static inline long epapr_hypercall1(unsigned int nr, unsigned long p1)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	in[0] = p1;
+	return epapr_hypercall(in, out, nr);
+}
+
+static inline long epapr_hypercall2(unsigned int nr, unsigned long p1,
+				  unsigned long p2)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	in[0] = p1;
+	in[1] = p2;
+	return epapr_hypercall(in, out, nr);
+}
+
+static inline long epapr_hypercall3(unsigned int nr, unsigned long p1,
+				  unsigned long p2, unsigned long p3)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	in[0] = p1;
+	in[1] = p2;
+	in[2] = p3;
+	return epapr_hypercall(in, out, nr);
+}
+
+static inline long epapr_hypercall4(unsigned int nr, unsigned long p1,
+				  unsigned long p2, unsigned long p3,
+				  unsigned long p4)
+{
+	unsigned long in[8];
+	unsigned long out[8];
+
+	in[0] = p1;
+	in[1] = p2;
+	in[2] = p3;
+	in[3] = p4;
+	return epapr_hypercall(in, out, nr);
+}
+
 #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 c18660e..336a91a 100644
--- a/arch/powerpc/include/asm/kvm_para.h
+++ b/arch/powerpc/include/asm/kvm_para.h
@@ -48,73 +48,6 @@ static inline int kvm_para_available(void)
 
 #endif
 
-static inline long kvm_hypercall0_1(unsigned int nr, unsigned long *r2)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-	unsigned long r;
-
-	r = epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-	*r2 = out[0];
-
-	return r;
-}
-
-static inline long kvm_hypercall0(unsigned int nr)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	in[0] = p1;
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
-				  unsigned long p2)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	in[0] = p1;
-	in[1] = p2;
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-static inline long kvm_hypercall3(unsigned int nr, unsigned long p1,
-				  unsigned long p2, unsigned long p3)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	in[0] = p1;
-	in[1] = p2;
-	in[2] = p3;
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
-				  unsigned long p2, unsigned long p3,
-				  unsigned long p4)
-{
-	unsigned long in[8];
-	unsigned long out[8];
-
-	in[0] = p1;
-	in[1] = p2;
-	in[2] = p3;
-	in[3] = p4;
-	return epapr_hypercall(in, out, KVM_HCALL_TOKEN(nr));
-}
-
-
 static inline unsigned int kvm_arch_para_features(void)
 {
 	unsigned long r;
@@ -122,7 +55,7 @@ static inline unsigned int kvm_arch_para_features(void)
 	if (!kvm_para_available())
 		return 0;
 
-	if(kvm_hypercall0_1(KVM_HC_FEATURES, &r))
+	if(epapr_hypercall0_1(KVM_HCALL_TOKEN(KVM_HC_FEATURES), &r))
 		return 0;
 
 	return r;
-- 
1.7.0.4

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

[Scott Wood]

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

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

[Scott Wood]

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

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

[Scott Wood]

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

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

[Scott Wood]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Scott Wood]

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

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

[Scott Wood]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Scott Wood]

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

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

[Scott Wood]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Scott Wood]

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

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

[Scott Wood]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Scott Wood]

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

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

[Scott Wood]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Scott Wood]

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

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

[Scott Wood]

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

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

[Bhushan Bharat-R65777]

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

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

[Bhushan Bharat-R65777]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogVGh1cnNkYXksIE9jdG9iZXIgMDMsIDIwMTMgMTI6MDQgQU0NCj4gVG86IEFs
ZXhhbmRlciBHcmFmDQo+IENjOiBCaHVzaGFuIEJoYXJhdC1SNjU3Nzc7IGt2bS1wcGNAdmdlci5r
ZXJuZWwub3JnOyBrdm1Admdlci5rZXJuZWwub3JnOyBCaHVzaGFuDQo+IEJoYXJhdC1SNjU3NzcN
Cj4gU3ViamVjdDogUmU6IFtQQVRDSCAxLzJdIGt2bS9wb3dlcnBjOiByZW5hbWUga3ZtX2h5cGVy
Y2FsbCgpIHRvDQo+IGVwYXByX2h5cGVyY2FsbCgpDQo+IA0KPiBPbiBXZWQsIDIwMTMtMTAtMDIg
YXQgMTk6NTQgKzAyMDAsIEFsZXhhbmRlciBHcmFmIHdyb3RlOg0KPiA+IE9uIDAyLjEwLjIwMTMs
IGF0IDE5OjQ5LCBTY290dCBXb29kIHdyb3RlOg0KPiA+DQo+ID4gPiBPbiBXZWQsIDIwMTMtMTAt
MDIgYXQgMTk6NDYgKzAyMDAsIEFsZXhhbmRlciBHcmFmIHdyb3RlOg0KPiA+ID4+IE9uIDAyLjEw
LjIwMTMsIGF0IDE5OjQyLCBTY290dCBXb29kIHdyb3RlOg0KPiA+ID4+DQo+ID4gPj4+IE9uIFdl
ZCwgMjAxMy0xMC0wMiBhdCAxOToxNyArMDIwMCwgQWxleGFuZGVyIEdyYWYgd3JvdGU6DQo+ID4g
Pj4+PiBPbiAwMi4xMC4yMDEzLCBhdCAxOTowNCwgU2NvdHQgV29vZCB3cm90ZToNCj4gPiA+Pj4+
DQo+ID4gPj4+Pj4gT24gV2VkLCAyMDEzLTEwLTAyIGF0IDE4OjUzICswMjAwLCBBbGV4YW5kZXIg
R3JhZiB3cm90ZToNCj4gPiA+Pj4+Pj4gT24gMDIuMTAuMjAxMywgYXQgMTg6NDAsIFNjb3R0IFdv
b2Qgd3JvdGU6DQo+ID4gPj4+Pj4+DQo+ID4gPj4+Pj4+PiBPbiBXZWQsIDIwMTMtMTAtMDIgYXQg
MTY6MTkgKzAyMDAsIEFsZXhhbmRlciBHcmFmIHdyb3RlOg0KPiA+ID4+Pj4+Pj4+IFdvbid0IHRo
aXMgYnJlYWsgd2hlbiBDT05GSUdfRVBBUFJfUEFSQVZJUlQ9bj8gV2Ugd291bGRuJ3QgaGF2ZQ0K
PiBlcGFwcl9oY2FsbHMuUyBjb21waWxlZCBpbnRvIHRoZSBjb2RlIGJhc2UgdGhlbiBhbmQgdGhl
IGJsIGFib3ZlIHdvdWxkIHJlZmVyZW5jZQ0KPiBhbiB1bmtub3duIGZ1bmN0aW9uLg0KPiA+ID4+
Pj4+Pj4NCj4gPiA+Pj4+Pj4+IEtWTV9HVUVTVCBzZWxlY3RzIEVQQVBSX1BBUkFWSVJULg0KPiA+
ID4+Pj4+Pg0KPiA+ID4+Pj4+PiBCdXQgeW91IGNhbiBub3Qgc2VsZWN0IEtWTV9HVUVTVCBhbmQg
c3RpbGwgY2FsbCB0aGVzZSBpbmxpbmUgZnVuY3Rpb25zLA0KPiBubz8NCj4gPiA+Pj4+Pg0KPiA+
ID4+Pj4+IE5vLg0KPiA+ID4+Pj4+DQo+ID4gPj4+Pj4+IExpa2Uga3ZtX2FyY2hfcGFyYV9mZWF0
dXJlcygpLg0KPiA+ID4+Pj4+DQo+ID4gPj4+Pj4gV2hlcmUgZG9lcyB0aGF0IGdldCBjYWxsZWQg
d2l0aG91dCBLVk1fR1VFU1Q/DQo+ID4gPj4+Pj4NCj4gPiA+Pj4+PiBIb3cgd291bGQgdGhhdCB3
b3JrIGN1cnJlbnRseSwgd2l0aCB0aGUgY2FsbCB0byBrdm1faHlwZXJjYWxsKCkNCj4gPiA+Pj4+
PiBpbiBhcmNoL3Bvd2VycGMva2VybmVsL2t2bS5jICh3aGljaCBjYWxscyBlcGFwcl9oeXBlcmNh
bGwsIEJUVyk/DQo+ID4gPj4+Pg0KPiA+ID4+Pj4gSXQgd291bGRuJ3QgZXZlciBnZXQgY2FsbGVk
IGJlY2F1c2Uga3ZtX2h5cGVyY2FsbCgpIGVuZHMgdXAgYWx3YXlzDQo+IHJldHVybmluZyBFVl9V
TklNUExFTUVOVEVEIHdoZW4gI2lmbmRlZiBDT05GSUdfS1ZNX0dVRVNULg0KPiA+ID4+Pg0KPiA+
ID4+PiBPSywgc28gdGhlIG9iamVjdGlvbiBpcyB0byByZW1vdmluZyB0aGF0IHN0dWI/ICBXaGVy
ZSB3b3VsZCB3ZQ0KPiA+ID4+PiBhY3R1YWxseSB3YW50IHRvIGNhbGwgdGhpcyB3aXRob3V0IGtu
b3dpbmcgdGhhdCBLVk1fR1VFU1Qgb3INCj4gPiA+Pj4gRVBBUFJfUEFSQVZJUlQgYXJlIGVuYWJs
ZWQ/DQo+ID4gPj4NCj4gPiA+PiBJbiBwcm9iaW5nIGNvZGUuIEkgdXN1YWxseSBwcmVmZXINCj4g
PiA+Pg0KPiA+ID4+IGlmIChrdm1fZmVhdHVyZV9hdmFpbGFibGUoWCkpIHsNCj4gPiA+PiAgIC4u
Lg0KPiA+ID4+IH0NCj4gPiA+Pg0KPiA+ID4+IG92ZXINCj4gPiA+Pg0KPiA+ID4+ICNpZmRlZiBD
T05GSUdfS1ZNX0dVRVNUDQo+ID4gPj4gaWYgKGt2bV9mZWF0dXJlX2F2YWlsYWJsZShYKSkgew0K
PiA+ID4+ICAgLi4uDQo+ID4gPj4gfQ0KPiA+ID4+ICNlbmRpZg0KPiA+ID4+DQo+ID4gPj4gYXQg
bGVhc3Qgd2hlbiBJIGNhbiBhdm9pZCBpdC4gV2l0aCB0aGUgY3VycmVudCBjb2RlIHRoZSBjb21w
aWxlciB3b3VsZCBiZQ0KPiBzbWFydCBlbm91Z2ggdG8ganVzdCBvcHRpbWl6ZSBvdXQgdGhlIGNv
bXBsZXRlIGJyYW5jaC4NCj4gPiA+DQo+ID4gPiBTdXJlLiAgTXkgcG9pbnQgaXMsIHdoZXJlIHdv
dWxkIHlvdSBiZSBjYWxsaW5nIHRoYXQgd2hlcmUgdGhlIGVudGlyZQ0KPiA+ID4gZmlsZSBpc24n
dCBwcmVkaWNhdGVkIG9uIChvciBzZWxlY3RpbmcpIENPTkZJR19LVk1fR1VFU1Qgb3Igc2ltaWxh
cj8NCj4gPiA+DQo+ID4gPiBXZSBkb24ndCBkbyB0aGVzZSBzdHVicyBmb3IgZXZlcnkgc2luZ2xl
IGZ1bmN0aW9uIGluIHRoZSBrZXJuZWwgLS0NCj4gPiA+IG9ubHkgb25lcyB3aGVyZSB0aGUgYWJv
dmUgaXMgYSByZWFzb25hYmxlIHVzZSBjYXNlLg0KPiA+DQo+ID4gWWVhaCwgSSdtIGZpbmUgb24g
ZHJvcHBpbmcgaXQsIGJ1dCB3ZSBuZWVkIHRvIG1ha2UgdGhhdCBhIGNvbnNjaW91cyBkZWNpc2lv
bg0KPiBhbmQgdmVyaWZ5IHRoYXQgbm8gY2FsbGVyIHJlbGllcyBvbiBpdC4NCj4gDQo+IGt2bV9w
YXJhX2hhc19mZWF0dXJlKCkgaXMgY2FsbGVkIGZyb20gYXJjaC9wb3dlcnBjL2tlcm5lbC9rdm0u
YywNCj4gYXJjaC94ODYva2VybmVsL2t2bS5jLCBhbmQgYXJjaC94ODYva2VybmVsL2t2bWNsb2Nr
LmMsIGFsbCBvZiB3aGljaCBhcmUgZW5hYmxlZA0KPiBieSBDT05GSUdfS1ZNX0dVRVNULg0KPiAN
Cj4gSSBkaWQgZmluZCBvbmUgZXhhbXBsZSBvZiBrdm1fcGFyYV9hdmFpbGFibGUoKSBiZWluZyB1
c2VkIGluIGFuIHVuZXhwZWN0ZWQgcGxhY2UNCj4gLS0gc291bmQvcGNpL2ludGVsOHgwLmMuICBJ
dCBkZWZpbmVzIGl0cyBvd24gbm9uLUNPTkZJR19LVk1fR1VFU1Qgc3R1YiwgZXZlbg0KPiB0aG91
Z2ggeDg2IGRlZmluZXMga3ZtX3BhcmFfYXZhaWxhYmxlKCkgdXNpbmcgaW5saW5lIENQVUlEIHN0
dWZmIHdoaWNoIHNob3VsZA0KPiB3b3JrIHdpdGhvdXQgQ09ORklHX0tWTV9HVUVTVC4NCj4gSSdt
IG5vdCBzdXJlIHdoeSBpdCBldmVuIG5lZWRzIHRvIGRvIHRoYXQsIHRob3VnaCAtLSBzaG91bGRu
J3QgdGhlIHN1YnNlcXVlbnQNCj4gUENJIHN1YnN5c3RlbSB2ZW5kb3IvZGV2aWNlIGNoZWNrIHNo
b3VsZCBiZSBzdWZmaWNpZW50PyAgTm8gaHlwZXJjYWxscyBhcmUNCj4gaW52b2x2ZWQuDQo+IA0K
PiBUaGF0IHNhaWQsIHRoZSBwb3NzaWJpbGl0eSB0aGF0IHNvbWUgcmFuZG9tIGRyaXZlciBtaWdo
dCB3YW50IHRvIG1ha2UgdXNlIG9mDQo+IHBhcmF2aXJ0IGZlYXR1cmVzIGlzIGEgZGVjZW50IGFy
Z3VtZW50IGZvciBrZWVwaW5nIHRoZSBzdHViLg0KPiANCg0KSSBhbSBub3Qgc3VyZSB3aGVyZSB3
ZSBhcmUgYWdyZWVpbmcgb24/DQpEbyB3ZSB3YW50IHRvIHJlbW92ZSB0aGUgc3R1YiBpbiBhcmNo
L3Bvd2VycGMvaW5jbHVkZS9hc20va3ZtX3BhcmEuaCA/IGFzIHRoZXJlIGlzIG5vIGNhbGxlciB3
aXRob3V0IEtWTV9HVUVTVCBhbmQgaW4gZnV0dXJlIGNhbGxlciBlbnN1cmUgdGhpcyB0byBiZSBj
YWxsZWQgb25seSBmcm9tIGNvZGUgc2VsZWN0ZWQgYnkgS1ZNX0dVRVNUPw0KDQpPciBsZXQgdGhp
cyBzdHViIHN0YXkgdG8gYXZvaWQgYW55IHJhbmRvbSBkcml2ZXIgY2FsbGluZyB0aGlzID8NCg0K
VGhhbmtzDQotQmhhcmF0DQoNCg0KDQoNCg=

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Bhushan Bharat-R65777]

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

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

[Bhushan Bharat-R65777]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Bhushan Bharat-R65777]

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

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

[Bhushan Bharat-R65777]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Bhushan Bharat-R65777]

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

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

[Bhushan Bharat-R65777]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Bhushan Bharat-R65777]

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

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

[Bhushan Bharat-R65777]

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