[Intel-gfx] [PATCH] drm/i915/vlv: Added write-enable pte bit support

[Goel, Akash]

Daniel,

>> I think a start would be to roll the r/o bit out just for ringbuffers.
>> With that we can then implement the byt_pte_encode stuff and related infrastructure.

Thanks for the review, so I will modify the patch, to use the ro protection only for the ring buffer.

Best Regards
Akash

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Sunday, February 09, 2014 4:04 PM
To: Goel, Akash
Cc: Daniel Vetter; Chris Wilson; intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: Added write-enable pte bit support

Aside: Your quoting is a bit strange, it looks like part of Chris' mail is indented differently than other paragraphs from the same mail, which then in turn match the indent for my reply. And the indent-levels are wrong, my replay has >> but Chris's has only one >. Rather confusing.

I suggest you switch to a saner mail client ;-)

On Fri, Feb 07, 2014 at 05:44:00AM +0000, Goel, Akash wrote:
> > Don't cause us to rewrite the PTE for the batch buffer between each 
> > execbuffer (ro for the batch, rw next time it gets used as a texture).
> > In fact, do not change ro without user intervention.
> > gt_old_ro is unused
> Sorry we didn't consider this scenario, that a batch buffer could be 
> subsequently used as a different type of buffer also, like as a 
> texture buffer. But is the remap going to have a significant overhead.
> 
> >> Use byt_pte_encode() instead of hacking the result of ppgtt->pte_encode().
> >> Consider expanding i915_cache_level so that it included the concept of PROT_READ | PROT_WRITE.
> Thanks, actually we initially thought of doing it in a same way. Can 
> we overload the i915_cache_level enum with the RO flag, while calling 
> 'insert_entries' for VALLEYVIEW platform.
> 
> >> Also, what's the exact use-case for this here? And if we need to expose this to userspace, then it needs a testcase.
> >> -Daniel
> Since we have this RO bit available for our disposal on BYT platform, 
> we thought we can avail it in order to have an extra protection on the 
> buffers.
> We initially used it only for Ring & Batch buffers as we know, without 
> User's input, that they are supposed to be read-only from GPU side.
> For extending this for other buffers, we need a Libdrm level change. 
> Or can we use the Read/Write domains information in relocation entries 
> to decide this internally on the Driver side, but that will require 
> some change in the exec-buffer path.

I think a start would be to roll the r/o bit out just for ringbuffers.
With that we can then implement the byt_pte_encode stuff and related infrastructure.

Once we have that we can use it in more places internally, e.g. the command scanner could copy scanned batches to r/o gem buffers. With that use-case we'll never have a conflict which requires switching to r/w access again since the batch buffer scanner gem bos will be on a separate list.

Once we've knocked down the easy security improvements this allows we can look at any benefits for userspace-allocated gem buffers. But since the gpu doesn't fault it's not useful as a debug feature. And for better isolation we really want ppgtt (which would resolve all these issues even without r/o bits).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch