Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

[Jason-JH Lin (林睿祥)]

On Mon, 2024-07-01 at 11:14 +0200, Christian König wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Am 28.06.24 um 15:40 schrieb mripard@xxxxxxxxxx:
> > [SNIP]
> > > > So nobody can map that buffer, and the firmware driver is the
> > > > one who
> > > > knows that this buffer cannot be accessed by anyone.
> > > 
> > > On most hw I know you can actually map that buffer, it's just
> > > that the
> > > CPU sees only garbage in it because you don't have the necessary
> > > decryption keys around.
> > 
> > So you can always map and access the buffer, but only if you're in
> > the
> > right "context" the content would be correct?
>  
> Exactly that, yes. You need to have access to the decryption keys.
> 
> > I think that part is pretty different than what ARM SoCs are doing,
> > where they would typically prevent any CPU access and fault on
> > access.
>  
> Yeah, that's indeed an important difference Nicolas noted as well.
> 
> 

As I know, there are 4 kind of buffer in SVP flow of MediaTek platform:
1) The normal buffer with the encrypted video bitstream content
(source buffer for decryption engine)
User APP will push encrypted data to this buffer for decrypted engine,
so it should be normal buffer.

2) The secure buffer with the decrypted video bitstream content
(source buffer for video decoder engine)

3) The secure buffer with decoded frame data content
(source buffer for GPU)

4) The secure buffer with resized and format changed frame data content
(source buffer for display engine)


The secure buffer in DRM SVP series:
https://patchwork.kernel.org/project/linux-mediatek/list/?series=855888
should be resized and format changed frame data for display engine and
it can only be accessed in TEE world by the secure display hardware.

If CPU access this buffer in normal world or non-secure display
hardware are configured to access this buffer, they will get an IOMMU
fault error and it can not translate to the correct buffer address.

> > > > Putting this on the userspace to know would be pretty weird,
> > > > and
> > > > wouldn't solve the case where the kernel would try to map it.
> > > 
> > > But that's exactly how all other implementations work as far as I
> > > know. I
> > > mean what do you do if the kernel maps the encrypted buffer?
> > > 
> > > On AMD we also block userspace and kernel CPU accesses, but that
> > > is only
> > > done to make it easier to find bugs not for correctness.
> > > 
> > > And userspace absolutely needs to be aware that a buffer is
> > > encrypted, cause
> > > otherwise it could potentially try to access it with the CPU.
> > 
> > I absolutely agree. I guess our discussion is whether it's
> > something
> > that should be implicit and understood by applications, or if it
> > should
> > be explicit and discoverable.
>  
> Oh good point as well. But I think that's more a question for the
> userspace stack design.
> 
> E.g. it can be handled explicitly by the application itself or
> implicitly by some V4L or VA-API library or something similar.
> 
> For the kernel UAPI design we agreed at some point that we don't want
> to have any implicit properties on the DMA-buf which are carried
> around by the kernel, e.g. the whole format metadata for example.
> 

I've asked our vcodec owner how to handle the same case in V4L2:

https://patchwork.kernel.org/project/linux-mediatek/patch/20240516122102.16379-2-yunfei.dong@xxxxxxxxxxxx/

They also added a secure flag in UAPI and V4L2 header and then passed
it from userspace to V4L2 driver.

So maybe we can use the same way to add a secure flag in UAPI and
extend the flag parameter of AddModeFB2().

I think here is currently the most suitable place to add the secure
flag besides adding the new ioctl().

Regards,
Jason-JH.Lin

> One notable exception to this is the actual hw topology, e.g. when
> for example a device has a special interconnect to another device and
> you need to make sure that the devices are powered up and down in a
> specific order to make things work.
> 
> This should then made known to the core kernel using device link
> structure. E.g. similar to how it's used between GPU and HDMI audio
> block for example.
> 
> Regards,
> Christian.
> 
> > Maxime
>