linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Scull <ascull@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kernel-team@android.com, suzuki.poulose@arm.com,
	catalin.marinas@arm.com, james.morse@arm.com,
	linux-arm-kernel@lists.infradead.org,
	Sudeep Holla <sudeep.holla@arm.com>,
	will@kernel.org, kvmarm@lists.cs.columbia.edu,
	julien.thierry.kdev@gmail.com
Subject: Re: [PATCH v3 14/18] smccc: Cast arguments to unsigned long
Date: Tue, 8 Sep 2020 11:58:28 +0100	[thread overview]
Message-ID: <20200908105828.GE3268721@google.com> (raw)
In-Reply-To: <87r1rdlnj6.wl-maz@kernel.org>

On Mon, Sep 07, 2020 at 02:33:17PM +0100, Marc Zyngier wrote:
> On Thu, 03 Sep 2020 14:53:03 +0100,
> Andrew Scull <ascull@google.com> wrote:
> > 
> > To avoid warning about implicit casting, make the casting explicit. This
> > allows, for example, pointers to be used as arguments as are used in the
> > KVM hyp interface.
> > 
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  include/linux/arm-smccc.h | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index 15c706fb0a37..3bb109a35554 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -260,7 +260,7 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
> >  	typeof(a1) __a1 = a1;						\
> >  	struct arm_smccc_res   *___res = res;				\
> >  	register unsigned long r0 asm("r0") = (u32)a0;			\
> > -	register unsigned long r1 asm("r1") = __a1;			\
> > +	register unsigned long r1 asm("r1") = (unsigned long)__a1;	\
> 
> Given the pain we go through to extract the type of each argument, it
> seems odd to end-up with casts everywhere. I'd rather keep the type
> system alive by having:
> 
> 	register typeof(a1) r1 asm("r1") = __a1;
> 
> Is there any reason why this doesn't work?

There is. r1 is used both for the argument and the result so if
typeof(a1) cannot be implicitly assigned to unsigned long you'll get a
warning when populating arm_smccc_res.

The approach in this patch made the removal of types explicit with a
case rather then implicit in the assignment.

I had a go at breaking the link between the inputs and outputs. I've
replaced this patch with the following for the time being.


commit 16fd4f7d3c12e94f96f77c43491fbfe80417f27b
Author: Andrew Scull <ascull@google.com>
Date:   Wed Jun 3 10:28:23 2020 +0100

    smccc: Use separate variables for args and results
    
    Using the same register-bound variable for both arguments and results
    means these values share a type. That type must allow the arguments to
    be assigned to it and must also be assignable to the unsigned long
    fields of struct arm_smccc_res.
    
    This restriction on types causes compiler warnings when the argument
    cannot be implicitly assigned to an unsigned long, for example the
    pointers that are used in the KVM hyp interface.
    
    By separating the arguments and results into their own variables, the
    type constraint is lifted allowing the arguments to avoid the need for
    any type conversion.
    
    Cc: Sudeep Holla <sudeep.holla@arm.com>
    Signed-off-by: Andrew Scull <ascull@google.com>

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 15c706fb0a37..4fe089ff2704 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -227,87 +227,67 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 #define __count_args(...)						\
 	___count_args(__VA_ARGS__, 7, 6, 5, 4, 3, 2, 1, 0)
 
-#define __constraint_write_0						\
-	"+r" (r0), "=&r" (r1), "=&r" (r2), "=&r" (r3)
-#define __constraint_write_1						\
-	"+r" (r0), "+r" (r1), "=&r" (r2), "=&r" (r3)
-#define __constraint_write_2						\
-	"+r" (r0), "+r" (r1), "+r" (r2), "=&r" (r3)
-#define __constraint_write_3						\
-	"+r" (r0), "+r" (r1), "+r" (r2), "+r" (r3)
-#define __constraint_write_4	__constraint_write_3
-#define __constraint_write_5	__constraint_write_4
-#define __constraint_write_6	__constraint_write_5
-#define __constraint_write_7	__constraint_write_6
-
-#define __constraint_read_0
-#define __constraint_read_1
-#define __constraint_read_2
-#define __constraint_read_3
-#define __constraint_read_4	"r" (r4)
-#define __constraint_read_5	__constraint_read_4, "r" (r5)
-#define __constraint_read_6	__constraint_read_5, "r" (r6)
-#define __constraint_read_7	__constraint_read_6, "r" (r7)
+#define __constraint_read_0	"r" (arg0)
+#define __constraint_read_1	__constraint_read_0, "r" (arg1)
+#define __constraint_read_2	__constraint_read_1, "r" (arg2)
+#define __constraint_read_3	__constraint_read_2, "r" (arg3)
+#define __constraint_read_4	__constraint_read_3, "r" (arg4)
+#define __constraint_read_5	__constraint_read_4, "r" (arg5)
+#define __constraint_read_6	__constraint_read_5, "r" (arg6)
+#define __constraint_read_7	__constraint_read_6, "r" (arg7)
 
 #define __declare_arg_0(a0, res)					\
 	struct arm_smccc_res   *___res = res;				\
-	register unsigned long r0 asm("r0") = (u32)a0;			\
-	register unsigned long r1 asm("r1");				\
-	register unsigned long r2 asm("r2");				\
-	register unsigned long r3 asm("r3")
+	register unsigned long arg0 asm("r0") = (u32)a0
 
 #define __declare_arg_1(a0, a1, res)					\
 	typeof(a1) __a1 = a1;						\
 	struct arm_smccc_res   *___res = res;				\
-	register unsigned long r0 asm("r0") = (u32)a0;			\
-	register unsigned long r1 asm("r1") = __a1;			\
-	register unsigned long r2 asm("r2");				\
-	register unsigned long r3 asm("r3")
+	register unsigned long arg0 asm("r0") = (u32)a0;			\
+	register typeof(a1) arg1 asm("r1") = __a1
 
 #define __declare_arg_2(a0, a1, a2, res)				\
 	typeof(a1) __a1 = a1;						\
 	typeof(a2) __a2 = a2;						\
 	struct arm_smccc_res   *___res = res;				\
-	register unsigned long r0 asm("r0") = (u32)a0;			\
-	register unsigned long r1 asm("r1") = __a1;			\
-	register unsigned long r2 asm("r2") = __a2;			\
-	register unsigned long r3 asm("r3")
+	register unsigned long arg0 asm("r0") = (u32)a0;			\
+	register typeof(a1) arg1 asm("r1") = __a1;			\
+	register typeof(a2) arg2 asm("r2") = __a2
 
 #define __declare_arg_3(a0, a1, a2, a3, res)				\
 	typeof(a1) __a1 = a1;						\
 	typeof(a2) __a2 = a2;						\
 	typeof(a3) __a3 = a3;						\
 	struct arm_smccc_res   *___res = res;				\
-	register unsigned long r0 asm("r0") = (u32)a0;			\
-	register unsigned long r1 asm("r1") = __a1;			\
-	register unsigned long r2 asm("r2") = __a2;			\
-	register unsigned long r3 asm("r3") = __a3
+	register unsigned long arg0 asm("r0") = (u32)a0;			\
+	register typeof(a1) arg1 asm("r1") = __a1;			\
+	register typeof(a2) arg2 asm("r2") = __a2;			\
+	register typeof(a3) arg3 asm("r3") = __a3
 
 #define __declare_arg_4(a0, a1, a2, a3, a4, res)			\
 	typeof(a4) __a4 = a4;						\
 	__declare_arg_3(a0, a1, a2, a3, res);				\
-	register unsigned long r4 asm("r4") = __a4
+	register typeof(a4) arg4 asm("r4") = __a4
 
 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)			\
 	typeof(a5) __a5 = a5;						\
 	__declare_arg_4(a0, a1, a2, a3, a4, res);			\
-	register unsigned long r5 asm("r5") = __a5
+	register typeof(a5) arg5 asm("r5") = __a5
 
 #define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res)		\
 	typeof(a6) __a6 = a6;						\
 	__declare_arg_5(a0, a1, a2, a3, a4, a5, res);			\
-	register unsigned long r6 asm("r6") = __a6
+	register typeof(a6) arg6 asm("r6") = __a6
 
 #define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res)		\
 	typeof(a7) __a7 = a7;						\
 	__declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);		\
-	register unsigned long r7 asm("r7") = __a7
+	register typeof(a7) arg7 asm("r7") = __a7
 
 #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
 #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
 
 #define ___constraints(count)						\
-	: __constraint_write_ ## count					\
 	: __constraint_read_ ## count					\
 	: "memory"
 #define __constraints(count)	___constraints(count)
@@ -319,8 +299,13 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
  */
 #define __arm_smccc_1_1(inst, ...)					\
 	do {								\
+		register unsigned long r0 asm("r0");			\
+		register unsigned long r1 asm("r1");			\
+		register unsigned long r2 asm("r2");			\
+		register unsigned long r3 asm("r3"); 			\
 		__declare_args(__count_args(__VA_ARGS__), __VA_ARGS__);	\
-		asm volatile(inst "\n"					\
+		asm volatile(inst "\n" :				\
+			     "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3)	\
 			     __constraints(__count_args(__VA_ARGS__)));	\
 		if (___res)						\
 			*___res = (typeof(*___res)){r0, r1, r2, r3};	\
@@ -366,7 +351,7 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 #define __fail_smccc_1_1(...)						\
 	do {								\
 		__declare_args(__count_args(__VA_ARGS__), __VA_ARGS__);	\
-		asm ("" __constraints(__count_args(__VA_ARGS__)));	\
+		asm ("" : __constraints(__count_args(__VA_ARGS__)));	\
 		if (___res)						\
 			___res->a0 = SMCCC_RET_NOT_SUPPORTED;		\
 	} while (0)

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

  reply	other threads:[~2020-09-08 11:00 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-03 13:52 [PATCH v3 00/18] Introduce separate nVHE hyp context Andrew Scull
2020-09-03 13:52 ` [PATCH v3 01/18] KVM: arm64: Remove __activate_vm wrapper Andrew Scull
2020-09-03 13:52 ` [PATCH v3 02/18] KVM: arm64: Remove hyp_panic arguments Andrew Scull
2020-09-07 10:21   ` Marc Zyngier
2020-09-03 13:52 ` [PATCH v3 03/18] KVM: arm64: Remove kvm_host_data_t typedef Andrew Scull
2020-09-03 13:52 ` [PATCH v3 04/18] KVM: arm64: Restrict symbol aliasing to outside nVHE Andrew Scull
2020-09-07 10:38   ` Marc Zyngier
2020-09-08 10:13     ` Andrew Scull
2020-09-03 13:52 ` [PATCH v3 05/18] KVM: arm64: Save chosen hyp vector to a percpu variable Andrew Scull
2020-09-03 13:52 ` [PATCH v3 06/18] KVM: arm64: nVHE: Use separate vector for the host Andrew Scull
2020-09-07 11:38   ` Marc Zyngier
2020-09-08 10:29     ` Andrew Scull
2020-09-03 13:52 ` [PATCH v3 07/18] KVM: arm64: nVHE: Don't consume host SErrors with ESB Andrew Scull
2020-09-07 11:46   ` Marc Zyngier
2020-09-03 13:52 ` [PATCH v3 08/18] KVM: arm64: Introduce hyp context Andrew Scull
2020-09-07 13:29   ` Marc Zyngier
2020-09-08 10:52     ` Andrew Scull
2020-09-03 13:52 ` [PATCH v3 09/18] KVM: arm64: Update context references from host to hyp Andrew Scull
2020-09-03 13:52 ` [PATCH v3 10/18] KVM: arm64: Restore hyp when panicking in guest context Andrew Scull
2020-09-03 13:53 ` [PATCH v3 11/18] KVM: arm64: Share context save and restore macros Andrew Scull
2020-09-03 13:53 ` [PATCH v3 12/18] KVM: arm64: nVHE: Switch to hyp context for EL2 Andrew Scull
2020-09-07 13:02   ` Marc Zyngier
2020-09-08 10:42     ` Andrew Scull
2020-09-03 13:53 ` [PATCH v3 13/18] KVM: arm64: nVHE: Handle hyp panics Andrew Scull
2020-09-07 13:24   ` Marc Zyngier
2020-09-03 13:53 ` [PATCH v3 14/18] smccc: Cast arguments to unsigned long Andrew Scull
2020-09-07 13:33   ` Marc Zyngier
2020-09-08 10:58     ` Andrew Scull [this message]
2020-09-03 13:53 ` [PATCH v3 15/18] KVM: arm64: nVHE: Pass pointers consistently to hyp-init Andrew Scull
2020-09-03 13:53 ` [PATCH v3 16/18] KVM: arm64: nVHE: Migrate hyp interface to SMCCC Andrew Scull
2020-09-07 13:47   ` Marc Zyngier
2020-09-07 14:20   ` Marc Zyngier
2020-09-08 11:02     ` Andrew Scull
2020-09-09  8:30       ` Andrew Scull
2020-09-03 13:53 ` [PATCH v3 17/18] KVM: arm64: nVHE: Migrate hyp-init " Andrew Scull
2020-09-03 13:53 ` [PATCH v3 18/18] KVM: arm64: nVHE: Fix pointers during SMCCC convertion Andrew Scull

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200908105828.GE3268721@google.com \
    --to=ascull@google.com \
    --cc=catalin.marinas@arm.com \
    --cc=james.morse@arm.com \
    --cc=julien.thierry.kdev@gmail.com \
    --cc=kernel-team@android.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=maz@kernel.org \
    --cc=sudeep.holla@arm.com \
    --cc=suzuki.poulose@arm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).