[Mesa-dev] [PATCH 06/10] i965: Add virtual memory allocator infrastructure to brw_bufmgr.

[Kenneth Graunke]

On Tuesday, May 8, 2018 10:02:22 AM PDT Scott D Phillips wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
[snip]
> > +   /* If this node is now completely full, remove it from the free list. */
> > +   if (node->bitmap == 0ull) {
> > +      (void) util_dynarray_pop(vma_list, struct vma_bucket_node);
> > +   }
> > +
> > +   return node->start_address + bit * bucket->size;
> 
> This looks like a use-after-free. It's not really unsafe because
> dynarray is just reducing .size, so it could only go bad if somebody
> else sneaks in there and _trims or _grows. So it's safe, but it hits the
> danger pattern matcher in my head.

Whoops.  Indeed, that should be safe, but it's also stupid.  Fixed
(by making a uint64_t addr = ... before popping and then using that).

> Along similar lines, what is the mechanism that prevents multiple
> threads from entering into brw_bo_alloc with the same bufmgr
> simultaneously? The util/vma functions explode under simultaneous entry,
> so I had to guard them with a mutex in anv. You don't have something
> like that so I'm assuming something like the base mesa state tracking
> code is keeping that from happening for you.

The bufmgr->lock mutex is held on all paths calling vma_alloc or
vma_free...except for brw_bufmgr_destroy, but there's no concurrency
going on at final tear down.

[snip]
> > +   /* Canonicalize the address.
> > +    *
> > +    * The Broadwell PRM Vol. 2a, MI_LOAD_REGISTER_MEM::MemoryAddress says:
> > +    *
> > +    *    "This field specifies the address of the memory location where the
> > +    *     register value specified in the DWord above will read from. The
> > +    *     address specifies the DWord location of the data. Range =
> > +    *     GraphicsVirtualAddress[63:2] for a DWord register GraphicsAddress
> > +    *     [63:48] are ignored by the HW and assumed to be in correct
> > +    *     canonical form [63:48] == [47]."
> > +    */
> > +   const int shift = 63 - 47;
> > +   addr = (((int64_t) addr) << shift) >> shift;
> 
> I updated this in anv to be:
> 
> (int64_t)(addr << shift) >> shift;
> 
> If addr happened to have any of the top 16 bits set then you will get
> Undefined Behavior(tm) in C. Of course, that won't happen right here
> because __vma_alloc always returns non-canonical addresses, but better
> to have the fixed copy of the function sitting around.
> 
> With that update, this is
> 
> Reviewed-by: Scott D Phillips <scott.d.phillips at intel.com>

Good call.  If you move your copy into a common header, I'll just use
that.  For now, I've updated it to match your code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180508/414740dd/attachment.sig>