Re: [Qemu-devel] ivshmem Windows Driver

[geoff@hostfission.com]

On 2017-10-18 16:31, Ladi Prosek wrote:
> Hi Geoff,
> 
> On Mon, Oct 16, 2017 at 8:31 PM,  <geoff@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