DRM/KMS vmwgfx second try
Hi! I just updated my series of vmwgfx patches. Could you take a look please? I implemented dma_pool in terms of vmem(9), and also rewrote vmwgfxfb so that it uses drm_fb_helper and drmfb. genfb(9) now has a new optional callback which allows drivers to notify GPU of dirty regions (and we need to bump the kernel ABI because of this): https://github.com/NetBSD/src/compare/trunk...depressed-pho:netbsd-src:drm2-vmwgfx
Re: Strange crash of DIAGNOSTIC kernel on cv_destroy(9)
On 7/24/23 23:08, Taylor R Campbell wrote: Date: Mon, 24 Jul 2023 12:05:34 +0900 From: PHO I realized the cause of this: [...] There were cases where the function was destroying a condvar that it didn't initialize! Ugh, this is the very reason why I dislike C... Oops... Sorry, was blowing through the vmwgfx code a little too hastily there. Here's a patch to simplify it and make it easier to read; might work better. No worries. Your patch worked fine. Thank you!
Re: Strange crash of DIAGNOSTIC kernel on cv_destroy(9)
On 7/23/23 17:27, PHO wrote:
On 7/22/23 22:41, Taylor R Campbell wrote:
Date: Sat, 22 Jul 2023 21:52:40 +0900
From: PHO
Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6151161]
vmw_fence_wait() at netbsd:vmw_fence_wait+0xdc
Just to confirm, what does `info line *(vmw_fence_wait+0xdc)' say in
gdb?
And, if you can get to the frame in gdb, what does gdb say is
in the vmw_fence_wait frame, and what cv is in the cv_destroy frame?
Let's confirm it is the cv you think it is -- I suspect it might be a
different one.
I just encountered the crash and could obtain a crash dump. It is indeed
the "DRM_DESTROY_WAITQUEUE()" in vmw_fence_wait() but the contents
of cb does not make sense to me:
...
CV_SLEEPQ(cv) is 0x01 (wtf) and CV_WMESG(cv) is not even a string?
I realized the cause of this:
static long vmw_fence_wait(struct dma_fence *f, bool intr, signed long
timeout)
{
...
if (likely(vmw_fence_obj_signaled(fence)))
return timeout;
...
spin_lock(f->lock);
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags))
goto out; // <-- THIS ONE
if (intr && signal_pending(current)) {
ret = -ERESTARTSYS;
goto out; // <-- OR THIS
}
#ifdef __NetBSD__
DRM_INIT_WAITQUEUE(, "vmwgfxwf");
#else
cb.task = current;
#endif
...
out:
spin_unlock(f->lock);
#ifdef __NetBSD__
DRM_DESTROY_WAITQUEUE();
#endif
...
}
There were cases where the function was destroying a condvar that it
didn't initialize! Ugh, this is the very reason why I dislike C...
Re: DRM/KMS: vmwgfx driver is now available
On 7/24/23 11:34, PHO wrote:
if (!list_empty(_put)) {
spin_unlock(>lock);
list_for_each_entry_safe(fence, next_fence, _put, head) {
list_del_init(>head);
dma_fence_put(>base);
}
spin_lock(>lock);
}
I pasted a wrong version. Actually this:
while ((fence = list_first_entry_or_null(
_put, typeof(*fence), head)) != NULL) {
list_del_init(>head);
spin_unlock(>lock);
dma_fence_put(>base);
spin_lock(>lock);
}
Re: DRM/KMS: vmwgfx driver is now available
On 7/24/23 00:03, Taylor R Campbell wrote:
Date: Sun, 23 Jul 2023 22:57:14 +0900
From: PHO
On 7/23/23 21:24, Taylor R Campbell wrote:
Why can't the loop just use dma_fence_get_rcu and dma_fence_put?
Might need to release and reacquire the lock around dma_fence_put.
I tried but it didn't work, apparently because someone was holding a
pointer to a dying fence and waiting for it to be signaled. Using
dma_fence_get_rcu() meant we didn't signal such fences, and the kernel
hanged and eventually got killed by heartbeat().
What exactly did you try and what was the heartbeat panic stack trace?
Nothing here should wait for signalling in a way that trips the kernel
heartbeat monitor -- that should only happen if you hold onto a spin
lock for too long.
In __vmw_fences_update():
...
list_for_each_entry_safe(fence, next_fence, >fence_list, head) {
if (seqno - fence->base.seqno < VMW_FENCE_WRAP) {
list_del_init(>head);
struct dma_fence *f =
dma_fence_get_rcu(>base);
if (f) {
dma_fence_signal_locked(f);
spin_unlock(>lock);
dma_fence_put(f);
spin_lock(>lock);
}
INIT_LIST_HEAD(_list);
...
And the stack trace was this:
__kernel_end() at 0
sys_reboot() at sys_reboot+0x30
vpanic() at vpanic+0x1ad
printf_nostamp() at printf_nostamp+0x30
heartbeat() at heartbeat+0x21c
hardclock() at hardclock+0xbb
Xresume_lapic_ltimer() at Xresume_lapic_ltimer+0x1e
--- interrupt ---
mutex_vector_enter() at mutex_vector_enter+0x36f
vmw_fifo_send_fence() at vmw_fifo_send_fence+0xea
vmw_execbuf_fence_commands() at vmw_execbuf_fence_commands+0x3b
vmw_execbuf_process() at vmw_execbuf_process+0x91e
vmw_execbuf_ioctl() at vmw_execbuf_ioctl+0x97
drm_ioctl() at drm_ioctl+0x26d
drm_read() at drm_read+0x15
sys_ioctl() at sys_ioctl+0x59d
syscall() at syscall+0x196
--- syscall (number 54) ---
syscall+0x196:
vmw_fifo_send_fence+0xea was at vmwgfx_fifo.c:622:
spin_lock(_priv->fence_lock);
But let's not worry about this too much, as it doesn't occur in the
latest code (read below).
(That said: yay, the heartbeat monitor is catching what would
otherwise be a silent nonresponsive hang!)
Yup, it has helped me so many times.
1. Move vmw_fence_obj_destroy to an `RCU' callback so that we can
safely do dma_fence_put under the lock.
I can't see how to do it without interfering with dma_fence_free(), so
2. Create a list on the stack of signalled fences to put in
__vmw_fences_update and put them after the loop, outside the lock:
I tried this. Your code didn't work as-is because it could release the
same fence twice, but in the end I managed to get it work! Yay, now I
can withdraw my changes to linux_dma_fence.c:
static void __vmw_fences_update(struct vmw_fence_manager *fman)
{
...
struct list_head to_put = LIST_HEAD_INIT(to_put);
...
list_for_each_entry_safe(fence, next_fence, >fence_list, head) {
if (seqno - fence->base.seqno < VMW_FENCE_WRAP) {
list_del_init(>head);
if (dma_fence_get_rcu(>base) != NULL) {
list_add(>head, _put);
dma_fence_signal_locked(>base);
}
INIT_LIST_HEAD(_list);
list_splice_init(>seq_passed_actions,
_list);
vmw_fences_perform_actions(fman, _list);
} else
break;
}
...
if (!list_empty(_put)) {
spin_unlock(>lock);
list_for_each_entry_safe(fence, next_fence, _put, head) {
list_del_init(>head);
dma_fence_put(>base);
}
spin_lock(>lock);
}
}
Re: DRM/KMS: vmwgfx driver is now available
On 7/23/23 21:24, Taylor R Campbell wrote: Date: Sun, 23 Jul 2023 12:36:10 +0900 From: PHO This: https://github.com/depressed-pho/netbsd-src/blob/91daa67f17222da355d3fddd6fa849c786d9c545/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_fence.c#L45 and this: https://github.com/depressed-pho/netbsd-src/blob/91daa67f17222da355d3fddd6fa849c786d9c545/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_fence.c#L527 vma_fence_manager::fence_list holds pointers to all the fence objects in use but the list doesn't retain their refcount. I tried to change the way how it works by simply incrementing the refcount in vmw_fence_obj_init() and decrementing it in __vmw_fences_update(). But it didn't work well because fences could also be released by dma_resv_add_excl_fence() and dma_resv_add_shared_fence(). I cannot recall all the details but sometimes refcounts went down to zero even if they were retained by fence_list, and that's when I gave up fighting against it. I admit I don't fully understand the code. Why can't the loop just use dma_fence_get_rcu and dma_fence_put? Might need to release and reacquire the lock around dma_fence_put. I tried but it didn't work, apparently because someone was holding a pointer to a dying fence and waiting for it to be signaled. Using dma_fence_get_rcu() meant we didn't signal such fences, and the kernel hanged and eventually got killed by heartbeat(). If that doesn't work for some reason and we do have to add a special case, I'd strongly prefer to add a new front end like dma_fence_signal_vmwgfxspecialhack or something (other examples in the tasklet API) that has the relaxed assertion instead of relaxing the assertions for every other driver which isn't bonkers. Alright. I'll do that.
Re: Strange crash of DIAGNOSTIC kernel on cv_destroy(9)
On 7/22/23 22:41, Taylor R Campbell wrote:
Date: Sat, 22 Jul 2023 21:52:40 +0900
From: PHO
Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6151161]
vmw_fence_wait() at netbsd:vmw_fence_wait+0xdc
Just to confirm, what does `info line *(vmw_fence_wait+0xdc)' say in
gdb?
And, if you can get to the frame in gdb, what does gdb say is
in the vmw_fence_wait frame, and what cv is in the cv_destroy frame?
Let's confirm it is the cv you think it is -- I suspect it might be a
different one.
I just encountered the crash and could obtain a crash dump. It is indeed
the "DRM_DESTROY_WAITQUEUE()" in vmw_fence_wait() but the contents
of cb does not make sense to me:
-
(gdb) fr 6
#6 vmw_fence_wait (f=0xaa67d3170608, intr=,
timeout=)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_fence.c:289
289 DRM_DESTROY_WAITQUEUE();
(gdb) p cb
$11 = {base = {func = 0x814fbb80 , fcb_entry = {
tqe_next = 0x80df6a0b ,
tqe_prev = 0xaa675e629238}, fcb_onqueue = 90}, wq =
{cv_opaque = {
0x1, 0x80fa0189 }}}
(gdb) fr 4
#4 0x80dcdd20 in cv_destroy (cv=cv@entry=0xbf8138b44630)
at /home/pho/sandbox/_netbsd/src/sys/kern/kern_condvar.c:553
553 return !LIST_EMPTY(CV_SLEEPQ(cv));
(gdb) p *cv
$12 = {cv_opaque = {0x1, 0x80fa0189 }}
-
CV_SLEEPQ(cv) is 0x01 (wtf) and CV_WMESG(cv) is not even a string?
Here's the stack trace:
-
#0 0x80239c85 in cpu_reboot (howto=howto@entry=260,
bootstr=bootstr@entry=0x0)
at /home/pho/sandbox/_netbsd/src/sys/arch/amd64/amd64/machdep.c:717
#1 0x80e03cee in kern_reboot (howto=howto@entry=260,
bootstr=bootstr@entry=0x0)
at /home/pho/sandbox/_netbsd/src/sys/kern/kern_reboot.c:73
#2 0x80e4c698 in vpanic (
fmt=0x815a5cf8 "kernel %sassertion \"%s\" failed: file
\"%s\", line
%d ", ap=ap@entry=0xbf8138b44578)
at /home/pho/sandbox/_netbsd/src/sys/kern/subr_prf.c:292
#3 0x81012b8f in kern_assert (
fmt=fmt@entry=0x815a5cf8 "kernel %sassertion \"%s\" failed:
file \"%s\", line %d ")
at /home/pho/sandbox/_netbsd/src/sys/lib/libkern/kern_assert.c:51
#4 0xffff80dcdd20 in cv_destroy (cv=cv@entry=0xbf8138b44630)
at /home/pho/sandbox/_netbsd/src/sys/kern/kern_condvar.c:553
#5 0x80a72824 in DRM_DESTROY_WAITQUEUE (q=0xbf8138b44630)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/include/drm/drm_wait_netbsd.h:59
#6 vmw_fence_wait (f=0xaa67d3170608, intr=,
timeout=)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_fence.c:289
#7 0x80fa2184 in linux_dma_fence_wait_timeout (
fence=0xaa67d3170608, intr=intr@entry=false,
timeout=timeout@entry=1500)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/linux/linux_dma_fence.c:1005
#8 0x80fa4c96 in linux_dma_resv_wait_timeout_rcu (
robj=0xaa675e629238, shared=shared@entry=true,
intr=intr@entry=false,
timeout=)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/linux/linux_dma_resv.c:1213
#9 0x80fb5671 in ttm_bo_wait (bo=bo@entry=0xaa675e629170,
interruptible=interruptible@entry=false, no_wait=)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/ttm/ttm_bo.c:1896
#10 0x80a815d0 in vmw_resource_unbind_list (
vbo=vbo@entry=0xaa675e629170)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_resource.c:860
#11 0x80a6712c in vmw_bo_move_notify
(bo=bo@entry=0xffffaa675e629170,
mem=mem@entry=0xbf8138b44928)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_bo.c:1205
#12 0x80a8a6c7 in vmw_move_notify (bo=0xffffaa675e629170,
evict=, mem=0xbf8138b44928)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_ttm_buffer.c:933
#13 0x80fb3505 in ttm_bo_handle_move_mem (
bo=bo@entry=0xaa675e629170, mem=mem@entry=0xbf8138b44928,
evict=evict@entry=true, ctx=ctx@entry=0xbf8138b44b00)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/ttm/ttm_bo.c:380
#14 0x80fb4382 in ttm_bo_evict (ctx=0xbf8138b44b00,
bo=0xaa675e629170)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/ttm/ttm_bo.c:757
#15 ttm_mem_evict_first (bdev=bdev@entry=0xaa687f031008,
mem_type=,
place=place@entry=0x814fea78 ,
ctx=ctx@entry=0xffffbf8138b44b00,
ticket=ticket@entry=0xbf8138b44c98)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/ttm/ttm_bo.c:913
#16 0x80fb4850 in ttm_bo_mem_force_space (ctx=0xbf8138b44b00,
mem=0xbf8138b44a58, place=0x814fea78 ,
bo=0xaa6752dd4b30)
at
/home/pho/sandbox/_netbsd/src/sys/external/bsd/drm2/dist/drm/
Re: DRM/KMS: vmwgfx driver is now available
On 7/23/23 06:06, Taylor R Campbell wrote:
Date: Sun, 23 Jul 2023 00:47:39 +0900
From: PHO
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?
It's used for a command queue. Every time the driver wants to enqueue a
GPU command (like creating a texture, running shaders, notifying the GPU
of updated areas of a framebuffer, and so on) it needs to allocate some
memory, write the command and perform a DMA. So I believe applications
would call it like at least 100 times every second.
- 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 = >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.
Ah. That should work. Thanks for the tip.
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?
This:
https://github.com/depressed-pho/netbsd-src/blob/91daa67f17222da355d3fddd6fa849c786d9c545/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_fence.c#L45
and this:
https://github.com/depressed-pho/netbsd-src/blob/91daa67f17222da355d3fddd6fa849c786d9c545/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_fence.c#L527
vma_fence_manager::fence_list holds pointers to all the fence objects in
use but the list doesn't retain their refcount. I tried to change the
way how it works by simply incrementing the refcount in
vmw_fence_obj_init() and decrementing it in __vmw_fences_update(). But
it didn't work well because fences could also be released by
dma_resv_add_excl_fence() and dma_resv_add_shared_fence(). I cannot
recall all the details but sometimes refcounts went down to zero even if
they were retained by fence_list, and that's when I gave up fighting
against it. I admit I don't fully understand the code.
Re: DRM/KMS: vmwgfx driver is now available
On 7/23/23 00:09, Martin Husemann wrote: On Sat, Jul 22, 2023 at 01:23:39PM +, Taylor R Campbell wrote: 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. Don't we have a "shadow" thing that does a quick access framebuffer in main ram and causes (sometimes delayed) copy back to hardware for slower graphic cards? See the comments about "shadow framebuffer" in rasops.h (ri_hwbits vs. ri_bits). It is used e.g. in dev/sbus/cgtwelve.c. That still means we have to allocate the real framebuffer in VRAM doesn't it? Unfortunately we just can't do it in the case of vmwgfx.
Re: DRM/KMS: vmwgfx driver is now available
On 7/22/23 22:23, Taylor R Campbell wrote: Date: Sun, 16 Jul 2023 04:50:35 +0900 From: PHO On 7/16/23 04:19, Taylor R Campbell wrote: 1. Did you reimplement an allocator for the dma_pool_* API? Should it use vmem(9) instead or can that not handle the requests that it needs? Yes I reimplemented it but it's probably far from efficient. And I didn't know the existence of vmem! Lol, I should have used it. Could I talk you into converting it to use vmem(9), so we have fewer allocators to debug? Should be pretty easy, there's a couple other examples of vmem(9) in drm already: drm_vma_manager.c, drm_gem_cma_helper.c. Yeah, I will give it a try. 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. 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. 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. 3. Why all the logic in vmwgfxfb.c duplicating genfb? Why not just use genfb via drmfb, like the other major drm drivers use (i915, radeon, nouveau, amdgpu)? Because vmwgfx does not, and can not use drm_fb_helper. The helper assumes that you can just allocate a framebuffer in VRAM and map it to a virtual address. The VMware GPU appears to support this operation mode too, but the vmwgfx driver isn't capable of doing it. So I had to allocate a framebuffer in the system memory and ask the driver to upload dirty areas to the GPU every time something changes in the buffer. What a nightmare! 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. - In: /* Pretend we just completed a DMA. The device sometimes updates * the status field synchronously without an IRQ, and we need our * CPU to realize that. */ Seems suspicious. Do you mean that header->cb_header->status gets updated but the device doesn't subsequently deliver an interrupt? Yes, exactly that. This weird device really does it. - In: #if defined(__NetBSD__) /* XXX: This is the only mode we support at the moment. The other * modes will cause the driver to panic() because we don't have * ttm_pool_populate(). */ dev_priv->map_mode = vmw_dma_alloc_coherent; Can you make vmw_ttm_populate use ttm_bus_dma_populate more or less unconditionally like all the other drivers do? Okay, I'll do it. vmwgfx_drv.h: - Why is vmw_ttm_fault_reserve_notify here? Why isn't it static to vmwgfx_ttm_buffer.c? Can't remember why I did it. I'll move the function back to vmwgfx_ttm_buffer.c and make it static. - In: dev->driver->irq_handler = vmw_irq_handler; Why is this not statically initialized? dev->driver should really be const so the compiler rejects this. Can't remember why. Probably because vmw_irc_handler() was a static function? I'll fix it. vmwgfx_page_dirty.c: - `const struct ttm_buffer_object *bo', not `struct ttm_buffer_object const* bo' - In: return ttm_bo_uvm_fault(ufi, vaddr, pps, npages, centeridx, access_type, flags); Do we eve have to define vmw_bo_vm_fault? Let's just set vmw_uvm_ops.pgo_fault = ttm_bo_uvm_fault, to keep it simpler? Nope, we don't need to. I'll do it. vmwgfx_resource.c: - In: /* This looks wrong, doesn't it? But it's actually what the * original Linux code does. The tree
Re: Strange crash of DIAGNOSTIC kernel on cv_destroy(9)
n 7/22/23 22:31, Taylor R Campbell wrote: What exactly is the panic you see and the evidence when you see it? Stack trace, gdb print cb in crash dump? Wait, can we use gdb for examining the kernel dump? I thought gdb couldn't read it. Here's the stacktrace found in /var/log/message: Yep, you can use gdb to examine a crash dump: $ gdb ./netbsd.gdb (gdb) target kvm netbsd.123.core Wow, that actually worked. Thank you for the tip. I wish I knew this when I was porting the driver :D
Re: Strange crash of DIAGNOSTIC kernel on cv_destroy(9)
On 7/22/23 22:41, Taylor R Campbell wrote: Date: Sat, 22 Jul 2023 21:52:40 +0900 From: PHO Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6151161] vmw_fence_wait() at netbsd:vmw_fence_wait+0xdc Just to confirm, what does `info line *(vmw_fence_wait+0xdc)' say in gdb? And, if you can get to the frame in gdb, what does gdb say is in the vmw_fence_wait frame, and what cv is in the cv_destroy frame? Let's confirm it is the cv you think it is -- I suspect it might be a different one. Since I no longer have the exact kernel that crashed, I tried to reproduce the issue with the latest one I have (with my workaround backed out). But it seems like the panic doesn't occur anymore. I'll give you the info the next time it panics.
Re: Strange crash of DIAGNOSTIC kernel on cv_destroy(9)
On 7/22/23 20:48, Taylor R Campbell wrote: A note about one of the problems there: spin_unlock(f->lock); ret = dma_fence_add_callback(f, , vmwgfx_wait_cb); spin_lock(f->lock); #if defined(__NetBSD__) /* This is probably an upstream bug: there is a time window between * the call of vmw_fence_obj_signaled() above, and this * dma_fence_add_callback(). If the fence gets signaled during it * dma_fence_add_callback() returns -ENOENT, which is really not an * error condition. By the way, why the heck does dma_fence work in * this way? If a callback is being added but it lost the race, why * not just call it immediately as if it were just signaled? */ Not an upstream bug -- I introduced this bug when I patched the code that reached into the guts of what should be an opaque data structure for direct modification, to use drm_fence_add_callback instead. Need to look at the diff from upstream, not just the #ifdefs. Usually I use #ifdef __NetBSD__ to mark NetBSDisms separately from Linuxisms, and just patch the code when the patched code can use a common API that isn't any one OSism. In this case I don't even remember why I left any #ifdefs, was probably just working fast to make progress on a large code base, might have left the #ifdefs in for visual reference while I was editing the code and forgot to remove them. Could also simplify some of the lock/unlock cycles by doing that. Ah okay. I used #if defined(__NetBSD__) for everything needing any changes, and I assumed you did the same without actually checking the original code. cv_destroy(); // <-- Panics! It seldom panics on KASSERT(!cv_has_waiters(cv)) in cv_destroy() but not always. The panic seems to happen when cv_timedwait_sig() exits due to the timeout expiring before it gets signaled. Confused by `seldom panics on ... but not always' -- was that supposed to be `often panics on ... but not always', or is there a more frequent panic than KASSERT(!cv_has_waiters(cv))? I meant it didn't panic for most cases as if nothing wrong happened, but it occasionally panicked due to KASSERT(!cv_has_waiters(cv)). Sorry for my bad English. What exactly is the panic you see and the evidence when you see it? Stack trace, gdb print cb in crash dump? Wait, can we use gdb for examining the kernel dump? I thought gdb couldn't read it. Here's the stacktrace found in /var/log/message: Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.3652130] panic: kernel diagnostic assertion "!cv_has_waiters(cv)" failed: file "/home/pho/sandbox/_netbsd/src/sys/kern/kern_condvar.c", line 108 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.3782663] cpu0: Begin traceback... Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.5355447] vpanic() at netbsd:vpanic+0x173 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.5454410] kern_assert() at netbsd:kern_assert+0x4b Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.5551143] cv_destroy() at netbsd:cv_destroy+0x8a Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6151161] vmw_fence_wait() at netbsd:vmw_fence_wait+0xdc Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6151161] linux_dma_fence_wait_timeout() at netbsd:linux_dma_fence_wait_timeout+0x8b Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6151161] linux_dma_resv_wait_timeout_rcu() at netbsd:linux_dma_resv_wait_timeout_rcu+0xbe Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6251241] ttm_bo_wait() at netbsd:ttm_bo_wait+0x4c Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6251241] vmw_resource_unbind_list() at netbsd:vmw_resource_unbind_list+0x103 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6251241] vmw_move_notify() at netbsd:vmw_move_notify+0x16 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6351198] ttm_bo_handle_move_mem() at netbsd:ttm_bo_handle_move_mem+0xe6 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6451175] ttm_mem_evict_first() at netbsd:ttm_mem_evict_first+0x702 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6451175] ttm_bo_mem_space() at netbsd:ttm_bo_mem_space+0x21e Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6451175] ttm_bo_validate() at netbsd:ttm_bo_validate+0xe6 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6551178] vmw_validation_bo_validate_single() at netbsd:vmw_validation_bo_validate_single+0x93 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6551178] vmw_validation_bo_validate() at netbsd:vmw_validation_bo_validate+0xaa Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6551178] vmw_execbuf_process() at netbsd:vmw_execbuf_process+0x771 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6651177] vmw_execbuf_ioctl() at netbsd:vmw_execbuf_ioctl+0x97 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6651177] drm_ioctl() at netbsd:drm_ioctl+0x23d Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6751234] drm_ioctl_shim() at netbsd:drm_ioctl_shim+0x25 Jul 17 00:52:34 netbsd-current /netbsd: [ 64017.6751234] sys
Re: DRM/KMS: vmwgfx driver is now available
On 7/18/23 00:17, nia wrote: Cool. I've been recommending "-vga vmware" with QEMU/NVMM because it works quite well (even without acceleration on the NetBSD side) and offers the widest choice in resolutions. Does this affect QEMU/NVMM when used with that option? I haven't tested that. Theoretically it would give you accelerated 2D graphics (like a DMA-blit between host and guest), but I cannot say much without testing. Also there is a caveat right now. For some reason DDB stops working when the driver is attached. The screen just freezes and the only thing you can do is to type DDB commands blindly and check the dmesg after rebooting the guest. I have yet to discover the cause of this.
Strange crash of DIAGNOSTIC kernel on cv_destroy(9)
Hi,
While further testing my DRM/KMS vmwgfx driver patches by playing
games/minetest from pkgsrc, I experienced inexplicable kernel panics on
this code:
https://github.com/depressed-pho/netbsd-src/blob/91daa67f17222da355d3fddd6fa849c786d9c545/sys/external/bsd/drm2/dist/drm/vmwgfx/vmwgfx_fence.c#L289
Here is a simplified version of the function to illustrate what's going
on. The function is called with a process context:
kmutex_t mtx;
mutex_init(, MUTEX_DEFAULT, IPL_VM);
...
mutex_spin_enter();
kcondvar_t cv;
cv_init(, "whatever");
{{ Store and somewhere else so that a softint will later
signal the cv. }}
while (true) {
if (!...) {
cv_timedwait_sig(, , timeout);
}
}
{{ Ask the softint not to signal it anymore. }}
mutex_spin_exit();
cv_destroy(); // <-- Panics!
It seldom panics on KASSERT(!cv_has_waiters(cv)) in cv_destroy() but not
always. The panic seems to happen when cv_timedwait_sig() exits due to
the timeout expiring before it gets signaled.
In this particular case the only LWP that can possibly be put into the
sleep queue is curlwp, which should have been removed by
sleepq_timeout() before leaving cv_timedwait_sig(), yet CV_SLEEPQ(cv)
contains some totally unrelated LWPs and even invalid pointers to lwp_t
when it panics.
The implementation of cv_destroy(9) is as follows:
void
cv_destroy(kcondvar_t *cv)
{
sleepq_destroy(CV_SLEEPQ(cv));
#ifdef DIAGNOSTIC
KASSERT(cv_is_valid(cv));
KASSERT(!cv_has_waiters(cv));
CV_SET_WMESG(cv, deadcv);
#endif
}
Yes we are calling cv_has_waiters() without locking the mutex. I could
work around the panic by destroying the cv before unlocking it, but I
really can't see why this is problematic and why I can avoid the panic
by doing that. Does anyone have any clues on what's going on?
Re: DRM/KMS: vmwgfx driver is now available
On 7/16/23 04:19, Taylor R Campbell wrote: > Amazing! Cool, I'll take a look. Yes please! Some quick questions before I've looked much into details (so don't take the questions too seriously; the answers might be obvious and I might be barking up the wrong tree): 1. Did you reimplement an allocator for the dma_pool_* API? Should it use vmem(9) instead or can that not handle the requests that it needs? Yes I reimplemented it but it's probably far from efficient. And I didn't know the existence of vmem! Lol, I should have used it. 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. 3. Why all the logic in vmwgfxfb.c duplicating genfb? Why not just use genfb via drmfb, like the other major drm drivers use (i915, radeon, nouveau, amdgpu)? Because vmwgfx does not, and can not use drm_fb_helper. The helper assumes that you can just allocate a framebuffer in VRAM and map it to a virtual address. The VMware GPU appears to support this operation mode too, but the vmwgfx driver isn't capable of doing it. So I had to allocate a framebuffer in the system memory and ask the driver to upload dirty areas to the GPU every time something changes in the buffer.
Re: DRM/KMS: vmwgfx driver is now available
On 7/16/23 01:43, Robert Swindells wrote: PHO wrote: On 7/16/23 01:24, Robert Swindells wrote: PHO wrote: I've been working on vmwgfx driver, a driver for VMware virtualized GPU. It was partly ported to NetBSD but wasn't buildable, and now it builds and actually works. (Thank you riastradh@, your prior work helped tremendously. I don't think I could do this without it.) [snip] Note that you need to update your pkgsrc tree to try the driver, as I fixed a few things that needed fixing. Also the base X.org is not capable of making use of the driver because its Mesa is built without libxatracker. You need the modular one from pkgsrc. Which bits of Mesa does it need apart from xa? Only this change: https://mail-index.netbsd.org/pkgsrc-changes/2023/07/14/msg278732.html Which drivers or compiler backends does it use? x11/xf86-video-vmware is the only X.org driver that works with vmwgfx. It's built on top of Gallium3D. Is this what you're asking?
Re: DRM/KMS: vmwgfx driver is now available
On 7/16/23 01:24, Robert Swindells wrote: PHO wrote: I've been working on vmwgfx driver, a driver for VMware virtualized GPU. It was partly ported to NetBSD but wasn't buildable, and now it builds and actually works. (Thank you riastradh@, your prior work helped tremendously. I don't think I could do this without it.) [snip] Note that you need to update your pkgsrc tree to try the driver, as I fixed a few things that needed fixing. Also the base X.org is not capable of making use of the driver because its Mesa is built without libxatracker. You need the modular one from pkgsrc. Which bits of Mesa does it need apart from xa? Only this change: https://mail-index.netbsd.org/pkgsrc-changes/2023/07/14/msg278732.html
DRM/KMS: vmwgfx driver is now available
Hi, I've been working on vmwgfx driver, a driver for VMware virtualized GPU. It was partly ported to NetBSD but wasn't buildable, and now it builds and actually works. (Thank you riastradh@, your prior work helped tremendously. I don't think I could do this without it.) This is the list of patches I'd like to commit: https://github.com/NetBSD/src/compare/trunk...depressed-pho:netbsd-src:drm2-vmwgfx Here's what I tested: - benchmarks/glmark2 completes without crashing, and it tells me that the virtual GPU is 37x better than software rasterization. - wm/compiz seems to work, but since I had no prior experiences there might be broken things I didn't realize. - "picom --backend glx" works flawlessly in combination with wm/fluxbox. - x11/enlightenment works, but I don't know if it actually uses GLX. Note that you need to update your pkgsrc tree to try the driver, as I fixed a few things that needed fixing. Also the base X.org is not capable of making use of the driver because its Mesa is built without libxatracker. You need the modular one from pkgsrc.
Re: crash in timerfd building pandoc / ghc94 related
Committed a workaround to lang/ghc94. I hope it can avoid the panic. You can remove the workaround simply by deleting lang/ghc94/hacks.mk. On 2/7/23 12:36 AM, Greg Troxel wrote: PHO writes: On 2/6/23 5:27 PM, Nikita Ronja Gillmann wrote: I encountered this on some version of 10.99.2 and last night again on 10.99.2 from Friday morning. This is an obvious blocker for me for making 9.4.4 the default. I propose to either revert to the last version or make the default GHC version setable. I wish I could do the latter, but unfortunately not all Haskell packages are buildable with 2 major versions of GHC at the same time (most are, but there are a few exceptions). Alternatively, I think I can patch GHC 9.4 so that it won't use timerfd. It appears to be an optional feature after all; if its ./configure doesn't find timerfd it won't use it. Let me try that. If it's possible to only do this on NetBSD 10.99, that would be good. Yeah I did exactly that. It seems so far, from not really paying attention, that there is nothing wrong with ghc but that there is a bug in the kernel. It would also be good to get a reproduction recipe without haskell. Yes of course no userland code should be able to crash the kernel :D
Re: crash in timerfd building pandoc / ghc94 related
On 2/6/23 5:27 PM, Nikita Ronja Gillmann wrote: > I encountered this on some version of 10.99.2 and last night again on > 10.99.2 from Friday morning. > This is an obvious blocker for me for making 9.4.4 the default. > I propose to either revert to the last version or make the default GHC > version setable. I wish I could do the latter, but unfortunately not all Haskell packages are buildable with 2 major versions of GHC at the same time (most are, but there are a few exceptions). Alternatively, I think I can patch GHC 9.4 so that it won't use timerfd. It appears to be an optional feature after all; if its ./configure doesn't find timerfd it won't use it. Let me try that.
Re: crash in timerfd building pandoc / ghc94 related
On 2/6/23 8:54 AM, matthew green wrote:> hi folks.
>
>
> i saw a report about ghc94 related crashes, and while it's easy
> to build ghc94 itself, it's easy to trigger a crash by having
> packages use it. for me 'pandoc' wants a bunch of hs-* pkgs and
> i had crashes in 2 separate ones.
>
> i added some addditional logging to the failed assert to confirm
> what part of it is failing. here's the panic and stack:
>
> [ 2875.6028592] panic: kernel diagnostic assertion "c->c_cpu->cc_lwp
== curlwp || c->c_cpu->cc_active != c" failed: file
"/usr/src/sys/kern/kern_timeout.c", line 381 running callout
0xfaa403b50e00: c_func (0x80f53893) c_flags (0x100) c_active
(0xfaa403b50e00) cc_lwp (0xfab1b4bba080) destroyed from
0x80fa0d89
>
> breakpoint() at netbsd:breakpoint+0x5
> vpanic() at netbsd:vpanic+0x183
> kern_assert() at netbsd:kern_assert+0x4b
> callout_destroy() at netbsd:callout_destroy+0xbc
> timerfd_fop_close() at netbsd:timerfd_fop_close+0x36
> closef() at netbsd:closef+0x60
> fd_close() at netbsd:fd_close+0x138
> sys_close() at netbsd:sys_close+0x22
> syscall() at netbsd:syscall+0x196
> --- syscall (number 6) ---
>
>
> as you can see, "c_active" is "c", and cc_lwp is not curlwp, so
> the assert triggers. the active lwp is a softint thread:
>
> db{1}> bt/a 0xfab1b4bba080
> trace: pid 0 lid 5 at 0xa990969120e0
> softint_dispatch() at netbsd:softint_dispatch+0x1ba
> DDB lost frame for netbsd:Xsoftintr+0x4c, trying 0xa990969120f0
> Xsoftintr() at netbsd:Xsoftintr+0x4c
> --- interrupt ---
>
> this softint_dispatch() address is:
>
> (gdb) l *(softint_dispatch+0x1ba)
> 0x80f45c4b is in softint_dispatch
(/usr/src/sys/kern/kern_softint.c:623).
> 621 PSREF_DEBUG_BARRIER();
> 622
> 623 CPU_COUNT(CPU_COUNT_NSOFT, 1);
>
> and the actual address is a "test" instruction, so it seems that
> this lwp was interrupted by the panic and saved at this point of
> execution. so the assert is firing because the callout is both
> currently about to run _and_ being destroyed.
Thank you for your analysis. I tried to make a small test case to
reproduce the issue but so far without a success. This is what GHC 9.4
basically does:
https://gist.github.com/depressed-pho/5d117dbca872ef7c28ee7786e0ad8a8a
But this code does not trigger the panic.
Re: devel/gnustep-objc: Undefined references to libunwind symbols
Sorry, I didn't mean to post this here. On 9/8/22 6:46 PM, PHO wrote: Hello, devel/gnustep-objc currently doesn't build due to this linkage failure: http://www.ki.nu/pkgsrc/reports/current/NetBSD-9.0/20220904.2257/gnustep-objc-1.8.1/build.log The library does refer to symbols from LLVM libunwind without linking with -lunwind, but what confuses me is that exactly the same package built fine on Feb 25 2022. To my surprise libobjc at that time did contain definitions of those symbols even though the source code didn't. Does anyone have any clue what's going on? I can build the package with the attached diff, but I'm not sure if it's the correct way to fix it.
devel/gnustep-objc: Undefined references to libunwind symbols
Hello,
devel/gnustep-objc currently doesn't build due to this linkage failure:
http://www.ki.nu/pkgsrc/reports/current/NetBSD-9.0/20220904.2257/gnustep-objc-1.8.1/build.log
The library does refer to symbols from LLVM libunwind without linking
with -lunwind, but what confuses me is that exactly the same package
built fine on Feb 25 2022. To my surprise libobjc at that time did
contain definitions of those symbols even though the source code didn't.
Does anyone have any clue what's going on? I can build the package with
the attached diff, but I'm not sure if it's the correct way to fix it.
Index: Makefile
===
RCS file: /cvsroot/pkgsrc/devel/gnustep-objc/Makefile,v
retrieving revision 1.36
diff -u -u -r1.36 Makefile
--- Makefile 28 Nov 2021 15:45:13 - 1.36
+++ Makefile 8 Sep 2022 09:44:24 -
@@ -20,6 +20,10 @@
MAKE_ENV+= MAJOR_VERSION=4
MAKE_ENV+= MINOR_VERSION=6
+# libobjc uses symbols from LLVM libunwind (see ${WRKSRC}/unwind-itanium.h)
+# but CMakeLists.txt does not mention the library.
+LDFLAGS+= -L${PREFIX}/lib ${COMPILER_RPATH_FLAG}${PREFIX}/lib -lunwind
+
INSTALLATION_DIRS= lib include/objc
do-configure:
@@ -41,5 +45,6 @@
PLIST.objcxx= yes
.endif
+.include "../../lang/libunwind/buildlink3.mk"
.include "../../mk/pthread.buildlink3.mk"
.include "../../mk/bsd.pkg.mk"
Re: Hinted mmap(2) without MAP_FIXED can fail prematurely
I expected mmap(2) to search for an available region from the entire address space starting from the hint, not only half of it. On 2/17/22 5:32 PM, Johnny Billquist wrote: If it would ignore the hint, what's the point of the hint then? Johnny On 2022-02-17 08:23, PHO wrote: I'm not sure if this is a bug or not, but on NetBSD/amd64 9.2, hinted mmap(2) without MAP_FIXED can fail prematurely with ENOMEM if all the regions below the given hint address are occupied. The man page for mmap(2) suggests that the hint is just a hint. Shouldn't mmap(2) also search for available regions above the hint then? Test code: https://gist.github.com/depressed-pho/a629247b48b3e6178e35a14c62e9d44f#file-mmap-with-hint-c Test result: https://gist.github.com/depressed-pho/a629247b48b3e6178e35a14c62e9d44f#file-gistfile1-txt
Hinted mmap(2) without MAP_FIXED can fail prematurely
I'm not sure if this is a bug or not, but on NetBSD/amd64 9.2, hinted mmap(2) without MAP_FIXED can fail prematurely with ENOMEM if all the regions below the given hint address are occupied. The man page for mmap(2) suggests that the hint is just a hint. Shouldn't mmap(2) also search for available regions above the hint then? Test code: https://gist.github.com/depressed-pho/a629247b48b3e6178e35a14c62e9d44f#file-mmap-with-hint-c Test result: https://gist.github.com/depressed-pho/a629247b48b3e6178e35a14c62e9d44f#file-gistfile1-txt
Re: GHC 9.2.1 is now available as lang/ghc92
Sorry, I meant to post this to tech-pkg. On 2/6/22 2:43 PM, PHO wrote: Hi all, I just imported GHC 9.2.1 as lang/ghc92. One of the notable improvements since GHC 9.0 is that the compiler now has a native codegen for aarch64, which means LLVM is now optional on the architecture. After I upstream local patches it currently has, I'm going to update all of the Haskell packages so that we can eventually switch the default Haskell compiler to lang/ghc92. This will take several weeks though.
GHC 9.2.1 is now available as lang/ghc92
Hi all, I just imported GHC 9.2.1 as lang/ghc92. One of the notable improvements since GHC 9.0 is that the compiler now has a native codegen for aarch64, which means LLVM is now optional on the architecture. After I upstream local patches it currently has, I'm going to update all of the Haskell packages so that we can eventually switch the default Haskell compiler to lang/ghc92. This will take several weeks though.
