On Wed, Oct 18, 2017 at 7:50 AM, <ge...@hostfission.com> wrote: > 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.
Interesting, thanks! If that's really the case then we can remove the code from VirtioLib. I have cloned the latest Windows-driver-samples but can't find this under general/toaster. Namely ToasterEvtDevicePrepareHardware just prints some info about all resources but does not do anything order-related. Can you point me to the right code? >> >> * 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
