Re: Re: [PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler
On Tue, Jun 18, 2019 at 06:47:01AM -0500, Segher Boessenkool wrote: > Hi Paul, > > On Mon, Jun 17, 2019 at 12:06:32PM +1000, Paul Mackerras wrote: > > The thing we need to consider is that when SMFCTRL[E] = 0, a ucall > > instruction becomes a hcall (that is, sc 2 is executed as if it was > > sc 1). In that case, the first argument to the ucall will be > > interpreted as the hcall number. Mostly that will happen not to be a > > valid hcall number, but sometimes it might unavoidably be a valid but > > unintended hcall number. > > Shouldn't a caller of the ultravisor *know* that it is talking to the > ultravisor in the first place? And not to the hypervisor. It may or may not. But if it knows and still decides to make the ucall, the hypervisor must gracefully handle it. We can't control who makes a ucall. A normal process within the VM could make a ucall too. Or a normal process running on top of the hypervisor could make a ucall. RP
Re: [PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler
Hi Paul, On Mon, Jun 17, 2019 at 12:06:32PM +1000, Paul Mackerras wrote: > The thing we need to consider is that when SMFCTRL[E] = 0, a ucall > instruction becomes a hcall (that is, sc 2 is executed as if it was > sc 1). In that case, the first argument to the ucall will be > interpreted as the hcall number. Mostly that will happen not to be a > valid hcall number, but sometimes it might unavoidably be a valid but > unintended hcall number. Shouldn't a caller of the ultravisor *know* that it is talking to the ultravisor in the first place? And not to the hypervisor. Segher
Re: Re: [PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler
On Mon, Jun 17, 2019 at 12:06:32PM +1000, Paul Mackerras wrote: > On Thu, Jun 06, 2019 at 02:36:09PM -0300, Claudio Carvalho wrote: > > From: Ram Pai > > > > Add the ucall() function, which can be used to make ultravisor calls > > with varied number of in and out arguments. Ultravisor calls can be made > > from the host or guests. > > > > This copies the implementation of plpar_hcall(). > > One point which I missed when I looked at this patch previously is > that the ABI that we're defining here is different from the hcall ABI > in that we are putting the ucall number in r0, whereas hcalls have the > hcall number in r3. That makes ucalls more like syscalls, which have > the syscall number in r0. So that last sentence quoted above is > somewhat misleading. > > The thing we need to consider is that when SMFCTRL[E] = 0, a ucall > instruction becomes a hcall (that is, sc 2 is executed as if it was > sc 1). In that case, the first argument to the ucall will be > interpreted as the hcall number. Mostly that will happen not to be a > valid hcall number, but sometimes it might unavoidably be a valid but > unintended hcall number. > > I think that will make it difficult to get ucalls to fail gracefully > in the case where SMF/PEF is disabled. It seems like the assignment > of ucall numbers was made so that they wouldn't overlap with valid > hcall numbers; presumably that was so that we could tell when an hcall > was actually intended to be a ucall. However, using a different GPR > to pass the ucall number defeats that. Right this is a valid point. Glad that you caught it, otherwise it would have become a difficult to fix it in the future. > > I realize that there is ultravisor code in development that takes the > ucall number in r0, and also that having the ucall number in r3 would > possibly make life more difficult for the place where we call > UV_RETURN in assembler code. Its called from one place in the hypervisor, and the changes look simple. - LOAD_REG_IMMEDIATE(r0, UV_RETURN) + LOAD_REG_IMMEDIATE(r3, UV_RETURN) ld r7, VCPU_GPR(R7)(r4) ld r6, VCPU_GPR(R6)(r4) ld r4, VCPU_GPR(R4)(r4) What am i missing? > Nevertheless, perhaps we should consider > changing the ABI to be like the hcall ABI before everything gets set > in concrete. yes. Thanks Paul! RP
Re: [PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler
On Thu, Jun 06, 2019 at 02:36:09PM -0300, Claudio Carvalho wrote: > From: Ram Pai > > Add the ucall() function, which can be used to make ultravisor calls > with varied number of in and out arguments. Ultravisor calls can be made > from the host or guests. > > This copies the implementation of plpar_hcall(). One point which I missed when I looked at this patch previously is that the ABI that we're defining here is different from the hcall ABI in that we are putting the ucall number in r0, whereas hcalls have the hcall number in r3. That makes ucalls more like syscalls, which have the syscall number in r0. So that last sentence quoted above is somewhat misleading. The thing we need to consider is that when SMFCTRL[E] = 0, a ucall instruction becomes a hcall (that is, sc 2 is executed as if it was sc 1). In that case, the first argument to the ucall will be interpreted as the hcall number. Mostly that will happen not to be a valid hcall number, but sometimes it might unavoidably be a valid but unintended hcall number. I think that will make it difficult to get ucalls to fail gracefully in the case where SMF/PEF is disabled. It seems like the assignment of ucall numbers was made so that they wouldn't overlap with valid hcall numbers; presumably that was so that we could tell when an hcall was actually intended to be a ucall. However, using a different GPR to pass the ucall number defeats that. I realize that there is ultravisor code in development that takes the ucall number in r0, and also that having the ucall number in r3 would possibly make life more difficult for the place where we call UV_RETURN in assembler code. Nevertheless, perhaps we should consider changing the ABI to be like the hcall ABI before everything gets set in concrete. Paul.
Re: [PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler
On Thu, Jun 06, 2019 at 02:36:09PM -0300, Claudio Carvalho wrote: > From: Ram Pai > > Add the ucall() function, which can be used to make ultravisor calls > with varied number of in and out arguments. Ultravisor calls can be made > from the host or guests. > > This copies the implementation of plpar_hcall(). Again, we will want all of this on every powernv-capable kernel, since they will all need to do UV_WRITE_PATE, even if they have no other support for the ultravisor. Paul.
[PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler
From: Ram Pai
Add the ucall() function, which can be used to make ultravisor calls
with varied number of in and out arguments. Ultravisor calls can be made
from the host or guests.
This copies the implementation of plpar_hcall().
Signed-off-by: Ram Pai
[Change ucall.S to not save CR, rename and move the headers, build
ucall.S if CONFIG_PPC_UV set, and add some comments in the code]
Signed-off-by: Claudio Carvalho
---
arch/powerpc/include/asm/ultravisor-api.h | 20 +++
arch/powerpc/include/asm/ultravisor.h | 20 +++
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/ucall.S | 31 +++
arch/powerpc/kernel/ultravisor.c | 4 +++
5 files changed, 76 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/ultravisor-api.h
create mode 100644 arch/powerpc/kernel/ucall.S
diff --git a/arch/powerpc/include/asm/ultravisor-api.h
b/arch/powerpc/include/asm/ultravisor-api.h
new file mode 100644
index ..5f538f33c704
--- /dev/null
+++ b/arch/powerpc/include/asm/ultravisor-api.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Ultravisor calls.
+ *
+ * Copyright 2019, IBM Corporation.
+ *
+ */
+#ifndef _ASM_POWERPC_ULTRAVISOR_API_H
+#define _ASM_POWERPC_ULTRAVISOR_API_H
+
+#include
+
+/* Return codes */
+#define U_NOT_AVAILABLEH_NOT_AVAILABLE
+#define U_SUCCESS H_SUCCESS
+#define U_FUNCTION H_FUNCTION
+#define U_PARAMETERH_PARAMETER
+
+#endif /* _ASM_POWERPC_ULTRAVISOR_API_H */
+
diff --git a/arch/powerpc/include/asm/ultravisor.h
b/arch/powerpc/include/asm/ultravisor.h
index e5009b0d84ea..7500771a8ebd 100644
--- a/arch/powerpc/include/asm/ultravisor.h
+++ b/arch/powerpc/include/asm/ultravisor.h
@@ -8,8 +8,28 @@
#ifndef _ASM_POWERPC_ULTRAVISOR_H
#define _ASM_POWERPC_ULTRAVISOR_H
+#include
+
+#if !defined(__ASSEMBLY__)
+
/* Internal functions */
extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
int depth, void *data);
+/* API functions */
+#define UCALL_BUFSIZE 4
+/**
+ * ucall: Make a powerpc ultravisor call.
+ * @opcode: The ultravisor call to make.
+ * @retbuf: Buffer to store up to 4 return arguments in.
+ *
+ * This call supports up to 6 arguments and 4 return arguments. Use
+ * UCALL_BUFSIZE to size the return argument buffer.
+ */
+#if defined(CONFIG_PPC_UV)
+long ucall(unsigned long opcode, unsigned long *retbuf, ...);
+#endif
+
+#endif /* !__ASSEMBLY__ */
+
#endif /* _ASM_POWERPC_ULTRAVISOR_H */
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index c8ca219e54bf..43ff4546e469 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -154,7 +154,7 @@ endif
obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o
obj-$(CONFIG_KVM_GUEST)+= kvm.o kvm_emul.o
-obj-$(CONFIG_PPC_UV) += ultravisor.o
+obj-$(CONFIG_PPC_UV) += ultravisor.o ucall.o
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
diff --git a/arch/powerpc/kernel/ucall.S b/arch/powerpc/kernel/ucall.S
new file mode 100644
index ..ecc88998a13b
--- /dev/null
+++ b/arch/powerpc/kernel/ucall.S
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Generic code to perform an ultravisor call.
+ *
+ * Copyright 2019, IBM Corporation.
+ *
+ */
+#include
+
+/*
+ * This function is based on the plpar_hcall()
+ */
+_GLOBAL_TOC(ucall)
+ mr r0,r3
+ std r4,STK_PARAM(R4)(r1) /* Save ret buffer */
+ mr r3,r5
+ mr r4,r6
+ mr r5,r7
+ mr r6,r8
+ mr r7,r9
+ mr r8,r10
+
+ sc 2/* invoke the ultravisor */
+
+ ld r12,STK_PARAM(R4)(r1)
+ std r4, 0(r12)
+ std r5, 8(r12)
+ std r6, 16(r12)
+ std r7, 24(r12)
+
+ blr /* return r3 = status */
diff --git a/arch/powerpc/kernel/ultravisor.c b/arch/powerpc/kernel/ultravisor.c
index dc6021f63c97..02ddf79a9522 100644
--- a/arch/powerpc/kernel/ultravisor.c
+++ b/arch/powerpc/kernel/ultravisor.c
@@ -8,10 +8,14 @@
#include
#include
#include
+#include
#include
#include
+/* in ucall.S */
+EXPORT_SYMBOL_GPL(ucall);
+
int __init early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
int depth, void *data)
{
--
2.20.1
