Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

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

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