Message-ID: <AM6PR06MB4691764C8ABBF608837557D7A6F30@AM6PR06MB4691.eurprd06.prod.outlook.com>
Date:   Wed, 2 Dec 2020 09:40:59 +0000
From:   ZHIZHIKIN Andrey <andrey.zhizhikin@...ca-geosystems.com>
To:     Jens Wiklander <jens.wiklander@...aro.org>
CC:     "op-tee@...ts.trustedfirmware.org" <op-tee@...ts.trustedfirmware.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] optee: extend normal memory check to also write-through

Hello Jens,

> -----Original Message-----
> From: Jens Wiklander <jens.wiklander@...aro.org>
> Sent: Wednesday, December 2, 2020 9:07 AM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@...ca-geosystems.com>
> Cc: op-tee@...ts.trustedfirmware.org; Linux Kernel Mailing List <linux-
> kernel@...r.kernel.org>; stable@...r.kernel.org
> Subject: Re: [PATCH] optee: extend normal memory check to also write-through
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> Hi Andrey,
> 
> On Wed, Dec 2, 2020 at 8:11 AM Andrey Zhizhikin <andrey.zhizhikin@...ca-
> geosystems.com> wrote:
> >
> > ARMv7 Architecture Reference Manual [1] section A3.5.5 details Normal
> > memory type, together with cacheability attributes that could be
> > applied to memory regions defined as "Normal memory".
> >
> > Section B2.1.2 of the Architecture Reference Manual [1] also provides
> > details regarding the Memory attributes that could be assigned to
> > particular memory regions, which includes the descrption of
> > cacheability attributes and cache allocation hints.
> >
> > Memory type and cacheability attributes forms 2 separate definitions,
> > where cacheability attributes defines a mechanism of coherency control
> > rather than the type of memory itself.
> >
> > In other words: Normal memory type can be configured with several
> > combination of cacheability attributes, namely:
> > - Write-Through (WT)
> > - Write-Back (WB) followed by cache allocation hint:
> >   - Write-Allocate
> >   - No Write-Allocate (also known as Read-Allocate)
> >
> > Those types are mapped in the kernel to corresponding macros:
> > - Write-Through: L_PTE_MT_WRITETHROUGH
> > - Write-Back Write-Allocate: L_PTE_MT_WRITEALLOC
> > - Write-Back Read-Allocate: L_PTE_MT_WRITEBACK
> >
> > Current implementation of the op-tee driver takes in account only 2
> > last memory region types, while performing a check if the memory block
> > is allocated as "Normal memory", leaving Write-Through allocations to
> > be not considered.
> >
> > Extend verification mechanism to include also Normal memory regios,
> > which are designated with Write-Through cacheability attributes.
> 
> Are you trying to fix a real error with this or are you just trying to cover all cases? I
> suspect the latter since you'd likely have coherency problems with OP-TEE in
> Secure world if you used Write-Through instead.

Yes, this aims to provide consistency in detection which memory blocks can be identified
as Normal memory in ARMv7 architecture.

WT coherency control and (especially) observability behavior is described in section A3.5.5 of the
ARMv7 RefMan, where it is stated that write operations performed on WT memory locations
are guaranteed to be visible to all observers inside and outside of cache level.

As the Write-Through (WT) provides a better coherency control, it does make sense to include it
into the verification performed by is_normal_memory() in order to provide a possibility for
future implementations to mitigate issues and select appropriate cache allocation attributes
for memory blocks used.

> Correct me if I'm wrong, but "Write-Back Write-Allocate" and "Write-Back Read-Allocate"
> are both compatible with each other as the "Allocate" part is just a hint.

Correct, "Allocate" just designates the cache allocation hint. "Write-Back Read-Allocate" should
actually be read as "Write-Back no Write-Allocate", with " Write-Allocate" being a hint. But since
this is controlled by a TEX[0] - this hint is handled separately by L_PTE_MT_WRITEBACK and 
L_PTE_MT_WRITEALLOC macros.

> 
> Cheers,
> Jens
> 
> >
> > Link: [1]:
> > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeve
> >
> loper.arm.com%2Fdocumentation%2Fddi0406%2Fcd&amp;data=04%7C01%7C%7
> Ca40
> >
> ffd35912f4fe3d97308d896993b87%7C1b16ab3eb8f64fe39f3e2db7fe549f6a%7C0%
> 7
> >
> C1%7C637424932169074654%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLC
> >
> JQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=c0jK2gT
> m
> > qrAyo0%2Ffr07t%2Fg5NbPdm4dh7Rl7alNWlaQc%3D&amp;reserved=0
> > Fixes: 853735e40424 ("optee: add writeback to valid memory type")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Andrey Zhizhikin
> > <andrey.zhizhikin@...ca-geosystems.com>
> > ---
> >  drivers/tee/optee/call.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index
> > c981757ba0d4..8da27d02a2d6 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -535,7 +535,8 @@ static bool is_normal_memory(pgprot_t p)  {  #if
> > defined(CONFIG_ARM)
> >         return (((pgprot_val(p) & L_PTE_MT_MASK) == L_PTE_MT_WRITEALLOC)
> ||
> > -               ((pgprot_val(p) & L_PTE_MT_MASK) == L_PTE_MT_WRITEBACK));
> > +               ((pgprot_val(p) & L_PTE_MT_MASK) == L_PTE_MT_WRITEBACK) ||
> > +               ((pgprot_val(p) & L_PTE_MT_MASK) ==
> > + L_PTE_MT_WRITETHROUGH));
> >  #elif defined(CONFIG_ARM64)
> >         return (pgprot_val(p) & PTE_ATTRINDX_MASK) ==
> > PTE_ATTRINDX(MT_NORMAL);  #else
> > --
> > 2.17.1
> >

Regards,
Andrey

Powered by blists - more mailing lists