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

[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 <rbernon at codeweavers.com>
>>>> ---
>>>>
>>>> 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-iosetcancelroutine 
>>
>>
>> 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.
-- 
Rémi Bernon <rbernon at codeweavers.com>