[PATCH] ntoskrnl.exe: Implement Ke(Initialize|Insert|Remove)DeviceQueue.

[Rémi Bernon]

Signed-off-by: Rémi Bernon 
---

MSDN says that the cancel spinlock has to be held whenever these device
queues are used, when setting cancel routines, but it's not clear to me
why, as IRP cancellation has apparently no effect on these functions
behavior (I initially thought they should check and not queue cancelled
IRPs, but it looks like they don't).

dlls/ntoskrnl.exe/ntoskrnl.exe.spec |  6 +-
 dlls/ntoskrnl.exe/sync.c            | 41 ++++++++++++
 dlls/ntoskrnl.exe/tests/driver.c    | 96 +++++++++++++++++++++++++++++
 include/ddk/wdm.h                   |  4 ++
 4 files changed, 144 insertions(+), 3 deletions(-)

diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
index 5fdaa922f45..c1244a42146 100644
--- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
+++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
@@ -567,7 +567,7 @@
 @ stub KeI386SetGdtSelector
 @ stub KeIcacheFlushCount
 @ stdcall KeInitializeApc(ptr ptr long ptr ptr ptr long ptr)
-@ stub KeInitializeDeviceQueue
+@ stdcall KeInitializeDeviceQueue(ptr)
 @ stdcall KeInitializeDpc(ptr ptr ptr)
 @ stdcall KeInitializeEvent(ptr long long)
 @ stub KeInitializeInterrupt
@@ -579,7 +579,7 @@
 @ stdcall KeInitializeTimer(ptr)
 @ stdcall KeInitializeTimerEx(ptr long)
 @ stub KeInsertByKeyDeviceQueue
-@ stub KeInsertDeviceQueue
+@ stdcall KeInsertDeviceQueue(ptr ptr)
 @ stub KeInsertHeadQueue
 @ stdcall KeInsertQueue(ptr ptr)
 @ stub KeInsertQueueApc
@@ -617,7 +617,7 @@
 @ stdcall KeReleaseSpinLockFromDpcLevel(ptr)
 @ stub KeRemoveByKeyDeviceQueue
 @ stub KeRemoveByKeyDeviceQueueIfBusy
-@ stub KeRemoveDeviceQueue
+@ stdcall KeRemoveDeviceQueue(ptr)
 @ stub KeRemoveEntryDeviceQueue
 @ stub KeRemoveQueue
 @ stub KeRemoveQueueDpc
diff --git a/dlls/ntoskrnl.exe/sync.c b/dlls/ntoskrnl.exe/sync.c
index 445c8d890ab..9063608be4a 100644
--- a/dlls/ntoskrnl.exe/sync.c
+++ b/dlls/ntoskrnl.exe/sync.c
@@ -1362,3 +1362,44 @@ BOOLEAN WINAPI KeSetTimer(KTIMER *timer, LARGE_INTEGER duetime, KDPC *dpc)

return KeSetTimerEx(timer, duetime, 0, dpc);
 }
+
+void WINAPI KeInitializeDeviceQueue( KDEVICE_QUEUE *queue )
+{
+    TRACE( "queue %p.\n", queue );
+
+    KeInitializeSpinLock( &queue->Lock );
+    InitializeListHead( &queue->DeviceListHead );
+    queue->Busy = FALSE;
+    queue->Type = IO_TYPE_DEVICE_QUEUE;
+    queue->Size = sizeof(*queue);
+}
+
+BOOLEAN WINAPI KeInsertDeviceQueue( KDEVICE_QUEUE *queue, KDEVICE_QUEUE_ENTRY *entry )
+{
+    KIRQL irql;
+
+    TRACE( "queue %p, entry %p.\n", queue, entry );
+
+    KeAcquireSpinLock( &queue->Lock, &irql );
+    if ((entry->Inserted = queue->Busy)) InsertTailList( &queue->DeviceListHead, &entry->DeviceListEntry );
+    queue->Busy = TRUE;
+    KeReleaseSpinLock( &queue->Lock, irql );
+
+    return entry->Inserted;
+}
+
+KDEVICE_QUEUE_ENTRY * WINAPI KeRemoveDeviceQueue( KDEVICE_QUEUE *queue )
+{
+    LIST_ENTRY *entry = NULL;
+    KIRQL irql;
+
+    TRACE( "queue %p.\n", queue );
+
+    KeAcquireSpinLock( &queue->Lock, &irql );
+    if (IsListEmpty( &queue->DeviceListHead )) queue->Busy = FALSE;
+    else entry = RemoveHeadList( &queue->DeviceListHead );
+    KeReleaseSpinLock( &queue->Lock, irql );
+
+    if (!entry) return NULL;
+    return CONTAINING_RECORD( entry, KDEVICE_QUEUE_ENTRY, DeviceListEntry );
+}
diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c
index f1c72d8fdaf..3446dc8e529 100644
--- a/dlls/ntoskrnl.exe/tests/driver.c
+++ b/dlls/ntoskrnl.exe/tests/driver.c
@@ -141,6 +141,101 @@ static void test_irp_struct(IRP *irp, DEVICE_OBJECT *device)
     IoFreeIrp(irp);
 }

+static int cancel_queue_cnt;
+
+static void WINAPI cancel_queued_irp(DEVICE_OBJECT *device, IRP *irp)
+{
+    IoReleaseCancelSpinLock(irp->CancelIrql);
+    ok(irp->Cancel == TRUE, "Cancel = %x\n", irp->Cancel);
+    ok(!irp->CancelRoutine, "CancelRoutine = %p\n", irp->CancelRoutine);
+    irp->IoStatus.Status = STATUS_CANCELLED;
+    irp->IoStatus.Information = 0;
+    cancel_queue_cnt++;
+}
+
+static void test_queue(void)
+{
+    KDEVICE_QUEUE_ENTRY *entry;
+    KDEVICE_QUEUE queue;
+    BOOLEAN ret;
+    KIRQL irql;
+    IRP *irp;
+
+    irp = IoAllocateIrp(1, FALSE);
+
+    memset(&queue, 0xcd, sizeof(queue));
+    KeInitializeDeviceQueue(&queue);
+    ok(!queue.Busy, "unexpected Busy state\n");
+    ok(queue.Size == sizeof(queue), "unexpected Size %x\n", queue.Size);
+    ok(queue.Type == IO_TYPE_DEVICE_QUEUE, "unexpected Type %x\n", queue.Type);
+
+    ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
+    ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n");
+    ok(queue.Busy, "expected Busy state\n");
+
+    entry = KeRemoveDeviceQueue(&queue);
+    ok(!entry, "expected KeRemoveDeviceQueue to return NULL\n");
+    ok(!queue.Busy, "unexpected Busy state\n");
+
+    ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
+    ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n");
+    ok(queue.Busy, "expected Busy state\n");
+
+    ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
+    ok(ret, "expected KeInsertDeviceQueue to insert IRP\n");
+    ok(queue.Busy, "expected Busy state\n");
+
+    entry = KeRemoveDeviceQueue(&queue);
+    ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n");
+    ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, "unexpected IRP returned\n");
+    ok(queue.Busy, "expected Busy state\n");
+
+    entry = KeRemoveDeviceQueue(&queue);
+    ok(entry == NULL, "expected KeRemoveDeviceQueue to return NULL\n");
+    ok(!queue.Busy, "unexpected Busy state\n");
+
+    IoCancelIrp(irp);
+    ok(irp->Cancel, "unexpected non-cancelled state\n");
+
+    ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
+    ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n");
+    ok(queue.Busy, "expected Busy state\n");
+    ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
+    ok(ret, "expected KeInsertDeviceQueue to insert IRP\n");
+    ok(queue.Busy, "expected Busy state\n");
+
+    entry = KeRemoveDeviceQueue(&queue);
+    ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n");
+    ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, "unexpected IRP returned\n");
+    ok(queue.Busy, "expected Busy state\n");
+    ok(irp->Cancel, "unexpected non-cancelled state\n");
+
+    IoFreeIrp(irp);
+
+    irp = IoAllocateIrp(1, FALSE);
+
+    IoAcquireCancelSpinLock(&irql);
+    IoSetCancelRoutine(irp, cancel_queued_irp);
+    IoReleaseCancelSpinLock(irql);
+
+    ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
+    ok(ret, "expected KeInsertDeviceQueue to insert IRP\n");
+    ok(queue.Busy, "expected Busy state\n");
+
+    IoCancelIrp(irp);
+    ok(irp->Cancel, "unexpected non-cancelled state\n");
+    ok(cancel_queue_cnt, "expected cancel routine to be called\n");
+
+    entry = KeRemoveDeviceQueue(&queue);
+    ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n");
+    ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, "unexpected IRP returned\n");
+    ok(irp->Cancel, "unexpected non-cancelled state\n");
+    ok(cancel_queue_cnt, "expected cancel routine to be called\n");
+    ok(queue.Busy, "expected Busy state\n");
+
+    IoFreeIrp(irp);
+}
+
 static void test_mdl_map(void)
 {
     char buffer[20] = "test buffer";
@@ -2126,6 +2221,7 @@ static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *st
     test_init_funcs();
     test_load_driver();
     test_sync();
+    test_queue();
     test_version();
     test_stack_callout();
     test_lookaside_list();
diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
index 447833c4bc9..9bc3305471c 100644
--- a/include/ddk/wdm.h
+++ b/include/ddk/wdm.h
@@ -397,6 +397,7 @@ typedef struct _WAIT_CONTEXT_BLOCK {
 #define IO_TYPE_ERROR_LOG               0x0b
 #define IO_TYPE_ERROR_MESSAGE           0x0c
 #define IO_TYPE_DEVICE_OBJECT_EXTENSION 0x0d
+#define IO_TYPE_DEVICE_QUEUE            0x14

typedef struct _DEVICE_OBJECT {
   CSHORT  Type;
@@ -1750,6 +1751,7 @@ void      WINAPI KeEnterCriticalRegion(void);
 void      WINAPI KeGenericCallDpc(PKDEFERRED_ROUTINE,PVOID);
 ULONG     WINAPI KeGetCurrentProcessorNumber(void);
 PKTHREAD  WINAPI KeGetCurrentThread(void);
+void      WINAPI KeInitializeDeviceQueue(KDEVICE_QUEUE*);
 void      WINAPI KeInitializeDpc(KDPC*,PKDEFERRED_ROUTINE,void*);
 void      WINAPI KeInitializeEvent(PRKEVENT,EVENT_TYPE,BOOLEAN);
 void      WINAPI KeInitializeMutex(PRKMUTEX,ULONG);
@@ -1757,6 +1759,7 @@ void      WINAPI KeInitializeSemaphore(PRKSEMAPHORE,LONG,LONG);
 void      WINAPI KeInitializeSpinLock(KSPIN_LOCK*);
 void      WINAPI KeInitializeTimerEx(PKTIMER,TIMER_TYPE);
 void      WINAPI KeInitializeTimer(KTIMER*);
+BOOLEAN   WINAPI KeInsertDeviceQueue(KDEVICE_QUEUE*,KDEVICE_QUEUE_ENTRY*);
 void      WINAPI KeLeaveCriticalRegion(void);
 ULONG     WINAPI KeQueryActiveProcessorCountEx(USHORT);
 KAFFINITY WINAPI KeQueryActiveProcessors(void);
@@ -1769,6 +1772,7 @@ LONG      WINAPI KeReleaseMutex(PRKMUTEX,BOOLEAN);
 LONG      WINAPI KeReleaseSemaphore(PRKSEMAPHORE,KPRIORITY,LONG,BOOLEAN);
 void      WINAPI KeReleaseSpinLock(KSPIN_LOCK*,KIRQL);
 void      WINAPI KeReleaseSpinLockFromDpcLevel(KSPIN_LOCK*);
+KDEVICE_QUEUE_ENTRY * WINAPI KeRemoveDeviceQueue(KDEVICE_QUEUE*);
 LONG      WINAPI KeResetEvent(PRKEVENT);
 void      WINAPI KeRevertToUserAffinityThread(void);
 void      WINAPI KeRevertToUserAffinityThreadEx(KAFFINITY affinity);

[Zebediah Figura (she/her)]

On 6/22/21 7:02 AM, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon
>
> MSDN says that the cancel spinlock has to be held whenever these device
> queues are used, when setting cancel routines, but it's not clear to me
> why, as IRP cancellation has apparently no effect on these functions
> behavior (I initially thought they should check and not queue cancelled
> IRPs, but it looks like they don't).

Where do you see this? I couldn't find any mention of it from a brief 
search.
> dlls/ntoskrnl.exe/ntoskrnl.exe.spec |  6 +-
>   dlls/ntoskrnl.exe/sync.c            | 41 ++++++++++++
>   dlls/ntoskrnl.exe/tests/driver.c    | 96 +++++++++++++++++++++++++++++
>   include/ddk/wdm.h                   |  4 ++
>   4 files changed, 144 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> index 5fdaa922f45..c1244a42146 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -567,7 +567,7 @@
>   @ stub KeI386SetGdtSelector
>   @ stub KeIcacheFlushCount
>   @ stdcall KeInitializeApc(ptr ptr long ptr ptr ptr long ptr)
> -@ stub KeInitializeDeviceQueue
> +@ stdcall KeInitializeDeviceQueue(ptr)
>   @ stdcall KeInitializeDpc(ptr ptr ptr)
>   @ stdcall KeInitializeEvent(ptr long long)
>   @ stub KeInitializeInterrupt
> @@ -579,7 +579,7 @@
>   @ stdcall KeInitializeTimer(ptr)
>   @ stdcall KeInitializeTimerEx(ptr long)
>   @ stub KeInsertByKeyDeviceQueue
> -@ stub KeInsertDeviceQueue
> +@ stdcall KeInsertDeviceQueue(ptr ptr)
>   @ stub KeInsertHeadQueue
>   @ stdcall KeInsertQueue(ptr ptr)
>   @ stub KeInsertQueueApc
> @@ -617,7 +617,7 @@
>   @ stdcall KeReleaseSpinLockFromDpcLevel(ptr)
>   @ stub KeRemoveByKeyDeviceQueue
>   @ stub KeRemoveByKeyDeviceQueueIfBusy
> -@ stub KeRemoveDeviceQueue
> +@ stdcall KeRemoveDeviceQueue(ptr)
>   @ stub KeRemoveEntryDeviceQueue
>   @ stub KeRemoveQueue
>   @ stub KeRemoveQueueDpc
> diff --git a/dlls/ntoskrnl.exe/sync.c b/dlls/ntoskrnl.exe/sync.c
> index 445c8d890ab..9063608be4a 100644
> --- a/dlls/ntoskrnl.exe/sync.c
> +++ b/dlls/ntoskrnl.exe/sync.c
> @@ -1362,3 +1362,44 @@ BOOLEAN WINAPI KeSetTimer(KTIMER *timer, LARGE_INTEGER duetime, KDPC *dpc)
>   return KeSetTimerEx(timer, duetime, 0, dpc);
>
> }
> -
>
> +void WINAPI KeInitializeDeviceQueue( KDEVICE_QUEUE *queue )
> +{
> - TRACE( "queue %p.\n", queue );
> -
> - KeInitializeSpinLock( &queue->Lock );
> - InitializeListHead( &queue->DeviceListHead );
> - queue->Busy = FALSE;
> - queue->Type = IO_TYPE_DEVICE_QUEUE;
> - queue->Size = sizeof(*queue);
>
> +}
> -
>
> +BOOLEAN WINAPI KeInsertDeviceQueue( KDEVICE_QUEUE *queue, KDEVICE_QUEUE_ENTRY *entry )
> +{
> - KIRQL irql;
> -
> - TRACE( "queue %p, entry %p.\n", queue, entry );
> -
> - KeAcquireSpinLock( &queue->Lock, &irql );
> - if ((entry->Inserted = queue->Busy)) InsertTailList( &queue->DeviceListHead, &entry->DeviceListEntry );
> - queue->Busy = TRUE;
> - KeReleaseSpinLock( &queue->Lock, irql );
> -
> - return entry->Inserted;
>
> +}
> -
>
> +KDEVICE_QUEUE_ENTRY * WINAPI KeRemoveDeviceQueue( KDEVICE_QUEUE *queue )
> +{
> - LIST_ENTRY *entry = NULL;
> - KIRQL irql;
> -
> - TRACE( "queue %p.\n", queue );
> -
> - KeAcquireSpinLock( &queue->Lock, &irql );
> - if (IsListEmpty( &queue->DeviceListHead )) queue->Busy = FALSE;
> - else entry = RemoveHeadList( &queue->DeviceListHead );
> - KeReleaseSpinLock( &queue->Lock, irql );
> -
> - if (!entry) return NULL;
> - return CONTAINING_RECORD( entry, KDEVICE_QUEUE_ENTRY, DeviceListEntry );
>
> +}
> diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c
> index f1c72d8fdaf..3446dc8e529 100644
> --- a/dlls/ntoskrnl.exe/tests/driver.c
> +++ b/dlls/ntoskrnl.exe/tests/driver.c
> @@ -141,6 +141,101 @@ static void test_irp_struct(IRP *irp, DEVICE_OBJECT *device)
>       IoFreeIrp(irp);
>   }
>
> +static int cancel_queue_cnt;
> -
>
> +static void WINAPI cancel_queued_irp(DEVICE_OBJECT *device, IRP *irp)
> +{
> - IoReleaseCancelSpinLock(irp->CancelIrql);
> - ok(irp->Cancel == TRUE, "Cancel = %x\n", irp->Cancel);
> - ok(!irp->CancelRoutine, "CancelRoutine = %p\n", irp->CancelRoutine);
> - irp->IoStatus.Status = STATUS_CANCELLED;
> - irp->IoStatus.Information = 0;
> - cancel_queue_cnt++;
>
> +}
> -
>
> +static void test_queue(void)
> +{
> - KDEVICE_QUEUE_ENTRY *entry;
> - KDEVICE_QUEUE queue;
> - BOOLEAN ret;
> - KIRQL irql;
> - IRP *irp;
> -
> - irp = IoAllocateIrp(1, FALSE);
> -
> - memset(&queue, 0xcd, sizeof(queue));
> - KeInitializeDeviceQueue(&queue);
> - ok(!queue.Busy, "unexpected Busy state\n");
> - ok(queue.Size == sizeof(queue), "unexpected Size %x\n", queue.Size);
> - ok(queue.Type == IO_TYPE_DEVICE_QUEUE, "unexpected Type %x\n", queue.Type);

Could we test the actual contents of the queue list after all these calls?
> -
> - ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
> - ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n");
> - ok(queue.Busy, "expected Busy state\n");
> -
> - entry = KeRemoveDeviceQueue(&queue);
> - ok(!entry, "expected KeRemoveDeviceQueue to return NULL\n");
> - ok(!queue.Busy, "unexpected Busy state\n");
> -
> - ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
> - ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n");
> - ok(queue.Busy, "expected Busy state\n");
> -
> - ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
> - ok(ret, "expected KeInsertDeviceQueue to insert IRP\n");
> - ok(queue.Busy, "expected Busy state\n");
> -
> - entry = KeRemoveDeviceQueue(&queue);
> - ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n");
> - ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, "unexpected IRP returned\n");
> - ok(queue.Busy, "expected Busy state\n");
> -
> - entry = KeRemoveDeviceQueue(&queue);
> - ok(entry == NULL, "expected KeRemoveDeviceQueue to return NULL\n");
> - ok(!queue.Busy, "unexpected Busy state\n");
> -
> - IoCancelIrp(irp);
> - ok(irp->Cancel, "unexpected non-cancelled state\n");
> -
> - ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
> - ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n");
> - ok(queue.Busy, "expected Busy state\n");
> - ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
> - ok(ret, "expected KeInsertDeviceQueue to insert IRP\n");
> - ok(queue.Busy, "expected Busy state\n");
> -
> - entry = KeRemoveDeviceQueue(&queue);
> - ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n");
> - ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, "unexpected IRP returned\n");
> - ok(queue.Busy, "expected Busy state\n");
> - ok(irp->Cancel, "unexpected non-cancelled state\n");
> -
> - IoFreeIrp(irp);
> -
> - irp = IoAllocateIrp(1, FALSE);
> -
> - IoAcquireCancelSpinLock(&irql);
> - IoSetCancelRoutine(irp, cancel_queued_irp);
> - IoReleaseCancelSpinLock(irql);
> -
> - ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
> - ok(ret, "expected KeInsertDeviceQueue to insert IRP\n");
> - ok(queue.Busy, "expected Busy state\n");
> -
> - IoCancelIrp(irp);
> - ok(irp->Cancel, "unexpected non-cancelled state\n");
> - ok(cancel_queue_cnt, "expected cancel routine to be called\n");
> -
> - entry = KeRemoveDeviceQueue(&queue);
> - ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n");
> - ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, "unexpected IRP returned\n");
> - ok(irp->Cancel, "unexpected non-cancelled state\n");
> - ok(cancel_queue_cnt, "expected cancel routine to be called\n");
> - ok(queue.Busy, "expected Busy state\n");
> -
> - IoFreeIrp(irp);
>
> +}
> - static void test_mdl_map(void)
> {
>     char buffer[20] = "test buffer";
>
> @@ -2126,6 +2221,7 @@ static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *st
>       test_init_funcs();
>       test_load_driver();
>       test_sync();
> - test_queue();
>  test_version();
>  test_stack_callout();
>  test_lookaside_list();
>
> diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
> index 447833c4bc9..9bc3305471c 100644
> --- a/include/ddk/wdm.h
> +++ b/include/ddk/wdm.h
> @@ -397,6 +397,7 @@ typedef struct _WAIT_CONTEXT_BLOCK {
>   #define IO_TYPE_ERROR_LOG               0x0b
>   #define IO_TYPE_ERROR_MESSAGE           0x0c
>   #define IO_TYPE_DEVICE_OBJECT_EXTENSION 0x0d
> +#define IO_TYPE_DEVICE_QUEUE            0x14
>
> typedef struct _DEVICE_OBJECT {
>     CSHORT  Type;
> @@ -1750,6 +1751,7 @@ void      WINAPI KeEnterCriticalRegion(void);
>   void      WINAPI KeGenericCallDpc(PKDEFERRED_ROUTINE,PVOID);
>   ULONG     WINAPI KeGetCurrentProcessorNumber(void);
>   PKTHREAD  WINAPI KeGetCurrentThread(void);
> +void      WINAPI KeInitializeDeviceQueue(KDEVICE_QUEUE*);
>   void      WINAPI KeInitializeDpc(KDPC*,PKDEFERRED_ROUTINE,void*);
>   void      WINAPI KeInitializeEvent(PRKEVENT,EVENT_TYPE,BOOLEAN);
>   void      WINAPI KeInitializeMutex(PRKMUTEX,ULONG);
> @@ -1757,6 +1759,7 @@ void      WINAPI KeInitializeSemaphore(PRKSEMAPHORE,LONG,LONG);
>   void      WINAPI KeInitializeSpinLock(KSPIN_LOCK*);
>   void      WINAPI KeInitializeTimerEx(PKTIMER,TIMER_TYPE);
>   void      WINAPI KeInitializeTimer(KTIMER*);
> +BOOLEAN   WINAPI KeInsertDeviceQueue(KDEVICE_QUEUE*,KDEVICE_QUEUE_ENTRY*);
>   void      WINAPI KeLeaveCriticalRegion(void);
>   ULONG     WINAPI KeQueryActiveProcessorCountEx(USHORT);
>   KAFFINITY WINAPI KeQueryActiveProcessors(void);
> @@ -1769,6 +1772,7 @@ LONG      WINAPI KeReleaseMutex(PRKMUTEX,BOOLEAN);
>   LONG      WINAPI KeReleaseSemaphore(PRKSEMAPHORE,KPRIORITY,LONG,BOOLEAN);
>   void      WINAPI KeReleaseSpinLock(KSPIN_LOCK*,KIRQL);
>   void      WINAPI KeReleaseSpinLockFromDpcLevel(KSPIN_LOCK*);
> +KDEVICE_QUEUE_ENTRY * WINAPI KeRemoveDeviceQueue(KDEVICE_QUEUE*);
>   LONG      WINAPI KeResetEvent(PRKEVENT);
>   void      WINAPI KeRevertToUserAffinityThread(void);
>   void      WINAPI KeRevertToUserAffinityThreadEx(KAFFINITY affinity);

[Rémi Bernon]

On 6/22/21 5:20 PM, Zebediah Figura (she/her) wrote:
> On 6/22/21 7:02 AM, Rémi Bernon wrote:
> > Signed-off-by: Rémi Bernon
> >
> > MSDN says that the cancel spinlock has to be held whenever these device
> > queues are used, when setting cancel routines, but it's not clear to me
> > why, as IRP cancellation has apparently no effect on these functions
> > behavior (I initially thought they should check and not queue cancelled
> > IRPs, but it looks like they don't).
>
> Where do you see this? I couldn't find any mention of it from a brief 
> search.

For instance [1]:

"A driver must hold the system cancel spin lock when calling this 
routine if the driver uses the I/O manager-supplied device queue in the 
device object. The driver runs at IRQL = DISPATCH_LEVEL after calling 
IoAcquireCancelSpinLock until it releases the cancel spin lock with 
IoReleaseCancelSpinLock."

[1] 
https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-ios...

I understand the "I/O manager-supplied device queue in the device 
object" is the "KDEVICE_QUEUE DeviceQueue" member that I wanted to use 
with these functions instead of re-implementing a queue and a lock again.
> > -
> >
> > +static void test_queue(void)
> > +{
> > +    KDEVICE_QUEUE_ENTRY *entry;
> > +    KDEVICE_QUEUE queue;
> > +    BOOLEAN ret;
> > +    KIRQL irql;
> > +    IRP *irp;
> > -
> >
> > +    irp = IoAllocateIrp(1, FALSE);
> > -
> >
> > +    memset(&queue, 0xcd, sizeof(queue));
> > +    KeInitializeDeviceQueue(&queue);
> > +    ok(!queue.Busy, "unexpected Busy state\n");
> > +    ok(queue.Size == sizeof(queue), "unexpected Size %x\n", queue.Size);
> > +    ok(queue.Type == IO_TYPE_DEVICE_QUEUE, "unexpected Type %x\n", 
> > queue.Type);
>
> Could we test the actual contents of the queue list after all these calls?

I guess. It's hopefully going to be empty?

[Zebediah Figura (she/her)]

On 6/22/21 10:29 AM, Rémi Bernon wrote:
> On 6/22/21 5:20 PM, Zebediah Figura (she/her) wrote:
> > On 6/22/21 7:02 AM, Rémi Bernon wrote:
> > > Signed-off-by: Rémi Bernon
> > >
> > > MSDN says that the cancel spinlock has to be held whenever these device
> > > queues are used, when setting cancel routines, but it's not clear to me
> > > why, as IRP cancellation has apparently no effect on these functions
> > > behavior (I initially thought they should check and not queue cancelled
> > > IRPs, but it looks like they don't).
> >
> > Where do you see this? I couldn't find any mention of it from a brief
> > search.
>
> For instance [1]:
>
> "A driver must hold the system cancel spin lock when calling this
> routine if the driver uses the I/O manager-supplied device queue in the
> device object. The driver runs at IRQL = DISPATCH_LEVEL after calling
> IoAcquireCancelSpinLock until it releases the cancel spin lock with
> IoReleaseCancelSpinLock."
>
> [1]
> https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-ios...
>
> I understand the "I/O manager-supplied device queue in the device
> object" is the "KDEVICE_QUEUE DeviceQueue" member that I wanted to use
> with these functions instead of re-implementing a queue and a lock again.

Based on [2] and [3] I'm guessing the device queue functions never 
acquire the lock, but rather the point is to synchronize with dequeuing 
and cancelling. I can't figure out why it makes a difference, though...

It doesn't seem obvious to me that using KDEVICE_QUEUEs in Wine makes 
our job any easier, fwiw. Especially the part where you can't actually 
queue the first pending IRP. I'm not sure it's meant to handle those 
kinds of pending IRPs, but then it's not really clear how these things 
are supposed to be used; there are no code samples anywhere.

[2] 
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/cancel-rout...

[3] 
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/synchronizi...
> > > -
> > >
> > > +static void test_queue(void)
> > > +{
> > > +    KDEVICE_QUEUE_ENTRY *entry;
> > > +    KDEVICE_QUEUE queue;
> > > +    BOOLEAN ret;
> > > +    KIRQL irql;
> > > +    IRP *irp;
> > > -
> > >
> > > +    irp = IoAllocateIrp(1, FALSE);
> > > -
> > >
> > > +    memset(&queue, 0xcd, sizeof(queue));
> > > +    KeInitializeDeviceQueue(&queue);
> > > +    ok(!queue.Busy, "unexpected Busy state\n");
> > > +    ok(queue.Size == sizeof(queue), "unexpected Size %x\n", queue.Size);
> > > +    ok(queue.Type == IO_TYPE_DEVICE_QUEUE, "unexpected Type %x\n",
> > > queue.Type);
> >
> > Could we test the actual contents of the queue list after all these calls?
>
> I guess. It's hopefully going to be empty?

Empty after this call, but not after an eventual KeInsertDeviceQueue 
call; I think it's worth testing the state after all of the Ke* calls below.

[Rémi Bernon]

On 6/22/21 6:14 PM, Zebediah Figura (she/her) wrote:
> On 6/22/21 10:29 AM, Rémi Bernon wrote:
> > On 6/22/21 5:20 PM, Zebediah Figura (she/her) wrote:
> > > On 6/22/21 7:02 AM, Rémi Bernon wrote:
> > > > Signed-off-by: Rémi Bernon
> > > >
> > > > MSDN says that the cancel spinlock has to be held whenever these device
> > > > queues are used, when setting cancel routines, but it's not clear to me
> > > > why, as IRP cancellation has apparently no effect on these functions
> > > > behavior (I initially thought they should check and not queue cancelled
> > > > IRPs, but it looks like they don't).
> > >
> > > Where do you see this? I couldn't find any mention of it from a brief
> > > search.
> >
> > For instance [1]:
> >
> > "A driver must hold the system cancel spin lock when calling this
> > routine if the driver uses the I/O manager-supplied device queue in the
> > device object. The driver runs at IRQL = DISPATCH_LEVEL after calling
> > IoAcquireCancelSpinLock until it releases the cancel spin lock with
> > IoReleaseCancelSpinLock."
> >
> > [1]
> > https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-ios...
> >
> > I understand the "I/O manager-supplied device queue in the device
> > object" is the "KDEVICE_QUEUE DeviceQueue" member that I wanted to use
> > with these functions instead of re-implementing a queue and a lock again.
>
> Based on [2] and [3] I'm guessing the device queue functions never 
> acquire the lock, but rather the point is to synchronize with dequeuing 
> and cancelling. I can't figure out why it makes a difference, though...
>
> It doesn't seem obvious to me that using KDEVICE_QUEUEs in Wine makes 
> our job any easier, fwiw. Especially the part where you can't actually 
> queue the first pending IRP. I'm not sure it's meant to handle those 
> kinds of pending IRPs, but then it's not really clear how these things 
> are supposed to be used; there are no code samples anywhere.

It probably also has something to do with IoStart(Next)Packet which need 
to handle cancellation.

Even when not using these, and even if queue-ing an IRP needs a loop I 
think using the device queue directly makes the code slightly simpler as 
it doesn't need locking.

It also saves a dedicated irp_queue and irp_queue_lock members which are 
already in the device object, and currently unused.
> > > > -
> > > >
> > > > +static void test_queue(void)
> > > > +{
> > > > +    KDEVICE_QUEUE_ENTRY *entry;
> > > > +    KDEVICE_QUEUE queue;
> > > > +    BOOLEAN ret;
> > > > +    KIRQL irql;
> > > > +    IRP *irp;
> > > > -
> > > >
> > > > +    irp = IoAllocateIrp(1, FALSE);
> > > > -
> > > >
> > > > +    memset(&queue, 0xcd, sizeof(queue));
> > > > +    KeInitializeDeviceQueue(&queue);
> > > > +    ok(!queue.Busy, "unexpected Busy state\n");
> > > > +    ok(queue.Size == sizeof(queue), "unexpected Size %x\n", 
> > > > queue.Size);
> > > > +    ok(queue.Type == IO_TYPE_DEVICE_QUEUE, "unexpected Type %x\n",
> > > > queue.Type);
> > >
> > > Could we test the actual contents of the queue list after all these 
> > > calls?
> >
> > I guess. It's hopefully going to be empty?
>
> Empty after this call, but not after an eventual KeInsertDeviceQueue 
> call; I think it's worth testing the state after all of the Ke* calls 
> below.

Yeah I figured, I'll add more tests.