On 2017-10-18 16:31, Ladi Prosek wrote:
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.
Noted, I will remove the prefix throughout and move the test
application.
* 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
The windows 'toaster' sample relies on the resource order, but as a
belt and braces approach I will update the code to use the same
approach.
* IOCTL codes on Windows have a structure to them:
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/defining-i-o-control-codes
Thanks, I will fix this.
* 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.
Good point, I will change this.
* 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
Noted re try/except. As for a child inheriting it, the owner is tracked
by the WDFFILEOBJECT, which the child I believe will inherit also, which
would mean that the child would gain the ability to issue IOCTLs to the
mapping.
Thanks!
Ladi
No, thank you! I am grateful someone is willing to provide some feedback
on this.
I have been working on adding MSI interrupt support to the driver
also which is close to ready, just trying to figure out why the driver
fails to start with STATUS_DEVICE_POWER_FAILURE when I try to setup the
IRQs with WdfInterruptCreate.
Thanks again,
Geoff
