| Message ID | 20180824165820.32620-2-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: SMCCC fixup and improvement | expand |
Hi Julien, Marc On 24.08.18 19:58, Julien Grall wrote: > From: Marc Zyngier <marc.zyngier@arm.com> > > An unfortunate consequence of having a strong typing for the input > values to the SMC call is that it also affects the type of the > return values, limiting r0 to 32 bits and r{1,2,3} to whatever > was passed as an input. > > Let's turn everything into "unsigned long", which satisfies the > requirements of both architectures, and allows for the full > range of return values. Maybe it better to use register_t then? By definition register_t has the same size as a CPU register and SMC uses CPU registers to pass parameters/return values. > Reported-by: Stefano Stabellini <stefanos@xilinx.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/include/asm-arm/smccc.h | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h > index 74c13f8419..a31d67a1de 100644 > --- a/xen/include/asm-arm/smccc.h > +++ b/xen/include/asm-arm/smccc.h > @@ -119,35 +119,35 @@ struct arm_smccc_res { > > #define __declare_arg_0(a0, res) \ > struct arm_smccc_res *___res = res; \ > - register uin32_t r0 asm("r0") = a0; \ > + register unsigned long r0 asm("r0") = (uint32_t)a0;\ Do you really want to silently drop upper 32 bits of the argument? I know, that SMCCC states that function id is a 32-bit value, but I don't think that it is a good place to enforce this behavior. I think it is better to allow user to shoot in his leg in this case. > register unsigned long r1 asm("r1"); \ > register unsigned long r2 asm("r2"); \ > register unsigned long r3 asm("r3") > > #define __declare_arg_1(a0, a1, res) \ > struct arm_smccc_res *___res = res; \ > - register uint32_t r0 asm("r0") = a0; \ > - register typeof(a1) r1 asm("r1") = a1; \ > + register unsigned long r0 asm("r0") = (uint32_t)a0;\ > + register unsigned long r1 asm("r1") = a1; \ > register unsigned long r2 asm("r2"); \ > register unsigned long r3 asm("r3") > > #define __declare_arg_2(a0, a1, a2, res) \ > struct arm_smccc_res *___res = res; \ > - register u32 r0 asm("r0") = a0; \ > - register typeof(a1) r1 asm("r1") = a1; \ > - register typeof(a2) r2 asm("r2") = a2; \ > + register unsigned long r0 asm("r0") = (uint32_t)a0;\ > + register unsigned long r1 asm("r1") = a1; \ > + register unsigned long r2 asm("r2") = a2; \ > register unsigned long r3 asm("r3") > > #define __declare_arg_3(a0, a1, a2, a3, res) \ > struct arm_smccc_res *___res = res; \ > - register u32 r0 asm("r0") = a0; \ > - register typeof(a1) r1 asm("r1") = a1; \ > - register typeof(a2) r2 asm("r2") = a2; \ > - register typeof(a3) r3 asm("r3") = a3 > + register unsigned long r0 asm("r0") = (uint32_t)a0;\ > + register unsigned long r1 asm("r1") = a1; \ > + register unsigned long r2 asm("r2") = a2; \ > + register unsigned long r3 asm("r3") = a3 > > #define __declare_arg_4(a0, a1, a2, a3, a4, res) \ > __declare_arg_3(a0, a1, a2, a3, res); \ > - register typeof(a4) r4 asm("r4") = a4 > + register unsigned long r4 asm("r4") = a4 > > #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \ > __declare_arg_4(a0, a1, a2, a3, a4, res); \ >
On 27/08/2018 15:03, Volodymyr Babchuk wrote: > Hi Julien, Marc Hi, > On 24.08.18 19:58, Julien Grall wrote: >> From: Marc Zyngier <marc.zyngier@arm.com> >> >> An unfortunate consequence of having a strong typing for the input >> values to the SMC call is that it also affects the type of the >> return values, limiting r0 to 32 bits and r{1,2,3} to whatever >> was passed as an input. > >> Let's turn everything into "unsigned long", which satisfies the >> requirements of both architectures, and allows for the full >> range of return values. > Maybe it better to use register_t then? By definition register_t has the > same size as a CPU register and SMC uses CPU registers to pass > parameters/return values. The code is based on Linux series (posted last Friday). I don't much want to diverge too much from it. So unsigned "unsigned long" here is ok. > >> Reported-by: Stefano Stabellini <stefanos@xilinx.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/include/asm-arm/smccc.h | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h >> index 74c13f8419..a31d67a1de 100644 >> --- a/xen/include/asm-arm/smccc.h >> +++ b/xen/include/asm-arm/smccc.h >> @@ -119,35 +119,35 @@ struct arm_smccc_res { >> #define __declare_arg_0(a0, res) \ >> struct arm_smccc_res *___res = res; \ >> - register uin32_t r0 asm("r0") = a0; \ >> + register unsigned long r0 asm("r0") = (uint32_t)a0;\ > Do you really want to silently drop upper 32 bits of the argument? > I know, that SMCCC states that function id is a 32-bit value, > but I don't think that it is a good place to enforce this > behavior. > I think it is better to allow user to shoot in his leg in this case. I don't see any issue with casting here. This is what the SMCCC request for x0 and after all you silently drop upper 32-bit when using a static inline function... Cheers,
Julien, On 27.08.18 18:23, Julien Grall wrote: > > > On 27/08/2018 15:03, Volodymyr Babchuk wrote: >> Hi Julien, Marc > > Hi, > >> On 24.08.18 19:58, Julien Grall wrote: >>> From: Marc Zyngier <marc.zyngier@arm.com> >>> >>> An unfortunate consequence of having a strong typing for the input >>> values to the SMC call is that it also affects the type of the >>> return values, limiting r0 to 32 bits and r{1,2,3} to whatever >>> was passed as an input. > >>> Let's turn everything into "unsigned long", which satisfies the >>> requirements of both architectures, and allows for the full >>> range of return values. >> Maybe it better to use register_t then? By definition register_t has >> the same size as a CPU register and SMC uses CPU registers to pass >> parameters/return values. > > The code is based on Linux series (posted last Friday). I don't much > want to diverge too much from it. So unsigned "unsigned long" here is ok. > >> >>> Reported-by: Stefano Stabellini <stefanos@xilinx.com> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> --- >>> xen/include/asm-arm/smccc.h | 22 +++++++++++----------- >>> 1 file changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h >>> index 74c13f8419..a31d67a1de 100644 >>> --- a/xen/include/asm-arm/smccc.h >>> +++ b/xen/include/asm-arm/smccc.h >>> @@ -119,35 +119,35 @@ struct arm_smccc_res { >>> #define __declare_arg_0(a0, res) \ >>> struct arm_smccc_res *___res = res; \ >>> - register uin32_t r0 asm("r0") = a0; \ >>> + register unsigned long r0 asm("r0") = (uint32_t)a0;\ >> Do you really want to silently drop upper 32 bits of the argument? >> I know, that SMCCC states that function id is a 32-bit value, >> but I don't think that it is a good place to enforce this >> behavior. >> I think it is better to allow user to shoot in his leg in this case. > > I don't see any issue with casting here. This is what the SMCCC request > for x0 and after all you silently drop upper 32-bit when using a static > inline function... > Yes, you are right. Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h index 74c13f8419..a31d67a1de 100644 --- a/xen/include/asm-arm/smccc.h +++ b/xen/include/asm-arm/smccc.h @@ -119,35 +119,35 @@ struct arm_smccc_res { #define __declare_arg_0(a0, res) \ struct arm_smccc_res *___res = res; \ - register uin32_t r0 asm("r0") = a0; \ + register unsigned long r0 asm("r0") = (uint32_t)a0;\ register unsigned long r1 asm("r1"); \ register unsigned long r2 asm("r2"); \ register unsigned long r3 asm("r3") #define __declare_arg_1(a0, a1, res) \ struct arm_smccc_res *___res = res; \ - register uint32_t r0 asm("r0") = a0; \ - register typeof(a1) r1 asm("r1") = a1; \ + register unsigned long r0 asm("r0") = (uint32_t)a0;\ + register unsigned long r1 asm("r1") = a1; \ register unsigned long r2 asm("r2"); \ register unsigned long r3 asm("r3") #define __declare_arg_2(a0, a1, a2, res) \ struct arm_smccc_res *___res = res; \ - register u32 r0 asm("r0") = a0; \ - register typeof(a1) r1 asm("r1") = a1; \ - register typeof(a2) r2 asm("r2") = a2; \ + register unsigned long r0 asm("r0") = (uint32_t)a0;\ + register unsigned long r1 asm("r1") = a1; \ + register unsigned long r2 asm("r2") = a2; \ register unsigned long r3 asm("r3") #define __declare_arg_3(a0, a1, a2, a3, res) \ struct arm_smccc_res *___res = res; \ - register u32 r0 asm("r0") = a0; \ - register typeof(a1) r1 asm("r1") = a1; \ - register typeof(a2) r2 asm("r2") = a2; \ - register typeof(a3) r3 asm("r3") = a3 + register unsigned long r0 asm("r0") = (uint32_t)a0;\ + register unsigned long r1 asm("r1") = a1; \ + register unsigned long r2 asm("r2") = a2; \ + register unsigned long r3 asm("r3") = a3 #define __declare_arg_4(a0, a1, a2, a3, a4, res) \ __declare_arg_3(a0, a1, a2, a3, res); \ - register typeof(a4) r4 asm("r4") = a4 + register unsigned long r4 asm("r4") = a4 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \ __declare_arg_4(a0, a1, a2, a3, a4, res); \