On Wed, Dec 2, 2020 at 10:41 AM ZHIZHIKIN Andrey <andrey.zhizhikin@xxxxxxxxxxxxxxxxxxxx> wrote: > > Hello Jens, > > > -----Original Message----- > > From: Jens Wiklander <jens.wiklander@xxxxxxxxxx> > > Sent: Wednesday, December 2, 2020 9:07 AM > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@xxxxxxxxxxxxxxxxxxxx> > > Cc: op-tee@xxxxxxxxxxxxxxxxxxxxxxxxx; Linux Kernel Mailing List <linux- > > kernel@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx > > 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@leica- > > 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. I think you're missing the purpose of this internal function. It's there to check that the memory is mapped in a way compatible with what OP-TEE is using in Secure world. > > 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. B3.11.3 in the spec requires cache maintenance when changing from Write-Back to Write-Through and vice versa, and we can't do that in this design. Cheers, Jens > > > > > Cheers, > > Jens > > > > > > > > Link: [1]: > > > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeve > > > > > loper.arm.com%2Fdocumentation%2Fddi0406%2Fcd&data=04%7C01%7C%7 > > Ca40 > > > > > ffd35912f4fe3d97308d896993b87%7C1b16ab3eb8f64fe39f3e2db7fe549f6a%7C0% > > 7 > > > > > C1%7C637424932169074654%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw > > MDAiLC > > > > > JQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=c0jK2gT > > m > > > qrAyo0%2Ffr07t%2Fg5NbPdm4dh7Rl7alNWlaQc%3D&reserved=0 > > > Fixes: 853735e40424 ("optee: add writeback to valid memory type") > > > Cc: stable@xxxxxxxxxxxxxxx > > > Signed-off-by: Andrey Zhizhikin > > > <andrey.zhizhikin@xxxxxxxxxxxxxxxxxxxx> > > > --- > > > 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