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

From: ZHIZHIKIN Andrey
Date: Wed Dec 02 2020 - 05:57:35 EST

Next message: Heiko Carstens: "Re: [GIT pull] locking/urgent for v5.10-rc6"
Previous message: Adrian Ratiu: "[PATCH v4] char: tpm: add i2c driver for cr50"
In reply to: Jens Wiklander: "Re: [PATCH] optee: extend normal memory check to also write-through"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello Jens,

> -----Original Message-----
> From: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> Sent: Wednesday, December 2, 2020 11:44 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
>
>
> On Wed, Dec 2, 2020 at 10:41 AM ZHIZHIKIN Andrey <andrey.zhizhikin@leica-
> geosystems.com> 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.

OK, now it's clear! Then it is more a matter of is_normal_memory() function name, which
is mis-leading. I was under impression that this function (at least from its name) is
verifying that the memory block is considered as Normal memory in terms of ARMv7
architecture, but it is rather a check if the memory block is *usable* for the OP-TEE
purposes.

If that is the case - you can drop this patch altogether, but I believe that the function
name should be changed to reflect the actual purpose to avoid future confusions.

Does that sound reasonable?

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

Correct. My expectation here would be that the cache allocation policy change should
be accompanies by explicit cache maintenance operation *before* it is submitted to
the driver, but that might not be how it is designed.

>
> Cheers,
> Jens
>
> >
> > >
> > > Cheers,
> > > Jens
> > >
> > > >
> > > > Link: [1]:
> > > > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > deve
> > > >
> > >
> 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=c0jK2g
> > > T
> > > 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

Regards,
Andrey

Next message: Heiko Carstens: "Re: [GIT pull] locking/urgent for v5.10-rc6"
Previous message: Adrian Ratiu: "[PATCH v4] char: tpm: add i2c driver for cr50"
In reply to: Jens Wiklander: "Re: [PATCH] optee: extend normal memory check to also write-through"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]