Re: DRM/KMS: vmwgfx driver is now available

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Sun, 23 Jul 2023 00:47:39 +0900
> From: PHO <pho%cielonegro.org@localhost>
> 
> On 7/22/23 22:23, Taylor R Campbell wrote:
> > For that matter, do we even need a new allocator here?  Can we just do
> > a new bus_dmamem_alloc/load for each one?
> 
> Sure, but I fear that would degrade the performance.

Any idea how much?  I don't have a good sense of how this is used in
the system.  Is this like a network driver that precreates a pool of
mbufs and dma maps up front, and then loads and unloads them during
packet processing?

> > Or, how important is this API?  It looks like there are two calls to
> > dma_pool_zalloc in vmwgfx.  Maybe it would be simpler to patch them
> > away so we don't have to spend a lot of time matching all the
> > semantics -- and tracking any changes in updates later -- for exactly
> > two users.
> 
> Which API? vmw_cmdbuf? It's a command buffer used for sending commands 
> to the virtual GPU, so we can't get rid of it.

I meant dma_pool.  I was thinking maybe we should just patch away the
uses of dma_pool in vmwgfx_cmdbuf instead of implementing the dma_pool
API.  But I haven't looked that closely; what you did is probably
fine.

> >>> 2. What's up with the restore_mode property?  Does anything set it?
> >>>      What's the protocol?  Why isn't this needed by all the other
> >>>      drivers, which can already gracefully handle exiting X?
> >>
> >> vmwgfxfb sets it and vmwgfx uses it. It's not needed as long as X
> >> gracefully exits, because it calls WSDISPLAYIO_SMODE ioctl to revert the
> >> mode back to WSDISPLAYIO_MODE_EMUL. But if X crashes or is killed by
> >> SIGKILL, the only way to perform a cleanup is to do something in the
> >> drm_driver::lastclose callback. That's why.
> > 
> > But why isn't this needed by all the other drivers?
> 
> I don't know. I suspect they have the same issue I explained because 
> their lastclose() looks like a no-op, but I can't test any other drivers 
> because I currently don't own any machines that runs NetBSD natively as 
> opposed to a VMware guest.

Seems very fishy.  Suspect either:

(a) this isn't necessary, and there's something else that's different
about vmwgfx; or

(b) this is necessary for all the drivers, and we need to have a
better generic mechanism (not kludging more function pointers into
stringy device properties or device calls) for switching the genfb
mode back on lastclose.

> > Is there a way we could just use or add a simple hook in genfb or
> > rasops for when there's dirty areas to update, so we don't have to
> > copy & paste all of that cruft from genfb and drmfb?
> > 
> > It looks like a painful maintenance burden; maintaining genfb on its
> > own is bad enough, and there's a lot of API cleanup warranted in
> > genfb, rasops, and wscons.
> 
> Yeah it is. I think we can extend the genfb API to do it and make 
> vmwgfxfb use it. I'll see what I can do.

Maybe macallan@ can help with getting the right hooks in here?

> > - In:
> > 
> >    static dma_addr_t __vmw_piter_dma_addr(struct vmw_piter *viter)
> >    {
> >    #if defined(__NetBSD__)
> > 	/* XXX: Is this... really? Really what we need to do? */
> > 	struct vm_page* const page = &viter->pages[viter->i]->p_vmp;
> > 	paddr_t const paddr = VM_PAGE_TO_PHYS(page);
> > 	bus_addr_t const baddr = PHYS_TO_BUS_MEM(viter->dmat, paddr);
> > 	return baddr;
> > 
> >    Seems wrong -- surely there should be a bus dma map here to use?
> 
> Yes there is, but bus_dmamap_t isn't page-oriented, right? I don't know 
> how to iterate through pages with it.

You can ask for maxsegsz=PAGE_SIZE boundary=PAGE_SIZE in
bus_dmamap_create, and then each segment's .ds_len will be PAGE_SIZE
and each .ds_addr will be page-aligned.

> > linux_dma_fence.c:
> > 
> > - In:
> > 
> >    -	KASSERT(dma_fence_referenced_p(fence));
> >    +	dma_fence_assert_alive(fence);
> > 
> >    These are not the same -- why the change?
> 
> Yeah this one... I know they aren't the same and you won't like it (_I 
> don't_) but there's a reason why the change is needed:
> 
> https://github.com/NetBSD/src/commit/af88505b56f338c25163dd3f172394b9f2a86492
> --
> linux/dma-fence.h: Allow fences to be signaled after their refcount 
> going down to zero as long as their .release callback hasn't called 
> dma_fence_free() yet. This semantics is not documented anywhere, and is 
> probably just an implementation detail, but the vmwgfx driver heavily 
> relies on it.
> 
> Sorry riastradh@, this is unfortunate but circumventing this (admittedly 
> healthy) assertion requires a major code reorganization of the driver. 
> We really can't fight against the upstream on this matter.
> --

You're right that I don't like it...it is completely bonkers
use-after-free!  Even i915 with its abuse of dma fence internals
doesn't violate these assertions.  What is vmwgfx doing that seems to
rely on it, which isn't just a broken missing reference count
acquire/release cycle or something?