Hi Geoff, On Mon, Oct 16, 2017 at 8:31 PM, <ge...@hostfission.com> wrote: > Hi Yan & Ladi. > > I have written an initial implementation that supports just the shared > memory > mapping at this time. I plan to add events also but before I go further I > would > like some feedback if possible on what I have implemented thus far. > > Please see: > > https://github.com/gnif/kvm-guest-drivers-windows/commit/8655cf12fbdd77b991f96d97bc20f967b5907c12
Thank you, looks good overall. * Please don't use the 'vio' prefix for this driver. ivshmem is not a VirtIO device (i.e. not using the VirtIO protocol). Also the test program should live in a subdirectory, so maybe something like /ivshmem and /ivshmem/test. * In VIOIVSHMEMEvtDevicePrepareHardware: I don't think that Windows guarantees that resources are enumerated in BAR order. In VirtIO drivers we read the PCI config space to identify the BAR index: https://github.com/virtio-win/kvm-guest-drivers-windows/blob/master/VirtIO/VirtIOPCICommon.c#L353 * IOCTL codes on Windows have a structure to them: https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/defining-i-o-control-codes * In VIOIVSHMEMEvtIoDeviceControl: The "only one mapping at a time is allowed" test has a race. I think that simply making the IO queue WdfIoQueueDispatchSequential instead of WdfIoQueueDispatchParallel will fix it. * According to MSDN, MmMapLockedPagesSpecifyCache(UserMode) should be wrapped in try/except. Also, what happens if the file handle is inherited by a child process? Can it unmap the mapping in parent's address space? What if the parent exits? A possible solution is discussed in this article: http://www.osronline.com/article.cfm?article=39 Thanks! Ladi
