Re: [PATCH] optee: extend normal memory check to also write-through

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux