Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

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