| Message ID | 123c27ed53ab50ca6f605a96002dcc7e1b7d3e6e.1609931750.git.rm.skakun@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] arm/optee: Use only least 32 bits for SMC type arg according to SMCCC | expand |
Hi Roman, Thank you for the contribution. Roman Skakun writes: > This patch added additional sanity and increases an understanding for > getting proper value from the first argument for SMC call on aarch64 > according to SMCC Convention. > > [0] ARM DEN0028B, page 12 > > Signed-off-by: Roman Skakun <roman_skakun@epam.com> Acked-by: Volodymyr Babchyk <volodymyr_babchuk@epam.com> > --- > xen/arch/arm/tee/optee.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c > index ee85359742..87060b52b8 100644 > --- a/xen/arch/arm/tee/optee.c > +++ b/xen/arch/arm/tee/optee.c > @@ -1643,7 +1643,8 @@ static bool optee_handle_call(struct cpu_user_regs *regs) > if ( !ctx ) > return false; > > - switch ( get_user_reg(regs, 0) ) > + /* Only least 32 bits are significant (see ARM DEN 0028B, page 12) */ > + switch ( (uint32_t)get_user_reg(regs, 0) ) > { > case OPTEE_SMC_CALLS_COUNT: > set_user_reg(regs, 0, OPTEE_MEDIATOR_SMC_COUNT);
Hi Roman, On 06/01/2021 11:26, Roman Skakun wrote: > This patch added additional sanity and increases an understanding for > getting proper value from the first argument for SMC call on aarch64 > according to SMCC Convention. I would suggest the following commit message: "xen/arm: optee: The function identifier is always 32-bit Per the SMCCC specification (see section 3.1 in ARM DEN 0028D), the function identifier is only stored in the least significant 32-bits. The most significant 32-bits should be ignored. The function optee_handle_call() is now updated to ignore the most significant 32-bits. " Note that I used the version D rather than B because the latter has buggy wording (it seems to suggest that the least significants bits should be ignored). Furthermore, I checked vsmc.c (the layer that dispatch the SMC) and it was already handled properly thanks to commit 7f4217cc6057 "xen/arm: vsmc: The function identifier is always 32-bit". > [0] ARM DEN0028B, page 12 > > Signed-off-by: Roman Skakun <roman_skakun@epam.com> > --- > xen/arch/arm/tee/optee.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c > index ee85359742..87060b52b8 100644 > --- a/xen/arch/arm/tee/optee.c > +++ b/xen/arch/arm/tee/optee.c > @@ -1643,7 +1643,8 @@ static bool optee_handle_call(struct cpu_user_regs *regs) > if ( !ctx ) > return false; > > - switch ( get_user_reg(regs, 0) ) > + /* Only least 32 bits are significant (see ARM DEN 0028B, page 12) */ I would suggest: /* The function identifier is always stored in the least significant 32-bit (see section ARM DEN 0028D) */ I can update it while committing, if both Volodymyr and you are happy with changes. Cheers, > + switch ( (uint32_t)get_user_reg(regs, 0) ) > { > case OPTEE_SMC_CALLS_COUNT: > set_user_reg(regs, 0, OPTEE_MEDIATOR_SMC_COUNT); >
Hi Julien, Julien Grall writes: > Hi Roman, > > On 06/01/2021 11:26, Roman Skakun wrote: >> This patch added additional sanity and increases an understanding for >> getting proper value from the first argument for SMC call on aarch64 >> according to SMCC Convention. > > I would suggest the following commit message: > > "xen/arm: optee: The function identifier is always 32-bit > > Per the SMCCC specification (see section 3.1 in ARM DEN 0028D), the > function identifier is only stored in the least significant > 32-bits. The most significant 32-bits should be ignored. > > The function optee_handle_call() is now updated to ignore the most > significant 32-bits. > > " > > Note that I used the version D rather than B because the latter has > buggy wording (it seems to suggest that the least significants bits > should be ignored). > > Furthermore, I checked vsmc.c (the layer that dispatch the SMC) and it > was already handled properly thanks to commit 7f4217cc6057 "xen/arm: > vsmc: The function identifier is always 32-bit". > >> [0] ARM DEN0028B, page 12 >> Signed-off-by: Roman Skakun <roman_skakun@epam.com> >> --- >> xen/arch/arm/tee/optee.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c >> index ee85359742..87060b52b8 100644 >> --- a/xen/arch/arm/tee/optee.c >> +++ b/xen/arch/arm/tee/optee.c >> @@ -1643,7 +1643,8 @@ static bool optee_handle_call(struct cpu_user_regs *regs) >> if ( !ctx ) >> return false; >> - switch ( get_user_reg(regs, 0) ) >> + /* Only least 32 bits are significant (see ARM DEN 0028B, page 12) */ > > I would suggest: > > /* The function identifier is always stored in the least significant > 32-bit (see section ARM DEN 0028D) */ > > I can update it while committing, if both Volodymyr and you are happy > with changes. I'm totally fine with the proposed changes. Thank you for tidying this up.
On 06/01/2021 23:22, Volodymyr Babchuk wrote: > > Hi Julien, > > Julien Grall writes: > >> Hi Roman, >> >> On 06/01/2021 11:26, Roman Skakun wrote: >>> This patch added additional sanity and increases an understanding for >>> getting proper value from the first argument for SMC call on aarch64 >>> according to SMCC Convention. >> >> I would suggest the following commit message: >> >> "xen/arm: optee: The function identifier is always 32-bit >> >> Per the SMCCC specification (see section 3.1 in ARM DEN 0028D), the >> function identifier is only stored in the least significant >> 32-bits. The most significant 32-bits should be ignored. >> >> The function optee_handle_call() is now updated to ignore the most >> significant 32-bits. >> >> " >> >> Note that I used the version D rather than B because the latter has >> buggy wording (it seems to suggest that the least significants bits >> should be ignored). >> >> Furthermore, I checked vsmc.c (the layer that dispatch the SMC) and it >> was already handled properly thanks to commit 7f4217cc6057 "xen/arm: >> vsmc: The function identifier is always 32-bit". >> >>> [0] ARM DEN0028B, page 12 >>> Signed-off-by: Roman Skakun <roman_skakun@epam.com> >>> --- >>> xen/arch/arm/tee/optee.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c >>> index ee85359742..87060b52b8 100644 >>> --- a/xen/arch/arm/tee/optee.c >>> +++ b/xen/arch/arm/tee/optee.c >>> @@ -1643,7 +1643,8 @@ static bool optee_handle_call(struct cpu_user_regs *regs) >>> if ( !ctx ) >>> return false; >>> - switch ( get_user_reg(regs, 0) ) >>> + /* Only least 32 bits are significant (see ARM DEN 0028B, page 12) */ >> >> I would suggest: >> >> /* The function identifier is always stored in the least significant >> 32-bit (see section ARM DEN 0028D) */ >> >> I can update it while committing, if both Volodymyr and you are happy >> with changes. > > I'm totally fine with the proposed changes. Thank you for tidying this up. Thanks! I have updated the patch and committed it. Cheers, >
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index ee85359742..87060b52b8 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -1643,7 +1643,8 @@ static bool optee_handle_call(struct cpu_user_regs *regs) if ( !ctx ) return false; - switch ( get_user_reg(regs, 0) ) + /* Only least 32 bits are significant (see ARM DEN 0028B, page 12) */ + switch ( (uint32_t)get_user_reg(regs, 0) ) { case OPTEE_SMC_CALLS_COUNT: set_user_reg(regs, 0, OPTEE_MEDIATOR_SMC_COUNT);
This patch added additional sanity and increases an understanding for getting proper value from the first argument for SMC call on aarch64 according to SMCC Convention. [0] ARM DEN0028B, page 12 Signed-off-by: Roman Skakun <roman_skakun@epam.com> --- xen/arch/arm/tee/optee.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)