[PATCH v4 3/8] KVM: PPC: Ultravisor: Add generic ultravisor call handler

[Michael Ellerman]

Claudio Carvalho <cclaudio at linux.ibm.com> writes:
> From: Ram Pai <linuxram at 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 at 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 at 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