[v4,3/8] KVM: PPC: Ultravisor: Add generic ultravisor call handler
diff mbox series

Message ID 20190628200825.31049-4-cclaudio@linux.ibm.com
State Superseded
Headers show
Series
  • kvmppc: Paravirtualize KVM to support ultravisor
Related show

Commit Message

Claudio Carvalho June 28, 2019, 8:08 p.m. UTC
From: Ram Pai <linuxram@us.ibm.com>

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 <linuxram@us.ibm.com>
[ Change ucall.S to not save CR, rename and move headers, build ucall.S
  if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
  comments in the code ]
Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
---
 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               | 30 +++++++++++++++++++++++
 arch/powerpc/kernel/ultravisor.c          |  4 +++
 5 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/include/asm/ultravisor-api.h
 create mode 100644 arch/powerpc/kernel/ucall.S

Comments

janani July 8, 2019, 5:55 p.m. UTC | #1
On 2019-06-28 15:08, Claudio Carvalho wrote:
> From: Ram Pai <linuxram@us.ibm.com>
> 
> 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 <linuxram@us.ibm.com>
> [ Change ucall.S to not save CR, rename and move headers, build ucall.S
>   if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
>   comments in the code ]
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
  Reviewed-by: Janani Janakiraman <janani@linux.ibm.com>
> ---
>  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               | 30 +++++++++++++++++++++++
>  arch/powerpc/kernel/ultravisor.c          |  4 +++
>  5 files changed, 75 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 000000000000..49e766adabc7
> --- /dev/null
> +++ b/arch/powerpc/include/asm/ultravisor-api.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Ultravisor API.
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#ifndef _ASM_POWERPC_ULTRAVISOR_API_H
> +#define _ASM_POWERPC_ULTRAVISOR_API_H
> +
> +#include <asm/hvcall.h>
> +
> +/* Return codes */
> +#define U_NOT_AVAILABLE		H_NOT_AVAILABLE
> +#define U_SUCCESS		H_SUCCESS
> +#define U_FUNCTION		H_FUNCTION
> +#define U_PARAMETER		H_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..a78a2dacfd0b 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 <asm/ultravisor-api.h>
> +
> +#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_POWERNV)
> +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 f0caa302c8c0..f28baccc0a79 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_POWERNV)	+= ultravisor.o
> +obj-$(CONFIG_PPC_POWERNV)	+= 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 000000000000..1678f6eb7230
> --- /dev/null
> +++ b/arch/powerpc/kernel/ucall.S
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Generic code to perform an ultravisor call.
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#include <asm/ppc_asm.h>
> +
> +/*
> + * This function is based on the plpar_hcall()
> + */
> +_GLOBAL_TOC(ucall)
> +	std	r4,STK_PARAM(R4)(r1)	/* Save ret buffer */
> +	mr	r4,r5
> +	mr	r5,r6
> +	mr	r6,r7
> +	mr	r7,r8
> +	mr	r8,r9
> +	mr	r9,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 <linux/init.h>
>  #include <linux/printk.h>
>  #include <linux/string.h>
> +#include <linux/export.h>
> 
>  #include <asm/ultravisor.h>
>  #include <asm/firmware.h>
> 
> +/* in ucall.S */
> +EXPORT_SYMBOL_GPL(ucall);
> +
>  int __init early_init_dt_scan_ultravisor(unsigned long node, const 
> char *uname,
>  					 int depth, void *data)
>  {
Michael Ellerman July 11, 2019, 12:57 p.m. UTC | #2
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> From: Ram Pai <linuxram@us.ibm.com>
>
> 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().

.. with quite a few changes?

This is one of the things I'd like to see in a Documentation file, so
that people can review the implementation vs the specification.

> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> [ Change ucall.S to not save CR, rename and move headers, build ucall.S
>   if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
>   comments in the code ]

Why are we not saving CR? See previous comment about Documentation :)

> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> ---
>  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               | 30 +++++++++++++++++++++++
>  arch/powerpc/kernel/ultravisor.c          |  4 +++
>  5 files changed, 75 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 000000000000..49e766adabc7
> --- /dev/null
> +++ b/arch/powerpc/include/asm/ultravisor-api.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Ultravisor API.
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#ifndef _ASM_POWERPC_ULTRAVISOR_API_H
> +#define _ASM_POWERPC_ULTRAVISOR_API_H
> +
> +#include <asm/hvcall.h>
> +
> +/* Return codes */
> +#define U_NOT_AVAILABLE		H_NOT_AVAILABLE
> +#define U_SUCCESS		H_SUCCESS
> +#define U_FUNCTION		H_FUNCTION
> +#define U_PARAMETER		H_PARAMETER

Is there any benefit in redefining these?

> diff --git a/arch/powerpc/include/asm/ultravisor.h b/arch/powerpc/include/asm/ultravisor.h
> index e5009b0d84ea..a78a2dacfd0b 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 <asm/ultravisor-api.h>
> +
> +#if !defined(__ASSEMBLY__)

Just #ifndef is fine.

>  /* Internal functions */

How is it internal?

>  extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
>  					 int depth, void *data);
>  
> +/* API functions */
> +#define UCALL_BUFSIZE 4

Please don't copy this design from the hcall code, it has led to bugs in
the past.

See my (still unmerged) attempt to fix this for the hcall case:
  https://patchwork.ozlabs.org/patch/683577/

Basically instead of asking callers nicely to define a certain sized
buffer, and them forgetting, define a proper type that has the right size.

> +/**
> + * 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_POWERNV)

#ifdef

> +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 f0caa302c8c0..f28baccc0a79 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_POWERNV)	+= ultravisor.o
> +obj-$(CONFIG_PPC_POWERNV)	+= ultravisor.o ucall.o

Same comment about being platforms/powernv ?
> 
> diff --git a/arch/powerpc/kernel/ucall.S b/arch/powerpc/kernel/ucall.S
> new file mode 100644
> index 000000000000..1678f6eb7230
> --- /dev/null
> +++ b/arch/powerpc/kernel/ucall.S
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Generic code to perform an ultravisor call.
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#include <asm/ppc_asm.h>
> +
> +/*
> + * This function is based on the plpar_hcall()

I don't think it meaningfully is any more.

> + */
> +_GLOBAL_TOC(ucall)

You don't need the TOC setup here (unless a later patch does?).

> +	std	r4,STK_PARAM(R4)(r1)	/* Save ret buffer */
> +	mr	r4,r5
> +	mr	r5,r6
> +	mr	r6,r7
> +	mr	r7,r8
> +	mr	r8,r9
> +	mr	r9,r10

Below you space the arguments, here you don't. Pick one or the other please.

> +
> +	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 <linux/init.h>
>  #include <linux/printk.h>
>  #include <linux/string.h>
> +#include <linux/export.h>
>  
>  #include <asm/ultravisor.h>
>  #include <asm/firmware.h>
>  
> +/* in ucall.S */
> +EXPORT_SYMBOL_GPL(ucall);

This should be in ucall.S

cheers
Nicholas Piggin July 12, 2019, 1:18 a.m. UTC | #3
Claudio Carvalho's on June 29, 2019 6:08 am:
> From: Ram Pai <linuxram@us.ibm.com>
> 
> 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 <linuxram@us.ibm.com>
> [ Change ucall.S to not save CR, rename and move headers, build ucall.S
>   if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
>   comments in the code ]
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> ---
>  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               | 30 +++++++++++++++++++++++
>  arch/powerpc/kernel/ultravisor.c          |  4 +++
>  5 files changed, 75 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 000000000000..49e766adabc7
> --- /dev/null
> +++ b/arch/powerpc/include/asm/ultravisor-api.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Ultravisor API.
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#ifndef _ASM_POWERPC_ULTRAVISOR_API_H
> +#define _ASM_POWERPC_ULTRAVISOR_API_H
> +
> +#include <asm/hvcall.h>
> +
> +/* Return codes */
> +#define U_NOT_AVAILABLE		H_NOT_AVAILABLE
> +#define U_SUCCESS		H_SUCCESS
> +#define U_FUNCTION		H_FUNCTION
> +#define U_PARAMETER		H_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..a78a2dacfd0b 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 <asm/ultravisor-api.h>
> +
> +#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_POWERNV)
> +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 f0caa302c8c0..f28baccc0a79 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_POWERNV)	+= ultravisor.o
> +obj-$(CONFIG_PPC_POWERNV)	+= 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 000000000000..1678f6eb7230
> --- /dev/null
> +++ b/arch/powerpc/kernel/ucall.S
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Generic code to perform an ultravisor call.
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#include <asm/ppc_asm.h>
> +
> +/*
> + * This function is based on the plpar_hcall()

A better comment might be to specify calling convention.

> + */
> +_GLOBAL_TOC(ucall)
> +	std	r4,STK_PARAM(R4)(r1)	/* Save ret buffer */
> +	mr	r4,r5
> +	mr	r5,r6
> +	mr	r6,r7
> +	mr	r7,r8
> +	mr	r8,r9
> +	mr	r9,r10

I guess this is fine, is there any particular reason that you made
the call convention this way rather than leaving regs in place and
r4 is a scratch?

> +
> +	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)
> +
Claudio Carvalho July 13, 2019, 5:42 p.m. UTC | #4
On 7/11/19 9:57 AM, Michael Ellerman wrote:
> Claudio Carvalho <cclaudio@linux.ibm.com> writes:
>> From: Ram Pai <linuxram@us.ibm.com>
>>
>> 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().
> .. with quite a few changes?
>
> This is one of the things I'd like to see in a Documentation file, so
> that people can review the implementation vs the specification.

I will document this (and other things) in a file under Documentation/powerpc.


>
>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> [ Change ucall.S to not save CR, rename and move headers, build ucall.S
>>   if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
>>   comments in the code ]
> Why are we not saving CR? See previous comment about Documentation :)
>
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
>> ---
>>  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               | 30 +++++++++++++++++++++++
>>  arch/powerpc/kernel/ultravisor.c          |  4 +++
>>  5 files changed, 75 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 000000000000..49e766adabc7
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/ultravisor-api.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Ultravisor API.
>> + *
>> + * Copyright 2019, IBM Corporation.
>> + *
>> + */
>> +#ifndef _ASM_POWERPC_ULTRAVISOR_API_H
>> +#define _ASM_POWERPC_ULTRAVISOR_API_H
>> +
>> +#include <asm/hvcall.h>
>> +
>> +/* Return codes */
>> +#define U_NOT_AVAILABLE		H_NOT_AVAILABLE
>> +#define U_SUCCESS		H_SUCCESS
>> +#define U_FUNCTION		H_FUNCTION
>> +#define U_PARAMETER		H_PARAMETER
> Is there any benefit in redefining these?
>
>> diff --git a/arch/powerpc/include/asm/ultravisor.h b/arch/powerpc/include/asm/ultravisor.h
>> index e5009b0d84ea..a78a2dacfd0b 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 <asm/ultravisor-api.h>
>> +
>> +#if !defined(__ASSEMBLY__)
> Just #ifndef is fine.
>
>>  /* Internal functions */
> How is it internal?
>
>>  extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
>>  					 int depth, void *data);
>>  
>> +/* API functions */
>> +#define UCALL_BUFSIZE 4
> Please don't copy this design from the hcall code, it has led to bugs in
> the past.
>
> See my (still unmerged) attempt to fix this for the hcall case:
>   https://patchwork.ozlabs.org/patch/683577/
>
> Basically instead of asking callers nicely to define a certain sized
> buffer, and them forgetting, define a proper type that has the right size.

I will keep that in mind. For now I think we don't need that since the v5
will have ucall_norets() instead.


>
>> +/**
>> + * 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_POWERNV)
> #ifdef
>
>> +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 f0caa302c8c0..f28baccc0a79 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_POWERNV)	+= ultravisor.o
>> +obj-$(CONFIG_PPC_POWERNV)	+= ultravisor.o ucall.o
> Same comment about being platforms/powernv ?
>> diff --git a/arch/powerpc/kernel/ucall.S b/arch/powerpc/kernel/ucall.S
>> new file mode 100644
>> index 000000000000..1678f6eb7230
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/ucall.S
>> @@ -0,0 +1,30 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Generic code to perform an ultravisor call.
>> + *
>> + * Copyright 2019, IBM Corporation.
>> + *
>> + */
>> +#include <asm/ppc_asm.h>
>> +
>> +/*
>> + * This function is based on the plpar_hcall()
> I don't think it meaningfully is any more.
>
>> + */
>> +_GLOBAL_TOC(ucall)
> You don't need the TOC setup here (unless a later patch does?).
>
>> +	std	r4,STK_PARAM(R4)(r1)	/* Save ret buffer */
>> +	mr	r4,r5
>> +	mr	r5,r6
>> +	mr	r6,r7
>> +	mr	r7,r8
>> +	mr	r8,r9
>> +	mr	r9,r10
> Below you space the arguments, here you don't. Pick one or the other please.
>
>> +
>> +	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 <linux/init.h>
>>  #include <linux/printk.h>
>>  #include <linux/string.h>
>> +#include <linux/export.h>
>>  
>>  #include <asm/ultravisor.h>
>>  #include <asm/firmware.h>
>>  
>> +/* in ucall.S */
>> +EXPORT_SYMBOL_GPL(ucall);
> This should be in ucall.S

Here I'm following the same way that hypercall wrapper symbols are exported.

Last time I tried to export that in ucall.S the linker complained that the
ucall
symbol will not be versioned. Something like this:
https://patchwork.kernel.org/patch/9452759/

Thanks,
Claudio


>
> cheers
Michael Ellerman July 15, 2019, 4:46 a.m. UTC | #5
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> On 7/11/19 9:57 AM, Michael Ellerman wrote:
>> Claudio Carvalho <cclaudio@linux.ibm.com> writes:
>>> From: Ram Pai <linuxram@us.ibm.com>
>>>
>>> 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().
>> .. with quite a few changes?
>>
>> This is one of the things I'd like to see in a Documentation file, so
>> that people can review the implementation vs the specification.
>
> I will document this (and other things) in a file under Documentation/powerpc.

Thanks.

>>> +/* API functions */
>>> +#define UCALL_BUFSIZE 4
>> Please don't copy this design from the hcall code, it has led to bugs in
>> the past.
>>
>> See my (still unmerged) attempt to fix this for the hcall case:
>>   https://patchwork.ozlabs.org/patch/683577/
>>
>> Basically instead of asking callers nicely to define a certain sized
>> buffer, and them forgetting, define a proper type that has the right size.
>
> I will keep that in mind. For now I think we don't need that since the v5
> will have ucall_norets() instead.

OK.

>>> 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 <linux/init.h>
>>>  #include <linux/printk.h>
>>>  #include <linux/string.h>
>>> +#include <linux/export.h>
>>>  
>>>  #include <asm/ultravisor.h>
>>>  #include <asm/firmware.h>
>>>  
>>> +/* in ucall.S */
>>> +EXPORT_SYMBOL_GPL(ucall);
>> This should be in ucall.S
>
> Here I'm following the same way that hypercall wrapper symbols are exported.

Yeah we used to not be able to export from .S but that was fixed a few
years ago.

> Last time I tried to export that in ucall.S the linker complained that the
> ucall
> symbol will not be versioned. Something like this:
> https://patchwork.kernel.org/patch/9452759/

I think you just need to include <asm/export.h> in the .S ?

cheers

Patch
diff mbox series

diff --git a/arch/powerpc/include/asm/ultravisor-api.h b/arch/powerpc/include/asm/ultravisor-api.h
new file mode 100644
index 000000000000..49e766adabc7
--- /dev/null
+++ b/arch/powerpc/include/asm/ultravisor-api.h
@@ -0,0 +1,20 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Ultravisor API.
+ *
+ * Copyright 2019, IBM Corporation.
+ *
+ */
+#ifndef _ASM_POWERPC_ULTRAVISOR_API_H
+#define _ASM_POWERPC_ULTRAVISOR_API_H
+
+#include <asm/hvcall.h>
+
+/* Return codes */
+#define U_NOT_AVAILABLE		H_NOT_AVAILABLE
+#define U_SUCCESS		H_SUCCESS
+#define U_FUNCTION		H_FUNCTION
+#define U_PARAMETER		H_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..a78a2dacfd0b 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 <asm/ultravisor-api.h>
+
+#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_POWERNV)
+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 f0caa302c8c0..f28baccc0a79 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_POWERNV)	+= ultravisor.o
+obj-$(CONFIG_PPC_POWERNV)	+= 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 000000000000..1678f6eb7230
--- /dev/null
+++ b/arch/powerpc/kernel/ucall.S
@@ -0,0 +1,30 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Generic code to perform an ultravisor call.
+ *
+ * Copyright 2019, IBM Corporation.
+ *
+ */
+#include <asm/ppc_asm.h>
+
+/*
+ * This function is based on the plpar_hcall()
+ */
+_GLOBAL_TOC(ucall)
+	std	r4,STK_PARAM(R4)(r1)	/* Save ret buffer */
+	mr	r4,r5
+	mr	r5,r6
+	mr	r6,r7
+	mr	r7,r8
+	mr	r8,r9
+	mr	r9,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 <linux/init.h>
 #include <linux/printk.h>
 #include <linux/string.h>
+#include <linux/export.h>
 
 #include <asm/ultravisor.h>
 #include <asm/firmware.h>
 
+/* in ucall.S */
+EXPORT_SYMBOL_GPL(ucall);
+
 int __init early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
 					 int depth, void *data)
 {