[Mesa-dev] [PATCH 2/4] i965: Fix intel_miptree_map() signature to be more 64-bit safe

[Ben Widawsky]

On Wed, Nov 19, 2014 at 10:40:56PM -0800, Ben Widawsky wrote:
> On Wed, Nov 19, 2014 at 09:18:54PM -0800, Kenneth Graunke wrote:
> > On Wednesday, November 19, 2014 02:13:03 PM Ian Romanick wrote:
> > > On 11/18/2014 09:11 PM, Chad Versace wrote:
> > > > This patch should diminish the likelihood of pointer arithmetic overflow
> > > > bugs, like the one fixed by b69c7c5dac.
> > > > 
> > > > Change the type of parameter 'out_stride' from int to ptrdiff_t. The
> > > > logic is that if you call intel_miptree_map() and use the value of
> > > > 'out_stride', then you must be doing pointer arithmetic on 'out_ptr'.
> > > > Using ptrdiff_t instead of int should make a little bit harder to hit
> > > > overflow bugs.
> > > 
> > > So... we talked about this a little bit on Monday, and I don't think we
> > > ever had a conclusion.  What happens if you have a 32-bit application
> > > running on a platform with 48-bit GPU address space?
> > 
> > CC'ing Ben, who knows all the gory details.
> > 
> > I don't really understand the problem - the GPU uses 48-bit addressing, and
> > can access more than 4G...but we're talking about map, which makes a buffer
> > available in an application's virtual address space...which is 32-bit in your
> > example.  It should always be placed at a < 4GB virtual address and work out
> > fine.
> 
> That's correct. Behind the scenes you very well may be getting 48b addresses,
> but like Ken said, that should present no issue assuming you can still reprsent
> a 64b data type. One would hope that whoever actually lands the patches provides
> a way on context create to only get a 4GB address space, since you'll save some
> memory, and make correctness a lot easier to "prove".
> 
> For the SVM case, there are two models.
> 1. Create a buffer on the CPU, and just use it at the same offset in the GPU
> - no problem.
> 2. Create a buffer with GEM on the GPU, and just use it on the CPU
> - This case, which doesn't make a whole lot of sense IMO, would take some work.
> 
> > 
> > That said, I don't think the kernel ever uses >= 4GB address spaces today.
> > Ben wrote 4GGGTT support and had both kernel and userspace patches to make
> > it work, but I don't think it ever actually landed.  I'm pretty sure
> > shipping userspace is not quite 48-bit safe - there are a few buffers that
> > have to be placed < 4GB (some hardware packets only take 32-bit addresses
> > still), and I don't think any software is in place to make that happen.
> 
> 4GGGTT landed (I haven't checked if it's been reverted). WRT to unsafe
> userspace, it's probably mostly safe after your patch to fix libdrm. Unless you
> start getting into the prelocation stuff, where I'd bet there are at least a few
> unsafe assumptions (unless my patches or equivalent were merged, the kernel
> isn't even 48b safe). I also think we determined the 0 state base thing won't
> work, which means the odds of actually needing more than 32b are quite small.
> 
> > 
> > --Ken

My memory unfailed. 4GGGTT is disabled for 32b platforms because of
aforementioned unreadiness in the kernel at that time.

-- 
Ben Widawsky, Intel Open Source Technology Center