Re: [PATCH v2] drm: omapdrm: Export correct scatterlist for TILER backed BOs

[Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>]

Hi,

On 15.11.21 г. 12:37 ч., Tomi Valkeinen wrote:
> On 15/11/2021 11:23, Matthijs van Duin wrote:
>> On Mon, Nov 15, 2021 at 10:42:41AM +0200, Tomi Valkeinen wrote:
>>> A BO's memory via the TILER memory is
>>> contiguous, although with consistent gaps of
>>> memory that should not be accessed.
>>
>> But pretending that these "gaps" are part of the buffer is a security
>> vulnerability, since that memory which "should not be accessed" may
>> belong to different security contexts, and exporting the entire
>> contiguous region covering the buffer allows untrusted contexts (e.g.
>> userspace) to access this memory.
> 
> Yes, I fully agree. I wasn't criticizing the patch, just wanted to 
> highlight the unmentioned aspects.
> 
>>> IPs that might use TILER
>>> backed BOs only support contiguous memory.
>>>
>>> This means that the drivers for such IPs cannot
>>> use the BOs exported like you do in this patch.
>>> I believe the drivers could be improved by
>>> writing a helper function which studies the
>>> sg_table and concludes that it's actually
>>> contiguous.
>>
>> That indeed sounds like the proper solution for such importers, rather
>> than making the exporter lie about the buffer bounds to work around
>> limitations of these importers.
> 
> The annoying thing with this solution is that we need to add extra code 
> to all the drivers that want to import TILER buffers, even if the 
> drivers shouldn't know anything about TILER.
> 
> Then again, the code is not really TILER or OMAP specific, and any IP on 
> any platform that only supports contiguous buffers, but supports stride, 
> could import such buffers. Which hints that maybe the code should be 
> somewhere in the framework, not in the driver. In practice it may be 
> better to just swallow the annoyance, add the code to the drivers and be 
> done with it =).
> 
>>> Did you look at the userspace mmap of TILER
>>> buffers? I wonder if that goes correctly or not.
>>> Isn't memory to userspace mapped per page, and
>>> lengths of the TILER lines are not page aligned?
>>
>> Mapping to userspace uses an ugly hack whereby small slabs of the
>> buffer (4096x64 (8bpp), 2048x32 (16bpp), or 1024x32 (32bpp) pixels) are
>> dynamically mapped to dedicated page-aligned regions of the TILER
>> virtual space.  For each of the three bitdepths only two such slabs can
>> be mapped into userspace at any given time (on the entire system), so
>> using this mechanism to render graphics from userspace can easily cause
>> hundreds if not thousands of page faults per second.
> 
> Ah, right, yes, now I remember. The userspace mmap of TILER buffers 
> isn't very usable.
> 
>> The alternative (used e.g. in the pyra kernel) is to force all TILER
>> buffers to be page-aligned, at the cost of wasting some TILER space.
>> This will presumably also be necessary to allow SGX to import these
>> buffers since its MMU can obviously also not map data which is not
>> page-aligned, same for any other importer which uses an MMU to enforce
>> memory security (rather than being trusted to simply refrain from
>> accessing data outside the declared bounds).
>>
>> Ideally such page-alignment should only be applied to buffers which are
>> intended to be consumed by importers which require this, though it's not
>> clear how that might be accomplished.
> 
> Do you mean that each TILER _line_ should be page aligned and the length 
> should be page divisible? Doesn't that cause quite a lot of wasted 
> space? Although that, of course, depends on the use. If the primary use 
> case is allocating a few full screen display buffers, perhaps the waste 
> is negligible.
> 

Yes, I think this is the idea, otherwise no MMU can be set correctly.

> In any case, I'm fine with that change, too, as it helps making TILER 
> usable.
> 

That's great, will send a patch ASAP.

> And while speaking about usable, if I recall right, the 
> omap_bo_new_tiled() is pretty annoying to use. You need to give the 
> width and OMAP_BO_TILED_x flag as a parameter, and if I recall right, 
> it's all but obvious how those need to be set for, e.g. NV12. But it 
> works so perhaps better to keep it as it is...
> 

To me the whole omap_bo_xxx stuff should go away and be replaced by 
gbm_bo_xxx stuff. The only issue there is with TILER BOs, but I think 
we'll be able to get away with that with a little abuse of GBM_BO_XXX 
flags (see the other mail)

> There was also some particular YUV format with some rotations that I 
> never got working, even after discussing with TI DSS HW guys.
> 
>   Tomi

Thanks,
Ivo