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@xxxxxxxxxxxx/#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
