Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

On Wed, Oct 18, 2017 at 8:56 AM,  <address@hidden> wrote:
> On 2017-10-18 17:50, Ladi Prosek wrote:
>>
>> 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?
>>
>
> Sorry, my mistake, it wasn't the toaster code but the kmdf driver, it
> assumes the BAR ordering to determine which is which.
>
> https://github.com/Microsoft/Windows-driver-samples/blob/aa6e0b36eb932099fa4eb950a6f5e289a23b6d6e/general/pcidrv/kmdf/HW/nic_init.c#L649

Got it. And MSDN says:

"A driver for a PCI bus device receives resources that are listed in
the order in which they appear in the device’s Base Address Registers
(BARs)."
https://docs.microsoft.com/en-us/windows-hardware/drivers/wdf/raw-and-translated-resources

So please disregard my comment.

We'll probably leave VirtioLib alone for now to stay robust, though.
Counting resources works only if the BAR layout is known. VirtIO 1.0
depends on knowing the exact BAR index and can't assume that the
second mem/IO resource is BAR #1, for example. Thanks!

>
>>>>
>>>> * 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
>
>