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

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

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

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 <oleksandr_tyshchenko@epam.com>
---
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

Re: [PATCH] xen/arm: optee: Fix arm_smccc_smc's a0 for OPTEE_SMC_DISABLE_SHM_CACHE

[Bertrand Marquis]

Hi Oleksandr,

> On 27 Sep 2021, at 14:54, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote:
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> 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 <oleksandr_tyshchenko@epam.com>

Sound like a reasonable change :-)

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

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
> 
> 

Re: [PATCH] xen/arm: optee: Fix arm_smccc_smc's a0 for OPTEE_SMC_DISABLE_SHM_CACHE

[Volodymyr Babchuk]

Hi Oleksandr,

Oleksandr Tyshchenko <olekstysh@gmail.com> writes:

> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> 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 <oleksandr_tyshchenko@epam.com>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
> 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 ) {


-- 
Volodymyr Babchuk at EPAM

Re: [PATCH] xen/arm: optee: Fix arm_smccc_smc's a0 for OPTEE_SMC_DISABLE_SHM_CACHE

[Stefano Stabellini]

On Mon, 27 Sep 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> 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 <oleksandr_tyshchenko@epam.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

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
> 

Re: [PATCH] xen/arm: optee: Fix arm_smccc_smc's a0 for OPTEE_SMC_DISABLE_SHM_CACHE

[Julien Grall]

Hi Stefano,

On 28/09/2021 06:52, Stefano Stabellini wrote:
> On Mon, 27 Sep 2021, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> 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 <oleksandr_tyshchenko@epam.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 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,

-- 
Julien Grall

Re: [PATCH] xen/arm: optee: Fix arm_smccc_smc's a0 for OPTEE_SMC_DISABLE_SHM_CACHE

[Stefano Stabellini]

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 <oleksandr_tyshchenko@epam.com>
> > > 
> > > 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 <oleksandr_tyshchenko@epam.com>
> > 
> > Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > 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.

Re: [PATCH] xen/arm: optee: Fix arm_smccc_smc's a0 for OPTEE_SMC_DISABLE_SHM_CACHE

[Oleksandr]

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 <oleksandr_tyshchenko@epam.com>
>>>>
>>>> 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 <oleksandr_tyshchenko@epam.com>
>>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>> 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)


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH] xen/arm: optee: Fix arm_smccc_smc's a0 for OPTEE_SMC_DISABLE_SHM_CACHE

[Volodymyr Babchuk]

Hi Oleksandr, Stefano,

Oleksandr <olekstysh@gmail.com> 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 <oleksandr_tyshchenko@epam.com>
>>>>>
>>>>> 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 <oleksandr_tyshchenko@epam.com>
>>>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>> 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.

-- 
Volodymyr Babchuk at EPAM