Le lundi 03 février 2025 à 16:43 +0000, Florent Tomasin a écrit :

Hi Maxime, Nicolas

On 30/01/2025 17:47, Nicolas Dufresne wrote:

quoted

Le jeudi 30 janvier 2025 à 17:38 +0100, Maxime Ripard a écrit :

quoted

Hi Nicolas,

On Thu, Jan 30, 2025 at 10:59:56AM -0500, Nicolas Dufresne wrote:

quoted

Le jeudi 30 janvier 2025 à 14:46 +0100, Maxime Ripard a écrit :

quoted

Hi,

I started to review it, but it's probably best to discuss it here.

On Thu, Jan 30, 2025 at 01:08:56PM +0000, Florent Tomasin wrote:

quoted

Hi,

This is a patch series covering the support for protected mode execution in
Mali Panthor CSF kernel driver.

The Mali CSF GPUs come with the support for protected mode execution at the
HW level. This feature requires two main changes in the kernel driver:

1) Configure the GPU with a protected buffer. The system must provide a DMA
   heap from which the driver can allocate a protected buffer.
   It can be a carved-out memory or dynamically allocated protected memory region.
   Some system includes a trusted FW which is in charge of the protected memory.
   Since this problem is integration specific, the Mali Panthor CSF kernel
   driver must import the protected memory from a device specific exporter.

Why do you need a heap for it in the first place? My understanding of
your series is that you have a carved out memory region somewhere, and
you want to allocate from that carved out memory region your buffers.

How is that any different from using a reserved-memory region, adding
the reserved-memory property to the GPU device and doing all your
allocation through the usual dma_alloc_* API?

How do you then multiplex this region so it can be shared between
GPU/Camera/Display/Codec drivers and also userspace ?

You could point all the devices to the same reserved memory region, and
they would all allocate from there, including for their userspace-facing
allocations.

I get that using memory region is somewhat more of an HW description, and
aligned with what a DT is supposed to describe. One of the challenge is that
Mediatek heap proposal endup calling into their TEE, meaning knowing the region
is not that useful. You actually need the TEE APP guid and its IPC protocol. If
we can dell drivers to use a head instead, we can abstract that SoC specific
complexity. I believe each allocated addressed has to be mapped to a zone, and
that can only be done in the secure application. I can imagine similar needs
when the protection is done using some sort of a VM / hypervisor.

Nicolas

The idea in this design is to abstract the heap management from the
Panthor kernel driver (which consumes a DMA buffer from it).

In a system, an integrator would have implemented a secure heap driver,
and could be based on TEE or a carved-out memory with restricted access,
or else. This heap driver would be responsible of implementing the
logic to: allocate, free, refcount, etc.

The heap would be retrieved by the Panthor kernel driver in order to
allocate protected memory to load the FW and allow the GPU to enter/exit
protected mode. This memory would not belong to a user space process.
The driver allocates it at the time of loading the FW and initialization
of the GPU HW. This is a device globally owned protected memory.

This use case also applies well for codec. The Mediatek SCP firmware needs to be
loaded with a restricted memory too, its a very similar scenario, plus Mediatek
chips often include a Mali. On top of that, V4L2 codecs (in general) do need to
allocate internal scratch buffer for the IP to write to for things like motion
vectors, reconstruction frames, entropy statistics, etc. The IP will only be
able to write if the memory is restricted.

Nicolas

When I came across this patch series:
-
https://lore.kernel.org/lkml/20230911023038.30649-1-yong.wu@mediatek.com/#t (local)
I found it could help abstract the interface between the secure heap and
the integration of protected memory in Panthor.

A kernel driver would have to find the heap: `dma_heap_find()`, then
request allocation of a DMA buffer from it. The heap driver would deal
with the specifities of the protected memory on the system.

quoted

quoted

quoted

Also, how the secure memory is allocted / obtained is a process that
can vary a lot between SoC, so implementation details assumption
should not be coded in the driver.

But yeah, we agree there, it's also the point I was trying to make :)

Maxime

Agree with your point, the Panthor kernel driver may not be aware of the
heap management logic. As an alternative to the DMA heap API used here,
I also tried to expose the heap by passing the phandle of a "heap"
device to Panthor. The reference to the DMA heap was stored as a private
data of the heap device as a new type: `struct dma_heap_import_info`,
similar to the existing type: `struct dma_heap_export_info`.
This made me think it could be problematic, as the private data type
would have to be cast before accessing it from the importer driver. I
worried about a mis-use of the types with this approach.

Regards,
Florent