RE: [PATCH v6 1/6] arm64: smccc: Add support for SMCCCv1.2 extended input/output registers

From: Michael Kelley <mikelley@microsoft.com>
To: Sudeep Holla <sudeep.holla@arm.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Cc: "arve@google.com" <arve@google.com>,
	Andrew Walbran <qwandor@google.com>,
	David Hartley <dhh@qti.qualcomm.com>,
	Achin Gupta <Achin.Gupta@arm.com>,
	Jens Wiklander <jens.wiklander@linaro.org>,
	Arunachalam Ganapathy <arunachalam.ganapathy@arm.com>,
	Marc Bonnici <marc.bonnici@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <Mark.Rutland@arm.com>
Subject: RE: [PATCH v6 1/6] arm64: smccc: Add support for SMCCCv1.2 extended input/output registers
Date: Wed, 5 May 2021 15:28:56 +0000	[thread overview]
Message-ID: <MWHPR21MB159352805E9DACBE428BA6FAD7599@MWHPR21MB1593.namprd21.prod.outlook.com> (raw)
In-Reply-To: <20210505093843.3308691-2-sudeep.holla@arm.com>

From: Sudeep Holla <sudeep.holla@arm.com> Sent: Wednesday, May 5, 2021 2:39 AM
> 
> SMCCC v1.2 allows x8-x17 to be used as parameter registers and x4—x17
> to be used as result registers in SMC64/HVC64. Arm Firmware Framework
> for Armv8-A specification makes use of x0-x7 as parameter and result
> registers. There are other users like Hyper-V who intend to use beyond
> x0-x7 as well.
> 
> Current SMCCC interface in the kernel just use x0-x7 as parameter and
> x0-x3 as result registers as required by SMCCCv1.0. Let us add new
> interface to support this extended set of input/output registers namely
> x0-x17 as both parameter and result registers.
> 
> Cc: Michael Kelley <mikelley@microsoft.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc:Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/kernel/asm-offsets.c |  9 ++++++
>  arch/arm64/kernel/smccc-call.S  | 57 +++++++++++++++++++++++++++++++++
>  include/linux/arm-smccc.h       | 55 +++++++++++++++++++++++++++++++
>  3 files changed, 121 insertions(+)
> 
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 0cb34ccb6e73..74321bc9a459 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -138,6 +138,15 @@ int main(void)
>    DEFINE(ARM_SMCCC_RES_X2_OFFS,		offsetof(struct arm_smccc_res, a2));
>    DEFINE(ARM_SMCCC_QUIRK_ID_OFFS,	offsetof(struct arm_smccc_quirk, id));
>    DEFINE(ARM_SMCCC_QUIRK_STATE_OFFS,	offsetof(struct arm_smccc_quirk, state));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X0_OFFS,	offsetof(struct arm_smccc_1_2_regs, a0));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X2_OFFS,	offsetof(struct arm_smccc_1_2_regs, a2));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X4_OFFS,	offsetof(struct arm_smccc_1_2_regs, a4));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X6_OFFS,	offsetof(struct arm_smccc_1_2_regs, a6));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X8_OFFS,	offsetof(struct arm_smccc_1_2_regs, a8));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X10_OFFS,	offsetof(struct
> arm_smccc_1_2_regs, a10));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X12_OFFS,	offsetof(struct
> arm_smccc_1_2_regs, a12));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X14_OFFS,	offsetof(struct
> arm_smccc_1_2_regs, a14));
> +  DEFINE(ARM_SMCCC_1_2_REGS_X16_OFFS,	offsetof(struct
> arm_smccc_1_2_regs, a16));
>    BLANK();
>    DEFINE(HIBERN_PBE_ORIG,	offsetof(struct pbe, orig_address));
>    DEFINE(HIBERN_PBE_ADDR,	offsetof(struct pbe, address));
> diff --git a/arch/arm64/kernel/smccc-call.S b/arch/arm64/kernel/smccc-call.S
> index d62447964ed9..7d79c5062c5d 100644
> --- a/arch/arm64/kernel/smccc-call.S
> +++ b/arch/arm64/kernel/smccc-call.S
> @@ -43,3 +43,60 @@ SYM_FUNC_START(__arm_smccc_hvc)
>  	SMCCC	hvc
>  SYM_FUNC_END(__arm_smccc_hvc)
>  EXPORT_SYMBOL(__arm_smccc_hvc)
> +
> +	.macro SMCCC_1_2 instr
> +	/* Save `res` and free a GPR that won't be clobbered */
> +	stp     x1, x19, [sp, #-16]!
> +
> +	/* Ensure `args` won't be clobbered while loading regs in next step */
> +	mov	x19, x0
> +
> +	/* Load the registers x0 - x17 from the struct arm_smccc_1_2_regs */
> +	ldp	x0, x1, [x19, #ARM_SMCCC_1_2_REGS_X0_OFFS]
> +	ldp	x2, x3, [x19, #ARM_SMCCC_1_2_REGS_X2_OFFS]
> +	ldp	x4, x5, [x19, #ARM_SMCCC_1_2_REGS_X4_OFFS]
> +	ldp	x6, x7, [x19, #ARM_SMCCC_1_2_REGS_X6_OFFS]
> +	ldp	x8, x9, [x19, #ARM_SMCCC_1_2_REGS_X8_OFFS]
> +	ldp	x10, x11, [x19, #ARM_SMCCC_1_2_REGS_X10_OFFS]
> +	ldp	x12, x13, [x19, #ARM_SMCCC_1_2_REGS_X12_OFFS]
> +	ldp	x14, x15, [x19, #ARM_SMCCC_1_2_REGS_X14_OFFS]
> +	ldp	x16, x17, [x19, #ARM_SMCCC_1_2_REGS_X16_OFFS]
> +
> +	\instr #0
> +
> +	/* Load the `res` from the stack */
> +	ldr	x19, [sp]
> +
> +	/* Store the registers x0 - x17 into the result structure */
> +	stp	x0, x1, [x19, #ARM_SMCCC_1_2_REGS_X0_OFFS]
> +	stp	x2, x3, [x19, #ARM_SMCCC_1_2_REGS_X2_OFFS]
> +	stp	x4, x5, [x19, #ARM_SMCCC_1_2_REGS_X4_OFFS]
> +	stp	x6, x7, [x19, #ARM_SMCCC_1_2_REGS_X6_OFFS]
> +	stp	x8, x9, [x19, #ARM_SMCCC_1_2_REGS_X8_OFFS]
> +	stp	x10, x11, [x19, #ARM_SMCCC_1_2_REGS_X10_OFFS]
> +	stp	x12, x13, [x19, #ARM_SMCCC_1_2_REGS_X12_OFFS]
> +	stp	x14, x15, [x19, #ARM_SMCCC_1_2_REGS_X14_OFFS]
> +	stp	x16, x17, [x19, #ARM_SMCCC_1_2_REGS_X16_OFFS]
> +
> +	/* Restore original x19 */
> +	ldp     xzr, x19, [sp], #16
> +	ret
> +.endm
> +
> +/*
> + * void arm_smccc_1_2_hvc(struct arm_smccc_1_2_regs *args,
> + *			  struct arm_smccc_1_2_regs *res);
> + */
> +SYM_FUNC_START(arm_smccc_1_2_hvc)
> +	SMCCC_1_2 hvc
> +SYM_FUNC_END(arm_smccc_1_2_hvc)
> +EXPORT_SYMBOL(arm_smccc_1_2_hvc)

I've tested the new arm_smccc_1_2_hvc() function in the context
of Linux guests making hypercalls on Hyper-V for ARM64, and it
works as intended.   But note that my test case only uses X0 thru X7.

Tested-by: Michael Kelley <mikelley@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>