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

[Ivaylo Dimitrov]

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