Re: [PATCH v5 2/7] arm64: smccc: Add support for SMCCCv1.2 input/output registers

[Mark Rutland <mark.rutland@arm.com>]

On Thu, Mar 25, 2021 at 02:41:13PM +0000, Mark Rutland wrote:
> Hi Sudeep,
> 
> On Thu, Mar 25, 2021 at 02:32:50PM +0000, Sudeep Holla wrote:
> > 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.
> > 
> > Current SMCCC interface in the kernel just use x0-x7 as parameter and
> > x0-x3 as result registers. Let us add new interface to support x0-x7
> > as parameter and result registers. This can be extended to include
> > x8-x17 when there are users for the same.
> 
> Michael Kelley is also looking at using SMCCCv1.2, and on his HyperV
> thread I'd suggested we should deal with the whole set of SMCCCv1.2
> registers now to avoid future churn in this area (using struct both for
> the arguments and return values).
> 
> How painful would it be to extend this patch to do that?

I *think* the major change with this would be making the interfaces:

void arm_smccc_1_2_{hvc,smc}(struct arm_smccc_1_2_args *args,
			     struct arm_smccc_1_2_res *res);

... and callers manipulating the structs directly, with arm64 having
more fields, e.g.

// arm64
struct arm_smccc_1_2_args {
	unsigned long a1;
	...
	unsigned long a17;
}

struct arm_smccc_1_2_res {
	unsigned long a0;
	...
	unsigned long a17;
}

// arm
struct arm_smccc_1_2_args {
	unsigned long a1;
	...
	unsigned long a7;
}

struct arm_smccc_1_2_res {
	unsigned long a0;
	...
	unsigned long a7;
}

I think that can be hidden in the FF-A wrappers, so that doesn't need to
be what FF-A drivers see. Does that sound plausible, or is that painful?
 
> > +  DEFINE(ARM_SMCCC_V1_2_RES_X0_OFFS,	offsetof(struct arm_smccc_v1_2_res, a0));

As a general nit, for consistency with the existing arm_smccc_1_1 code,
could we please drop the 'V' in these names, and use `ARM_SMCCC_1_2` or
`arm_smccc_1_2` ?

FWIW, other than the above comments, this all looks good to me

Thanks,
Mark.

> > diff --git a/arch/arm64/kernel/smccc-call.S b/arch/arm64/kernel/smccc-call.S
> > index d62447964ed9..0ea15c1742f3 100644
> > --- a/arch/arm64/kernel/smccc-call.S
> > +++ b/arch/arm64/kernel/smccc-call.S
> > @@ -43,3 +43,25 @@ SYM_FUNC_START(__arm_smccc_hvc)
> >  	SMCCC	hvc
> >  SYM_FUNC_END(__arm_smccc_hvc)
> >  EXPORT_SYMBOL(__arm_smccc_hvc)
> > +
> > +	.macro SMCCC_v1_2 instr
> > +	.cfi_startproc
> > +	\instr #0
> > +	ldr x8, [sp]
> > +	stp x0, x1, [x8, #ARM_SMCCC_V1_2_RES_X0_OFFS]
> > +	stp x2, x3, [x8, #ARM_SMCCC_V1_2_RES_X2_OFFS]
> > +	stp x4, x5, [x8, #ARM_SMCCC_V1_2_RES_X4_OFFS]
> > +	stp x6, x7, [x8, #ARM_SMCCC_V1_2_RES_X6_OFFS]
> > +	ret
> > +	.cfi_endproc
> > +.endm
> > +
> > +SYM_FUNC_START(arm_smccc_v1_2_hvc)
> > +	SMCCC_v1_2 hvc
> > +SYM_FUNC_END(arm_smccc_v1_2_hvc)
> > +EXPORT_SYMBOL(arm_smccc_v1_2_hvc)
> > +
> > +SYM_FUNC_START(arm_smccc_v1_2_smc)
> > +	SMCCC_v1_2 smc
> > +SYM_FUNC_END(arm_smccc_v1_2_smc)
> > +EXPORT_SYMBOL(arm_smccc_v1_2_smc)
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index 62c54234576c..0b8fa285a054 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -186,6 +186,56 @@ struct arm_smccc_res {
> >  	unsigned long a3;
> >  };
> >  
> > +#ifdef CONFIG_ARM64
> > +/* TODO Need to implement for ARM too */
> > +/**
> > + * struct arm_smccc_v1_2_res - Result from SMC/HVC call
> > + * @a0-a7 result values from registers 0 to 7
> > + */
> > +struct arm_smccc_v1_2_res {
> > +	unsigned long a0;
> > +	unsigned long a1;
> > +	unsigned long a2;
> > +	unsigned long a3;
> > +	unsigned long a4;
> > +	unsigned long a5;
> > +	unsigned long a6;
> > +	unsigned long a7;
> > +};
> > +
> > +/**
> > + * arm_smccc_v1_2_hvc() - make HVC calls
> > + * @a0-a7: arguments passed in registers 0 to 7
> > + * @res: result values from registers 0 to 7
> > + *
> > + * This function is used to make HVC calls following SMC Calling Convention
> > + * v1.2 or above. The content of the supplied param are copied to registers
> > + * 0 to 7 prior to the HVC instruction. The return values are updated with
> > + * the content from register 0 to 7 on return from the HVC instruction.
> > + */
> > +asmlinkage
> > +void arm_smccc_v1_2_hvc(unsigned long a0, unsigned long a1, unsigned long a2,
> > +			unsigned long a3, unsigned long a4, unsigned long a5,
> > +			unsigned long a6, unsigned long a7,
> > +			struct arm_smccc_v1_2_res  *res);
> > +
> > +/**
> > + * arm_smccc_v1_2_smc() - make SMC calls
> > + * @a0-a7: arguments passed in registers 0 to 7
> > + * @res: result values from registers 0 to 7
> > + *
> > + * This function is used to make SMC calls following SMC Calling Convention
> > + * v1.2 or above. The content of the supplied param are copied to registers
> > + * 0 to 7 prior to the SMC instruction. The return values are updated with
> > + * the content from register 0 to 7 on return from the SMC instruction.
> > + */
> > +asmlinkage
> > +void arm_smccc_v1_2_smc(unsigned long a0, unsigned long a1, unsigned long a2,
> > +			unsigned long a3, unsigned long a4, unsigned long a5,
> > +			unsigned long a6, unsigned long a7,
> > +			struct arm_smccc_v1_2_res  *res);
> > +#endif
> > +
> >  /**
> >   * struct arm_smccc_quirk - Contains quirk information
> >   * @id: quirk identification
> > -- 
> > 2.25.1
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel