Re: [RFC PATCH 0/5] drm/panthor: Protected mode support for Mali CSF GPUs

Florent Tomasin
2025-02-03T16:46:41Z

Hi Maxime, Nicolas

On 30/01/2025 17:47, Nicolas Dufresne wrote:
> Le jeudi 30 janvier 2025 à 17:38 +0100, Maxime Ripard a écrit :
>> Hi Nicolas,
>>
>> On Thu, Jan 30, 2025 at 10:59:56AM -0500, Nicolas Dufresne wrote:
>>> Le jeudi 30 janvier 2025 à 14:46 +0100, Maxime Ripard a écrit :
>>>> 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:
>>>>> 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.

When I came across this patch series:
-
https://lore.kernel.org/lkml/20230911023038.30649-1-yong.wu@mediatek.com/#t
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.

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