Implementation of a Windows IVSHMEM driver #174

gnif · 2017-10-19T08:52:31Z

This patch set implements a windows driver for the ivshmem device allowing a user space application to map shared memory into it's context, ring doorbells and wait on IRQ events.

For more information please see:
http://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03354.html

This is an initial implementation of a windows driver for the Inter-VM Shared Memory device. At current this only exposes the shared memory region (BAR2) to user mode by means of DeviceIoControl calls to the device (See test application for example usage). Limitations: * Only one IVSHMEM device is supported, additional devices will be ignored. * Only one user mode application may hold the mapping at a time and may not open the mapping more then once. The driver will cleanup if the user mode application crashes or fails to unmap the memory before termination. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Rev 0 devices have an additional BAR for MSI-X, and IRQ resources These are currently not implemented, but if the device is configured this way the shared memory functionallity will still correctly work Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

This also applies fixes suggested by Ladi Prosek to address the following: * IOCTL codes corrected to use the correct windows structure * Corrected possible race condition by switching the queue from Parallel to Sequential. * Added an exception handler to MmMapLockedPagesSpecifyCache as per Microsoft documentation. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Also moved the test project into a subdirectory of the driver Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

It is now possible to create events and pass them to the driver using IOCTL_IVSHMEM_REGISTER_EVENT, these events will be set when the the specified interrupt arrives. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

If the IRQ configuration failed the count of used IRQs would be out by one which was causing a BUGCHECK on driver unload. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

While this works fine under Windows 10 Creators Edition, earlier versions fail the call to WdfInterruptCreate with the status STATUS_INVALID_PARAMETER. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

YanVugenfirer · 2017-10-19T13:44:26Z

Hi,

Thanks a lot!

Could you add the build of the driver and test application to build.bat in the root directory (build.bat and other drivers directories)? In this way Appveyor CI will test the build of the ivshmem driver and test app as well.

Thanks,
Yan.

These spin locks were required when the interrupt isr was running at the passive level, since we switched back to running at DIRQL the locks are not needed, in fact, they actually can and do cause a deadlock. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

gnif · 2017-10-19T15:03:05Z

@YanVugenfirer Thanks & done ;).

Please note that working with visual studio is not my strong suit, I can see that there are more changes needed with the clean scripts and the likes but I am unsure what needs to be changed as these scripts perform tasks based on the build directories which I have also likely not set correctly.

YanVugenfirer · 2017-10-19T15:52:02Z

On Oct 19, 2017, at 18:03, Geoffrey McRae ***@***.***> wrote: @YanVugenfirer Thanks & done ;). Please note that working with visual studio is not my strong suit, I can see that there are more changes needed with the clean scripts and the likes but I am unsure what needs to be changed as these scripts perform tasks based on the build directories which I have also likely not set correctly.

No problem. I can adjust clean scripts later. For now I would like to have the driver in the main buildall.bat so CI will pick it up.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

This reverts commit d46e4bc. This assumption is inaccurate, there is a locking issue here still but this is not the solution. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Implemented proper deferred ISR to process the interrupts without deadlocking the kernel. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

gnif · 2017-10-19T18:41:48Z

With that last change set it is running flawless, all known issues have been resolved.

Changed the device and queue to run at a passive level and fixed the use of KeAcquireSpinLock in the DPC which should have been KeAcquireSpinLockAtDpcLevel. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

According to MSDN allocating memory dynamically like this for this purpose is to be avoided. Also freeing the allocated memory in the InterruptDPC is dangerous. This patch switches to using a block of events in the device context. It also adds cleanup of events in preperation of adding support for continually running events. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

It is now possible to register an event that is not removed from the event list each time it is triggered. This avoids a spin lock just to install the event if it is continually being reused. The down side to this is currently the driver can not tell if the event has been released. Releasing a registered event will not cause any problems as the driver holds a reference to the object, but it will use up a slot until the user mode application closes it's handle to the device which will trigger a cleanup of it's event objects. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

YanVugenfirer · 2017-10-20T20:04:02Z

Merging.

One comment for next commits - please separate cosmetics (indentations for example) from functional changes

YanVugenfirer · 2017-10-20T20:04:22Z

Thanks a lot!

gnif · 2017-10-20T22:46:55Z

Noted for future. Thanks! How soon can I expect to see a build containing this driver? I require it for a production system that can not run in test signing mode.

YanVugenfirer · 2017-10-22T08:11:39Z

I need to check when we plan to release next version. In any case, first, the driver will go through our internal test pipe and we will check if it passes HCK.

gnif · 2017-10-22T12:42:59Z

Thanks, I appreciate it.

ladipro

Thanks again for posting this!

I have added a few inline comments. Most are nits and suggestions but I believe that there's also a bug or two.

ladipro · 2017-10-23T06:34:27Z

ivshmem/Device.c

+
+	if (deviceContext->interruptCount > 0)
+	{
+		deviceContext->interrupts = (WDFINTERRUPT*)MmAllocateNonCachedMemory(


Any reason to allocate non-cached memory here? ExAllocatePoolWithTag(NonPagedPool, ...) would likely be a better choice.

Lack of experience :)

ladipro · 2017-10-23T06:35:03Z

ivshmem/Device.c

+		if (!deviceContext->interrupts)
+		{
+			DEBUG_ERROR("Failed to allocate space for %d interrupts", deviceContext->interrupts);
+			return STATUS_DEVICE_CONFIGURATION_ERROR;


STATUS_INSUFFICIENT_RESOURCES is a more appropriate error in this case.

ladipro · 2017-10-23T06:41:12Z

ivshmem/Device.c

+            }
+
+            if (++deviceContext->interruptsUsed == 64)
+              DEBUG_INFO("%s", "This driver does not support > 64 interrupts, they will be ignored in the ISR.");


nit: This will be printed after creating 64 interrupts, not "> 64 interrupts".

This is correct, it only needs to warn once.

Yes, but it incorrectly warns if the # of interrupts is 64. It probably should print the message when it finds the 65th interrupt. Hope I didn't misunderstand.

Ah, I understand, it's out by one.

ladipro · 2017-10-23T06:44:17Z

ivshmem/Device.c

+                if (!NT_SUCCESS(MmAllocateMdlForIoSpace(&deviceContext->shmemAddr, 1, &deviceContext->shmemMDL)))
+                {
+                    DEBUG_ERROR("%s", "Call to MmAllocateMdlForIoSpace failed");
+                    result = STATUS_DEVICE_HARDWARE_ERROR;


I would propagate the result of MmAllocateMdlForIoSpace up instead of hardcoding STATUS_DEVICE_HARDWARE_ERROR.

ladipro · 2017-10-23T06:46:19Z

ivshmem/Device.c

+    }
+
+    if (memIndex == 0)
+        result = STATUS_DEVICE_HARDWARE_ERROR;


This may override the result as set by the loop above. Also, deviceContext->shmemMDL should probably also be checked for non-NULL after the loop to make sure that that part of the code has run and all BARs have been found.

ladipro · 2017-10-23T07:32:39Z

ivshmem/Queue.c

+            }
+
+            IVSHMEM_PEERID *out = NULL;
+            if (!NT_SUCCESS(WdfRequestRetrieveOutputBuffer(Request, OutputBufferLength, (PVOID)&out, NULL)))


nit: (PVOID *)&out

ladipro · 2017-10-23T07:41:06Z

virtio-win.sln

 		WinXP Release|x64 = WinXP Release|x64
 		WinXP Release|x86 = WinXP Release|x86
 	EndGlobalSection
 	GlobalSection(ProjectConfigurationPlatforms) = postSolution
+		{01D87C47-437A-4A16-8FD9-33FA5C99339E}.Debug|ARM.ActiveCfg = Win8.1 Release|Win32


I'd prefer all these changes adding ARM targets to be reverted as we don't support ARM yet.

Adding ARM targets was in error, I will remove these

ladipro · 2017-10-23T07:43:02Z

ivshmem/Queue.c

+            }
+
+            // early non locked quick check to see if we are out of event space
+            if (deviceContext->eventBufferUsed == MAX_EVENTS)


I wouldn't try to optimize an error path.

This is done here to avoid holding the spin lock unnecessarily and thus holding up the ISR.

ladipro · 2017-10-23T07:53:38Z

ivshmem/Device.h

+    LONG64                     pendingISR;              // flags for ISRs pending processing
+
+    KSPIN_LOCK                 eventListLock;           // spinlock for the below event list
+    IVSHMEMEventListEntry      eventBuffer[MAX_EVENTS]; // buffer of pre-allocated events


(optional ideas):
Using WdfLookasideListCreate / WdfMemoryCreateFromLookaside might be slightly nicer as we would be able to get rid of the MAX_EVENTS limit and the loop that searches for a free slot.

Also, WDF allows you to add a driver-defined context to interrupt objects. I wonder if maybe having the list of pending events attached to each interrupt like this wouldn't be better (no need to loop over all pending events in the DPC). Removing the pendingISR field and running DPC per-interrupt would also eliminate the other artificial limit of 64 interrupts.

Great idea, I will update to implement this!

ladipro · 2017-10-23T08:04:29Z

ivshmem/Device.c

+    deviceContext = DeviceGetContext(device);
+
+    pending = InterlockedExchange64(&deviceContext->pendingISR, 0);
+


if (pending == 0) return;

gnif added 19 commits October 19, 2017 01:59

Added the ivshmem projects to the solution

61843bd

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] Renamed from VIOIVSHMEM to IVSHMEM

a384aee

Also moved the test project into a subdirectory of the driver Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] Ensure bytesReturned is valid

c1d0c28

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] Added interrupt support for events

3b79b63

It is now possible to create events and pass them to the driver using IOCTL_IVSHMEM_REGISTER_EVENT, these events will be set when the the specified interrupt arrives. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] Added check for event list item allocation

6add831

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] cosmetics

a5817f0

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] Fixed issue with rev 0 devices failing to init

dd31e76

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] removed dependency on WPP

ab462bc

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] fixed out by one bug

b289510

If the IRQ configuration failed the count of used IRQs would be out by one which was causing a BUGCHECK on driver unload. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem/vcproj] Fixed output paths of ivshmem

b427395

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] PassiveHandling is incorrect

cfa1212

While this works fine under Windows 10 Creators Edition, earlier versions fail the call to WdfInterruptCreate with the status STATUS_INVALID_PARAMETER. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] corrected the manufacturer name

0f6c80f

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] marked the device registers as volatile

fedea0a

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] use WRITE_REGISTER_ULONG to update the doorbell register

0ad4b48

[ivshmem] Added support for different struct passed from 32 bit

1b4f484

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem/vcproj] fixed 32bit build of test application

bed7ea8

gnif added 2 commits October 20, 2017 01:49

[ivshmem] added ivshmem and test app to buildAll.bat

0e9862c

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

gnif added 2 commits October 20, 2017 03:01

Revert "[ivshmem] removed inappropriate use of spin locks"

47e727a

This reverts commit d46e4bc. This assumption is inaccurate, there is a locking issue here still but this is not the solution. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] fixed incorrect usage of spinlocks

c6c7991

Implemented proper deferred ISR to process the interrupts without deadlocking the kernel. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

gnif added 3 commits October 20, 2017 11:07

[ivshmem] fixed IRQL run levels throughout

44f4188

Changed the device and queue to run at a passive level and fixed the use of KeAcquireSpinLock in the DPC which should have been KeAcquireSpinLockAtDpcLevel. Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

[ivshmem] fixed warnings raised by the SDV

1948c13

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>

YanVugenfirer merged commit 7b81cbd into virtio-win:master Oct 20, 2017

ladipro reviewed Oct 23, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of a Windows IVSHMEM driver #174

Implementation of a Windows IVSHMEM driver #174

gnif commented Oct 19, 2017

YanVugenfirer commented Oct 19, 2017

gnif commented Oct 19, 2017

YanVugenfirer commented Oct 19, 2017 via email

gnif commented Oct 19, 2017

YanVugenfirer commented Oct 20, 2017

YanVugenfirer commented Oct 20, 2017

gnif commented Oct 20, 2017

YanVugenfirer commented Oct 22, 2017

gnif commented Oct 22, 2017

ladipro left a comment

ladipro Oct 23, 2017

gnif Oct 23, 2017

ladipro Oct 23, 2017

ladipro Oct 23, 2017

gnif Oct 23, 2017

ladipro Oct 23, 2017

gnif Oct 23, 2017

ladipro Oct 23, 2017

ladipro Oct 23, 2017

ladipro Oct 23, 2017

ladipro Oct 23, 2017

gnif Oct 23, 2017

ladipro Oct 23, 2017

gnif Oct 23, 2017

ladipro Oct 23, 2017

gnif Oct 23, 2017

ladipro Oct 23, 2017

		deviceContext = DeviceGetContext(device);

		pending = InterlockedExchange64(&deviceContext->pendingISR, 0);

Implementation of a Windows IVSHMEM driver #174

Implementation of a Windows IVSHMEM driver #174

Conversation

gnif commented Oct 19, 2017

YanVugenfirer commented Oct 19, 2017

gnif commented Oct 19, 2017

YanVugenfirer commented Oct 19, 2017 via email

gnif commented Oct 19, 2017

YanVugenfirer commented Oct 20, 2017

YanVugenfirer commented Oct 20, 2017

gnif commented Oct 20, 2017

YanVugenfirer commented Oct 22, 2017

gnif commented Oct 22, 2017

ladipro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment