[RFC] flink_to

[Kristian Høgsberg]

2010/7/12 Dave Airlie <airlied at redhat.com>:
> On Mon, 2010-07-12 at 11:55 -0400, Kristian Høgsberg wrote:
>> [ Let's try this again... ]
>>
>> Ok, so the flink_to discussion rat-holed a bit on the binary blob
>> attachment issue.  But before we even get to that, there are a lot of
>> issues that I'd some feedback on as well.  There were a few bugs in
>> the patch that I've fixed, but I don't see the point in sending it out
>> again just yet, as I'd like to see if we can agree on some of the
>> higher level issues.
>>
>> First of, I'm hoping we can get this ready for the next merge window.
>> The patch is written and I've tested it here with a libdrm test case
>> and am currently finishing the EGL support for this.  One change I
>> might want to do is to add a blob type argument to the ioctls so
>> userspace has a standardized way of indication what the format of the
>> data is (what Keith pointed out).  That's a fairly simple change and
>> the patch itself is simple enough to begin with, so I don't expect a
>> lot of tricky issues with the implementation.
>>
>> Along with the flink_to ioctl, I'm proposing that we drop the DRM_AUTH
>> requirement for accessing the gem ioctls.  Specifically, for intel,
>> I'm suggesting that we drop the DRM_AUTH requirement on
>>
>>   DRM_IOCTL_GEM_FLINK,
>>   DRM_IOCTL_GEM_OPEN,
>>   DRM_I915_GEM_EXECBUFFER,
>>   DRM_I915_GEM_EXECBUFFER2,
>>   DRM_I915_GEM_BUSY,
>>   DRM_I915_GEM_THROTTLE,
>>   DRM_I915_GEM_CREATE,
>>   DRM_I915_GEM_PREAD,
>>   DRM_I915_GEM_PWRITE,
>>   DRM_I915_GEM_MMAP,
>>   DRM_I915_GEM_MMAP_GTT,
>>   DRM_I915_GEM_SET_DOMAIN,
>>   DRM_I915_GEM_SW_FINISH,
>>   DRM_I915_GEM_SET_TILING,
>>   DRM_I915_GEM_GET_TILING,
>>   DRM_I915_GEM_GET_APERTURE,
>>   DRM_I915_GEM_MADVISE,
>>
>> which should allow clients to create buffers, submit rendering against
>> them and share them with a priviledged (in the sense that it controls
>> scanout) display server.  Access to these ioctls will then only be
>> guarded by the permissions on /dev/dri/cardX, which all distros
>> restrict to the 'local' user (that is, excluding ssh and similar) in
>> one way or another.  How do we feel about that?  Maybe it's something
>> the master needs to request, so that only X servers that use flink_to
>> will activate this mode?
>
> I'd rather we only did this if we knew everyone was going to use
> flink_to, and then make sure normal flink doesn't work at all once
> someone started using flink_to perhaps.

It's only the master (and its clients) that have anything to lose from
dropping auth, so if the master can say "I'm using flink_to, go ahead"
shouldn't that be sufficient?  Additionally we could just make flink a
master only ioctl. In our current stack, only the X server is using
flink, so it wont make a difference.

Besides, isn't the only concern new gpgpu type clients using flink and
making their buffers available to every body?  And isn't that a case
of "dont do that"?  What other clients might use flink, and who's at
risk if they do?

> But otherwise it all sounds good.
>
>>
>> flink_to doesn't in itself solve the security problem, since user
>> space can still submit a batch buffer that reads or writes to an
>> absolute gtt offset (that is, no relocation).  The X front buffer
>> location is typically pretty predictable, for example.  flink_to does
>> give us the infrastructure to implement a secure system though.  There
>> are several ways this could be done: use a sw command checker to
>> reject absolute gtt offsets, unbind buffers from all other clients
>> before running executing the commands or use per-process gtt or
>> similar hw support.
>
> Doesn't solve the security problem for *Intel*. On radeon for example
> we've always provided this type of security, GEM's interface is the only
> hole in that case (apart from the sw checker maybe missing some cases).
> So I'm quite happy that this is what we'd prefer.

Oh, I know, I'm just saying that flink_to alone doesn't solve the
problem, there has to be something else enforcing the buffer access.

>> Then there's the data passing mechanism part of flink_to.  I'm
>> suggesting that we allow applications to attach a blob to an flink_to
>> name, which will be passed to the process calling open_with_data on
>> the name.  The format of the blob is defined by userspace, typically
>> libdrm or mesa, and lets us marshal meta data about the buffer along
>> with granting access to the buffer.  And just to be clear, the kernel
>> has no need for this meta data, it doesn't even understand the format.
>> But it will make protocoles and user level APIs simpler, and it's not
>> going to be a resource drain in the kernel.  There's a 2k max size on
>> the attached data, and a buffer can only have one flink_to name
>> pending per file_priv.  I didn't see any strong objections in the
>> thread, but I understand the concerns.  We're debating a minimal
>> kernel API with a kludgey userspace vs kernel API with convenience
>> features and much simpler userspace.
>
> Its really ugly, and its really going to end up as ABI, except the
> people hacking on the X server will forget that the people hacking on
> mesa need to have the same struct or some such as they will think they
> are giving the data to the kernel.

It will definitely be ABI, I'm not suggesting otherwise, just not
kernel ABI.  The blob will have a 32 bit type code that allows
userspace to check if it's the format it expects. I imagine the lower
16 bits could be a minor number to indicate backwards compatible
extension of the data and the upper 16 bits indicate incompatible,
different types of data.  It will be defined in libdrm as a struct
that only libdrm knows how to encode and decode (like how we
encapsulate ioctl structs).

> I really get the feeling this would
> work better in userspace, or with at least a format that works in the
> kernel. Is the data going to be per-GPU? per-driver? per-what? Who is
> expecting to interpret it in userspace? what happens if in a few months
> you realise you need 4k. (2k is pointless, since it'll eat a page
> anyways).

It will be a chipset specific struct.  That's a key part of the idea,
since then we don't have to generalize the contents or define generic
descriptions of format and tiling details etc.  Making the struct
chipset specific means we can describe everything the driver needs to
communicate about the buffer, in the native tokens used by the
hardware.  For intel, that means we can define the buffer format as
BRW_SURFACEFORMAT_B8G8R8A8_UNORM instead of trying to define DRM level
codes for buffer formats etc.

As for the size limit, I made it PAGE_SIZE - sizeof (struct
drm_gem_name) originally, but went with 2048 in case we needed to grow
the struct size kernel side.  And if you need more than 2048 bytes of
metadata, I think it's safe to say that you should use a different
mechanism.

If we did this in userspace, we would have all the same issues, except
we would have to pass a blob of data around in protocol and APIs.  The
ABI and version, encoding and decoding would still be managed in
libdrm, but with flink_to the kernel helps pass the data around.

> Yes its meta-data to the kernel, but flink_to is a generic
> userspace interface and attaching a bunch of non-generic data to it
> sounds hackish.

But when you think about how we use this generic interface
(flink+open) today, it's always only from chipset specific code.  It
can't ever be generic anyway, flink needs to mark the bo shared in the
bufmgr and take it out of caching lists, open needs to set up the
drm_$chipset_bo struct and do chipset specific ioctls anyway to get
tiling and other metadata.

My first take on this idea was indeed to make flink_to chipset
specific and use a good old struct defined in i915_drm.h for passing
the data in and out of the kernel.  This has the advantage that we
define the format in the kernel header and all clients have to agree
on the format whether or not they use libdrm.  However that seems a
little academic (since all clients use libdrm), and the blob approach
lets us define the format of the data used only by userspace in
userspace and keep the ioctl generic.  But we can go back to that if
that's a more acceptable approach.

Kristian