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

[Claudio Carvalho]

On 7/11/19 9:57 AM, Michael Ellerman wrote:
> 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.

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


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

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