xen/arm: optee: Fix arm_smccc_smc's a0 for OPTEE_SMC_DISABLE_SHM_CACHE

Commit Message

Oleksandr Tyshchenko Sept. 27, 2021, 1:54 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

Fix a possible copy-paste error in arm_smccc_smc's first argument (a0)
for OPTEE_SMC_DISABLE_SHM_CACHE case.

This error causes Linux > v5.14-rc5 (b5c10dd04b7418793517e3286cde5c04759a86de
optee: Clear stale cache entries during initialization) to stuck
repeatedly issuing OPTEE_SMC_DISABLE_SHM_CACHE call and waiting for
the result to be OPTEE_SMC_RETURN_ENOTAVAIL which will never happen.

Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
---
I wonder whether this patch wants backporting to the old versions
since OPTEE support went in.
---
 xen/arch/arm/tee/optee.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bertrand Marquis Sept. 27, 2021, 2:36 p.m. UTC | #1

Hi Oleksandr,

> On 27 Sep 2021, at 14:54, Oleksandr Tyshchenko <[e-mail]> wrote:
> 
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> Fix a possible copy-paste error in arm_smccc_smc's first argument (a0)
> for OPTEE_SMC_DISABLE_SHM_CACHE case.
> 
> This error causes Linux > v5.14-rc5 (b5c10dd04b7418793517e3286cde5c04759a86de
> optee: Clear stale cache entries during initialization) to stuck
> repeatedly issuing OPTEE_SMC_DISABLE_SHM_CACHE call and waiting for
> the result to be OPTEE_SMC_RETURN_ENOTAVAIL which will never happen.
> 
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>

Sound like a reasonable change :-)

Reviewed-by: Bertrand Marquis <[e-mail]>

Cheers
Bertrand

> ---
> I wonder whether this patch wants backporting to the old versions
> since OPTEE support went in.
> ---
> xen/arch/arm/tee/optee.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 3453615..6df0d44 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -1692,7 +1692,7 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
>         return true;
> 
>     case OPTEE_SMC_DISABLE_SHM_CACHE:
> -        arm_smccc_smc(OPTEE_SMC_ENABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
> +        arm_smccc_smc(OPTEE_SMC_DISABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
>                       OPTEE_CLIENT_ID(current->domain), &resp);
>         set_user_reg(regs, 0, resp.a0);
>         if ( resp.a0 == OPTEE_SMC_RETURN_OK ) {
> -- 
> 2.7.4
> 
>

Volodymyr Babchuk Sept. 27, 2021, 11:12 p.m. UTC | #2

Hi Oleksandr,

Oleksandr Tyshchenko <[e-mail]> writes:

> From: Oleksandr Tyshchenko <[e-mail]>
>
> Fix a possible copy-paste error in arm_smccc_smc's first argument (a0)
> for OPTEE_SMC_DISABLE_SHM_CACHE case.
>
> This error causes Linux > v5.14-rc5 (b5c10dd04b7418793517e3286cde5c04759a86de
> optee: Clear stale cache entries during initialization) to stuck
> repeatedly issuing OPTEE_SMC_DISABLE_SHM_CACHE call and waiting for
> the result to be OPTEE_SMC_RETURN_ENOTAVAIL which will never happen.
>
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>

Reviewed-by: Volodymyr Babchuk <[e-mail]>

> ---
> I wonder whether this patch wants backporting to the old versions
> since OPTEE support went in.
> ---
>  xen/arch/arm/tee/optee.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 3453615..6df0d44 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -1692,7 +1692,7 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
>          return true;
>  
>      case OPTEE_SMC_DISABLE_SHM_CACHE:
> -        arm_smccc_smc(OPTEE_SMC_ENABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
> +        arm_smccc_smc(OPTEE_SMC_DISABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
>                        OPTEE_CLIENT_ID(current->domain), &resp);
>          set_user_reg(regs, 0, resp.a0);
>          if ( resp.a0 == OPTEE_SMC_RETURN_OK ) {

Stefano Stabellini Sept. 28, 2021, 4:52 a.m. UTC | #3

On Mon, 27 Sep 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> Fix a possible copy-paste error in arm_smccc_smc's first argument (a0)
> for OPTEE_SMC_DISABLE_SHM_CACHE case.
> 
> This error causes Linux > v5.14-rc5 (b5c10dd04b7418793517e3286cde5c04759a86de
> optee: Clear stale cache entries during initialization) to stuck
> repeatedly issuing OPTEE_SMC_DISABLE_SHM_CACHE call and waiting for
> the result to be OPTEE_SMC_RETURN_ENOTAVAIL which will never happen.
> 
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>

Acked-by: Stefano Stabellini <[e-mail]>

I added Fixes: and Backport: tags to the commit


> ---
> I wonder whether this patch wants backporting to the old versions
> since OPTEE support went in.
> ---
>  xen/arch/arm/tee/optee.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 3453615..6df0d44 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -1692,7 +1692,7 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
>          return true;
>  
>      case OPTEE_SMC_DISABLE_SHM_CACHE:
> -        arm_smccc_smc(OPTEE_SMC_ENABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
> +        arm_smccc_smc(OPTEE_SMC_DISABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
>                        OPTEE_CLIENT_ID(current->domain), &resp);
>          set_user_reg(regs, 0, resp.a0);
>          if ( resp.a0 == OPTEE_SMC_RETURN_OK ) {
> -- 
> 2.7.4
>

Julien Grall Oct. 6, 2021, 6:46 p.m. UTC | #4

Hi Stefano,

On 28/09/2021 06:52, Stefano Stabellini wrote:
> On Mon, 27 Sep 2021, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <[e-mail]>
>>
>> Fix a possible copy-paste error in arm_smccc_smc's first argument (a0)
>> for OPTEE_SMC_DISABLE_SHM_CACHE case.
>>
>> This error causes Linux > v5.14-rc5 (b5c10dd04b7418793517e3286cde5c04759a86de
>> optee: Clear stale cache entries during initialization) to stuck
>> repeatedly issuing OPTEE_SMC_DISABLE_SHM_CACHE call and waiting for
>> the result to be OPTEE_SMC_RETURN_ENOTAVAIL which will never happen.
>>
>> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> 
> Acked-by: Stefano Stabellini <[e-mail]>
> 
> I added Fixes: and Backport: tags to the commit
Per SUPPORT.MD, OP-TEE is still a technical preview. So I would argue 
that we should not do any backport because the feature itself is not 
officially considered supported.

That said, what's missing to make the feature officially supported?

Cheers,

Stefano Stabellini Oct. 6, 2021, 10:42 p.m. UTC | #5

On Wed, 6 Oct 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 28/09/2021 06:52, Stefano Stabellini wrote:
> > On Mon, 27 Sep 2021, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <[e-mail]>
> > > 
> > > Fix a possible copy-paste error in arm_smccc_smc's first argument (a0)
> > > for OPTEE_SMC_DISABLE_SHM_CACHE case.
> > > 
> > > This error causes Linux > v5.14-rc5
> > > (b5c10dd04b7418793517e3286cde5c04759a86de
> > > optee: Clear stale cache entries during initialization) to stuck
> > > repeatedly issuing OPTEE_SMC_DISABLE_SHM_CACHE call and waiting for
> > > the result to be OPTEE_SMC_RETURN_ENOTAVAIL which will never happen.
> > > 
> > > Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> > 
> > Acked-by: Stefano Stabellini <[e-mail]>
> > 
> > I added Fixes: and Backport: tags to the commit
> Per SUPPORT.MD, OP-TEE is still a technical preview. So I would argue that we
> should not do any backport because the feature itself is not officially
> considered supported.

Good point!


> That said, what's missing to make the feature officially supported?

If Oleksandr is also happy to make OP-TEE support in Xen "Supported" in
SUPPORT.md I'd be happy with that too. Specifically I suggest to change
it to:

Status: Supported, not security supported

Security Support is a bit of a heavy process and I am thinking that
"Supported, not security supported" would be an excellent next step.

Oleksandr Tyshchenko Oct. 7, 2021, 5:57 p.m. UTC | #6

On 07.10.21 01:42, Stefano Stabellini wrote:

Hi Stefano, Julien.

> On Wed, 6 Oct 2021, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 28/09/2021 06:52, Stefano Stabellini wrote:
>>> On Mon, 27 Sep 2021, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <[e-mail]>
>>>>
>>>> Fix a possible copy-paste error in arm_smccc_smc's first argument (a0)
>>>> for OPTEE_SMC_DISABLE_SHM_CACHE case.
>>>>
>>>> This error causes Linux > v5.14-rc5
>>>> (b5c10dd04b7418793517e3286cde5c04759a86de
>>>> optee: Clear stale cache entries during initialization) to stuck
>>>> repeatedly issuing OPTEE_SMC_DISABLE_SHM_CACHE call and waiting for
>>>> the result to be OPTEE_SMC_RETURN_ENOTAVAIL which will never happen.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
>>> Acked-by: Stefano Stabellini <[e-mail]>
>>>
>>> I added Fixes: and Backport: tags to the commit
>> Per SUPPORT.MD, OP-TEE is still a technical preview. So I would argue that we
>> should not do any backport because the feature itself is not officially
>> considered supported.
> Good point!
>
>
>> That said, what's missing to make the feature officially supported?
> If Oleksandr is also happy to make OP-TEE support in Xen "Supported" in
> SUPPORT.md I'd be happy with that too. Specifically I suggest to change
> it to:
>
> Status: Supported, not security supported
>
> Security Support is a bit of a heavy process and I am thinking that
> "Supported, not security supported" would be an excellent next step.

I would be happy, and can send a formal patch. But I am not an expert in 
this code.

As a user I can say that OP-TEE mediator works perfectly fine, but let's 
wait for the input from Volodymyr,

(looks like there are some TODO left in the code and I have no idea what 
are the implications)

Volodymyr Babchuk Oct. 7, 2021, 11:32 p.m. UTC | #7

Hi Oleksandr, Stefano,

Oleksandr <[e-mail]> writes:

> On 07.10.21 01:42, Stefano Stabellini wrote:
>
> Hi Stefano, Julien.
>
>> On Wed, 6 Oct 2021, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 28/09/2021 06:52, Stefano Stabellini wrote:
>>>> On Mon, 27 Sep 2021, Oleksandr Tyshchenko wrote:
>>>>> From: Oleksandr Tyshchenko <[e-mail]>
>>>>>
>>>>> Fix a possible copy-paste error in arm_smccc_smc's first argument (a0)
>>>>> for OPTEE_SMC_DISABLE_SHM_CACHE case.
>>>>>
>>>>> This error causes Linux > v5.14-rc5
>>>>> (b5c10dd04b7418793517e3286cde5c04759a86de
>>>>> optee: Clear stale cache entries during initialization) to stuck
>>>>> repeatedly issuing OPTEE_SMC_DISABLE_SHM_CACHE call and waiting for
>>>>> the result to be OPTEE_SMC_RETURN_ENOTAVAIL which will never happen.
>>>>>
>>>>> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
>>>> Acked-by: Stefano Stabellini <[e-mail]>
>>>>
>>>> I added Fixes: and Backport: tags to the commit
>>> Per SUPPORT.MD, OP-TEE is still a technical preview. So I would argue that we
>>> should not do any backport because the feature itself is not officially
>>> considered supported.
>> Good point!
>>
>>
>>> That said, what's missing to make the feature officially supported?
>> If Oleksandr is also happy to make OP-TEE support in Xen "Supported" in
>> SUPPORT.md I'd be happy with that too. Specifically I suggest to change
>> it to:
>>
>> Status: Supported, not security supported
>>
>> Security Support is a bit of a heavy process and I am thinking that
>> "Supported, not security supported" would be an excellent next step.
>
> I would be happy, and can send a formal patch. But I am not an expert
> in this code.

I'm will be happy with this too. We are using this mediator in our
projects and I know that OP-TEE community adopted tests for
virtualization in theirs CI stack. So this is kind of official now.

Also, I helped other people to bring up virtualization on theirs
platforms, so there are other users for this feature besides EPAM and
Linaro.

> (looks like there are some TODO left in the code and I have no idea
> what are the implications)

Well, there were a lot of TODOs when I submitted initial
implementation. At that time it indeed wasn't ready for production. But
I eventually fixed almost all of them. Only one left now. It is about
very unlikely situation when one of guest pages in mapped at PA=0. I'm
not sure that is even possible at all.

Patch

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 3453615..6df0d44 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -1692,7 +1692,7 @@  static bool optee_handle_call(struct cpu_user_regs *regs)
         return true;
 
     case OPTEE_SMC_DISABLE_SHM_CACHE:
-        arm_smccc_smc(OPTEE_SMC_ENABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
+        arm_smccc_smc(OPTEE_SMC_DISABLE_SHM_CACHE, 0, 0, 0, 0, 0, 0,
                       OPTEE_CLIENT_ID(current->domain), &resp);
         set_user_reg(regs, 0, resp.a0);
         if ( resp.a0 == OPTEE_SMC_RETURN_OK ) {