[Qemu-devel] ivshmem Windows Driver

[Qemu-devel] ivshmem Windows Driver

[geoff]

Hi All,

I am writing some code that needs to share a block of ram between a 
Windows guest and Linux host. For this I am using the ivshmem device and 
I have written a very primitive driver for windows that allows a single 
application to request to memory map the pci bar (shared memory) into 
the program's context using DeviceIoControl.

This is all working fine, but the next problem is I need the driver to 
be signed. In it's current state I would not even suggest it be signed 
as it was just hacked together to test my concept, but now I know it's 
viable I would be willing to invest whatever time is required to write a 
driver that would be acceptable for signing.

The ideal driver would be general purpose and could be leveraged for any 
user mode application use, not just my specific case. It would need to 
implement the IRQ/even features of ivshmem and possibly even some kind 
of security to prevent unauthorized use by rogue applications (shared 
secret configured on the chardev?).

I have several qustions:

   1) Has someone done this? I can't find any reference to a windows 
driver for this device anywhere.
   2) If I was to pursue writing this driver, how would be the best way 
to go about it so as to ensure that it is in a state that it could be 
signed with the RedHat vendor key?
   3) What is the likelihood of having such a driver signed?
   4) Is there a preferred git host for such a driver?

Kind Regards
-Geoff

Re: [Qemu-devel] ivshmem Windows Driver

[Yan Vugenfirer]

He Geoff,

The official virtio-win drivers upstream repository is here: https://github.com/virtio-win/kvm-guest-drivers-windows

1. There is no ivshmem Windows Driver for now as far as I know

2. We are signing the drivers for community usage https://fedoraproject.org/wiki/Windows_Virtio_Drivers from the same repository. 
The process will be: submit the code for review with pull request (better use existing virtio library for virtio communication between the guest and the host), pass internal tests and at the least being able to pass MS HCK\HLK tests, later on the driver will be pulled into official build and release with rest of the drivers for community usage.
3. We are happy to cooperate on adding new functionality to current package of virtio drivers for Windows
4. As already mentioned: https://github.com/virtio-win/kvm-guest-drivers-windows

Thanks a lot!

If you have more questions, please don’t hesitate to talk to me, Ladi or anyone else from Red Hat involved with virtio-win development.

Best regards,
Yan.

> On 15 Oct 2017, at 12:32, geoff--- via Qemu-devel <qemu-devel@nongnu.org> wrote:
> 
> Hi All,
> 
> I am writing some code that needs to share a block of ram between a Windows guest and Linux host. For this I am using the ivshmem device and I have written a very primitive driver for windows that allows a single application to request to memory map the pci bar (shared memory) into the program's context using DeviceIoControl.
> 
> This is all working fine, but the next problem is I need the driver to be signed. In it's current state I would not even suggest it be signed as it was just hacked together to test my concept, but now I know it's viable I would be willing to invest whatever time is required to write a driver that would be acceptable for signing.
> 
> The ideal driver would be general purpose and could be leveraged for any user mode application use, not just my specific case. It would need to implement the IRQ/even features of ivshmem and possibly even some kind of security to prevent unauthorized use by rogue applications (shared secret configured on the chardev?).
> 
> I have several qustions:
> 
>  1) Has someone done this? I can't find any reference to a windows driver for this device anywhere.
>  2) If I was to pursue writing this driver, how would be the best way to go about it so as to ensure that it is in a state that it could be signed with the RedHat vendor key?
>  3) What is the likelihood of having such a driver signed?
>  4) Is there a preferred git host for such a driver?
> 
> Kind Regards
> -Geoff
> 

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

Hi Yan,

Thank you for the information. I am rather new to Windows Driver 
development and learning as I go, so this may take some time, but since 
the driver only needs to perform very basic functions I do not see this 
as being too much of a challenge.

-Geoff

On 2017-10-15 22:14, Yan Vugenfirer wrote:
> He Geoff,
> 
> The official virtio-win drivers upstream repository is here:
> https://github.com/virtio-win/kvm-guest-drivers-windows
> 
> 1. There is no ivshmem Windows Driver for now as far as I know
> 
> 2. We are signing the drivers for community usage
> https://fedoraproject.org/wiki/Windows_Virtio_Drivers from the same
> repository.
> The process will be: submit the code for review with pull request
> (better use existing virtio library for virtio communication between
> the guest and the host), pass internal tests and at the least being
> able to pass MS HCK\HLK tests, later on the driver will be pulled into
> official build and release with rest of the drivers for community
> usage.
> 3. We are happy to cooperate on adding new functionality to current
> package of virtio drivers for Windows
> 4. As already mentioned: 
> https://github.com/virtio-win/kvm-guest-drivers-windows
> 
> Thanks a lot!
> 
> If you have more questions, please don’t hesitate to talk to me, Ladi
> or anyone else from Red Hat involved with virtio-win development.
> 
> Best regards,
> Yan.
> 
>> On 15 Oct 2017, at 12:32, geoff--- via Qemu-devel 
>> <qemu-devel@nongnu.org> wrote:
>> 
>> Hi All,
>> 
>> I am writing some code that needs to share a block of ram between a 
>> Windows guest and Linux host. For this I am using the ivshmem device 
>> and I have written a very primitive driver for windows that allows a 
>> single application to request to memory map the pci bar (shared 
>> memory) into the program's context using DeviceIoControl.
>> 
>> This is all working fine, but the next problem is I need the driver to 
>> be signed. In it's current state I would not even suggest it be signed 
>> as it was just hacked together to test my concept, but now I know it's 
>> viable I would be willing to invest whatever time is required to write 
>> a driver that would be acceptable for signing.
>> 
>> The ideal driver would be general purpose and could be leveraged for 
>> any user mode application use, not just my specific case. It would 
>> need to implement the IRQ/even features of ivshmem and possibly even 
>> some kind of security to prevent unauthorized use by rogue 
>> applications (shared secret configured on the chardev?).
>> 
>> I have several qustions:
>> 
>>  1) Has someone done this? I can't find any reference to a windows 
>> driver for this device anywhere.
>>  2) If I was to pursue writing this driver, how would be the best way 
>> to go about it so as to ensure that it is in a state that it could be 
>> signed with the RedHat vendor key?
>>  3) What is the likelihood of having such a driver signed?
>>  4) Is there a preferred git host for such a driver?
>> 
>> Kind Regards
>> -Geoff
>> 

Re: [Qemu-devel] ivshmem Windows Driver

[Yan Vugenfirer]

> On 15 Oct 2017, at 15:21, geoff@hostfission.com wrote:
> 
> Hi Yan,
> 
> Thank you for the information. I am rather new to Windows Driver development and learning as I go, so this may take some time, but since the driver only needs to perform very basic functions I do not see this as being too much of a challenge.

I think you can look into Windows virtio-balloon implementation as an example of simple driver: https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/Balloon

It relies on virtio library (https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/VirtIO) and it is WDF driver (MS framework that simplifies the drivers development) that makes it very simple.

> 
> -Geoff
> 
> On 2017-10-15 22:14, Yan Vugenfirer wrote:
>> He Geoff,
>> The official virtio-win drivers upstream repository is here:
>> https://github.com/virtio-win/kvm-guest-drivers-windows
>> 1. There is no ivshmem Windows Driver for now as far as I know
>> 2. We are signing the drivers for community usage
>> https://fedoraproject.org/wiki/Windows_Virtio_Drivers from the same
>> repository.
>> The process will be: submit the code for review with pull request
>> (better use existing virtio library for virtio communication between
>> the guest and the host), pass internal tests and at the least being
>> able to pass MS HCK\HLK tests, later on the driver will be pulled into
>> official build and release with rest of the drivers for community
>> usage.
>> 3. We are happy to cooperate on adding new functionality to current
>> package of virtio drivers for Windows
>> 4. As already mentioned: https://github.com/virtio-win/kvm-guest-drivers-windows
>> Thanks a lot!
>> If you have more questions, please don’t hesitate to talk to me, Ladi
>> or anyone else from Red Hat involved with virtio-win development.
>> Best regards,
>> Yan.
>>> On 15 Oct 2017, at 12:32, geoff--- via Qemu-devel <qemu-devel@nongnu.org> wrote:
>>> Hi All,
>>> I am writing some code that needs to share a block of ram between a Windows guest and Linux host. For this I am using the ivshmem device and I have written a very primitive driver for windows that allows a single application to request to memory map the pci bar (shared memory) into the program's context using DeviceIoControl.
>>> This is all working fine, but the next problem is I need the driver to be signed. In it's current state I would not even suggest it be signed as it was just hacked together to test my concept, but now I know it's viable I would be willing to invest whatever time is required to write a driver that would be acceptable for signing.
>>> The ideal driver would be general purpose and could be leveraged for any user mode application use, not just my specific case. It would need to implement the IRQ/even features of ivshmem and possibly even some kind of security to prevent unauthorized use by rogue applications (shared secret configured on the chardev?).
>>> I have several qustions:
>>> 1) Has someone done this? I can't find any reference to a windows driver for this device anywhere.
>>> 2) If I was to pursue writing this driver, how would be the best way to go about it so as to ensure that it is in a state that it could be signed with the RedHat vendor key?
>>> 3) What is the likelihood of having such a driver signed?
>>> 4) Is there a preferred git host for such a driver?
>>> Kind Regards
>>> -Geoff
> 

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

On 2017-10-15 23:24, Yan Vugenfirer wrote:
>> On 15 Oct 2017, at 15:21, geoff@hostfission.com wrote:
>> 
>> Hi Yan,
>> 
>> Thank you for the information. I am rather new to Windows Driver 
>> development and learning as I go, so this may take some time, but 
>> since the driver only needs to perform very basic functions I do not 
>> see this as being too much of a challenge.
> 
> I think you can look into Windows virtio-balloon implementation as an
> example of simple driver:
> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/Balloon
> 
> It relies on virtio library
> (https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/VirtIO)
> and it is WDF driver (MS framework that simplifies the drivers
> development) that makes it very simple.
> 

Thanks again, I already have a prototype driver working using WDF, it's 
more learning the windows internals and how to best implement things.

>> 
>> -Geoff
>> 
>> On 2017-10-15 22:14, Yan Vugenfirer wrote:
>>> He Geoff,
>>> The official virtio-win drivers upstream repository is here:
>>> https://github.com/virtio-win/kvm-guest-drivers-windows
>>> 1. There is no ivshmem Windows Driver for now as far as I know
>>> 2. We are signing the drivers for community usage
>>> https://fedoraproject.org/wiki/Windows_Virtio_Drivers from the same
>>> repository.
>>> The process will be: submit the code for review with pull request
>>> (better use existing virtio library for virtio communication between
>>> the guest and the host), pass internal tests and at the least being
>>> able to pass MS HCK\HLK tests, later on the driver will be pulled 
>>> into
>>> official build and release with rest of the drivers for community
>>> usage.
>>> 3. We are happy to cooperate on adding new functionality to current
>>> package of virtio drivers for Windows
>>> 4. As already mentioned: 
>>> https://github.com/virtio-win/kvm-guest-drivers-windows
>>> Thanks a lot!
>>> If you have more questions, please don’t hesitate to talk to me, Ladi
>>> or anyone else from Red Hat involved with virtio-win development.
>>> Best regards,
>>> Yan.
>>>> On 15 Oct 2017, at 12:32, geoff--- via Qemu-devel 
>>>> <qemu-devel@nongnu.org> wrote:
>>>> Hi All,
>>>> I am writing some code that needs to share a block of ram between a 
>>>> Windows guest and Linux host. For this I am using the ivshmem device 
>>>> and I have written a very primitive driver for windows that allows a 
>>>> single application to request to memory map the pci bar (shared 
>>>> memory) into the program's context using DeviceIoControl.
>>>> This is all working fine, but the next problem is I need the driver 
>>>> to be signed. In it's current state I would not even suggest it be 
>>>> signed as it was just hacked together to test my concept, but now I 
>>>> know it's viable I would be willing to invest whatever time is 
>>>> required to write a driver that would be acceptable for signing.
>>>> The ideal driver would be general purpose and could be leveraged for 
>>>> any user mode application use, not just my specific case. It would 
>>>> need to implement the IRQ/even features of ivshmem and possibly even 
>>>> some kind of security to prevent unauthorized use by rogue 
>>>> applications (shared secret configured on the chardev?).
>>>> I have several qustions:
>>>> 1) Has someone done this? I can't find any reference to a windows 
>>>> driver for this device anywhere.
>>>> 2) If I was to pursue writing this driver, how would be the best way 
>>>> to go about it so as to ensure that it is in a state that it could 
>>>> be signed with the RedHat vendor key?
>>>> 3) What is the likelihood of having such a driver signed?
>>>> 4) Is there a preferred git host for such a driver?
>>>> Kind Regards
>>>> -Geoff
>> 

Re: [Qemu-devel] ivshmem Windows Driver

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

On 10/15/2017 04:32 AM, geoff--- via Qemu-devel wrote:
> Hi All,
> 
> I am writing some code that needs to share a block of ram between a
> Windows guest and Linux host. For this I am using the ivshmem device and
> I have written a very primitive driver for windows that allows a single
> application to request to memory map the pci bar (shared memory) into
> the program's context using DeviceIoControl.

Note that upstream support of ivshmem is rather lackluster.  Very often,
the first question in response to someone saying "I need ivshmem", is
"Why? and can we accomplish the same task using a better device
instead?"  Without knowing the full use case, it's hard to argue that
ivshmem is the right device to fit the use case.

There have been efforts to propose a better specified and better
structured replacement for ivshmem, such as vhost-pci, but I'm not sure
what status those patches are in, or if it would be a better fit for
your needs.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

On 2017-10-17 02:20, Eric Blake wrote:
> On 10/15/2017 04:32 AM, geoff--- via Qemu-devel wrote:
>> Hi All,
>> 
>> I am writing some code that needs to share a block of ram between a
>> Windows guest and Linux host. For this I am using the ivshmem device 
>> and
>> I have written a very primitive driver for windows that allows a 
>> single
>> application to request to memory map the pci bar (shared memory) into
>> the program's context using DeviceIoControl.
> 
> Note that upstream support of ivshmem is rather lackluster.  Very 
> often,
> the first question in response to someone saying "I need ivshmem", is
> "Why? and can we accomplish the same task using a better device
> instead?"  Without knowing the full use case, it's hard to argue that
> ivshmem is the right device to fit the use case.

Noted, but I believe that this is a very useful feature for even hacking
about with VMs before going as far as to write an entire driver for some
simple once use code.

There are some future personal projects I can see this being very handy
for into the future.

> 
> There have been efforts to propose a better specified and better
> structured replacement for ivshmem, such as vhost-pci, but I'm not sure
> what status those patches are in, or if it would be a better fit for
> your needs.

My needs are rather specific, I am capturing the desktop on a VM with
passthrough VGA using nVidia NvFBC and mapping it to the host for
extremely low latency high quality display. Unfortunately the use of
NvFBC is limited to high end professional cards such as the GRID. As 
such
I don't see it warranting a specific driver for my niche use.

I also lack the time to learn how to write a virtual PCI device for QEMU
that would be able to accommodate this exact feature. I would also need
to acquire a signing certificate to sign the driver which is a cost I 
can
not justify.

In comparison writing a windows interface to the existing IVSHMEM device
is not such a huge feat. In fact, I have already got a prototype driver
as part of my local virtio-win fork.

-Geoff

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

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

Kind Regards,
Geoff

On 2017-10-15 23:29, geoff@hostfission.com wrote:
> On 2017-10-15 23:24, Yan Vugenfirer wrote:
>>> On 15 Oct 2017, at 15:21, geoff@hostfission.com wrote:
>>> 
>>> Hi Yan,
>>> 
>>> Thank you for the information. I am rather new to Windows Driver 
>>> development and learning as I go, so this may take some time, but 
>>> since the driver only needs to perform very basic functions I do not 
>>> see this as being too much of a challenge.
>> 
>> I think you can look into Windows virtio-balloon implementation as an
>> example of simple driver:
>> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/Balloon
>> 
>> It relies on virtio library
>> (https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/VirtIO)
>> and it is WDF driver (MS framework that simplifies the drivers
>> development) that makes it very simple.
>> 
> 
> Thanks again, I already have a prototype driver working using WDF,
> it's more learning the windows internals and how to best implement
> things.
> 
>>> 
>>> -Geoff
>>> 
>>> On 2017-10-15 22:14, Yan Vugenfirer wrote:
>>>> He Geoff,
>>>> The official virtio-win drivers upstream repository is here:
>>>> https://github.com/virtio-win/kvm-guest-drivers-windows
>>>> 1. There is no ivshmem Windows Driver for now as far as I know
>>>> 2. We are signing the drivers for community usage
>>>> https://fedoraproject.org/wiki/Windows_Virtio_Drivers from the same
>>>> repository.
>>>> The process will be: submit the code for review with pull request
>>>> (better use existing virtio library for virtio communication between
>>>> the guest and the host), pass internal tests and at the least being
>>>> able to pass MS HCK\HLK tests, later on the driver will be pulled 
>>>> into
>>>> official build and release with rest of the drivers for community
>>>> usage.
>>>> 3. We are happy to cooperate on adding new functionality to current
>>>> package of virtio drivers for Windows
>>>> 4. As already mentioned: 
>>>> https://github.com/virtio-win/kvm-guest-drivers-windows
>>>> Thanks a lot!
>>>> If you have more questions, please don’t hesitate to talk to me, 
>>>> Ladi
>>>> or anyone else from Red Hat involved with virtio-win development.
>>>> Best regards,
>>>> Yan.
>>>>> On 15 Oct 2017, at 12:32, geoff--- via Qemu-devel 
>>>>> <qemu-devel@nongnu.org> wrote:
>>>>> Hi All,
>>>>> I am writing some code that needs to share a block of ram between a 
>>>>> Windows guest and Linux host. For this I am using the ivshmem 
>>>>> device and I have written a very primitive driver for windows that 
>>>>> allows a single application to request to memory map the pci bar 
>>>>> (shared memory) into the program's context using DeviceIoControl.
>>>>> This is all working fine, but the next problem is I need the driver 
>>>>> to be signed. In it's current state I would not even suggest it be 
>>>>> signed as it was just hacked together to test my concept, but now I 
>>>>> know it's viable I would be willing to invest whatever time is 
>>>>> required to write a driver that would be acceptable for signing.
>>>>> The ideal driver would be general purpose and could be leveraged 
>>>>> for any user mode application use, not just my specific case. It 
>>>>> would need to implement the IRQ/even features of ivshmem and 
>>>>> possibly even some kind of security to prevent unauthorized use by 
>>>>> rogue applications (shared secret configured on the chardev?).
>>>>> I have several qustions:
>>>>> 1) Has someone done this? I can't find any reference to a windows 
>>>>> driver for this device anywhere.
>>>>> 2) If I was to pursue writing this driver, how would be the best 
>>>>> way to go about it so as to ensure that it is in a state that it 
>>>>> could be signed with the RedHat vendor key?
>>>>> 3) What is the likelihood of having such a driver signed?
>>>>> 4) Is there a preferred git host for such a driver?
>>>>> Kind Regards
>>>>> -Geoff
>>> 

Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

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.

* 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

* IOCTL codes on Windows have a structure to them:
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/defining-i-o-control-codes

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

* 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

Thanks!
Ladi

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

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

Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

On Wed, Oct 18, 2017 at 7:50 AM,  <geoff@hostfission.com> wrote:
> 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.

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

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,  <geoff@hostfission.com> wrote:
>> 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.
> 
> 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

Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

On Wed, Oct 18, 2017 at 8:56 AM,  <geoff@hostfission.com> wrote:
> On 2017-10-18 17:50, Ladi Prosek wrote:
>>
>> On Wed, Oct 18, 2017 at 7:50 AM,  <geoff@hostfission.com> wrote:
>>>
>>> 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.
>>
>>
>> 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
>
>

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

Hi Ladi & Yan,

I am pleased to present the completed driver for review, please see:

https://github.com/gnif/kvm-guest-drivers-windows

All issues previously mentioned have been addressed and all missing
functionality has been added.

Please note that this work has exposed a bug in the qemu ivshmem
virtual device itself, it seems that if the MSI interrupts are enabled
and the driver is unloaded twice an assertion is thrown due to what
looks to be a double free, crashing out qemu.

Once this driver has been finalized I will look into the cause of
this problem and see if I can correct it also.

Kind Regards,
Geoffrey McRae

Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

On Wed, Oct 18, 2017 at 5:04 PM,  <geoff@hostfission.com> wrote:
> Hi Ladi & Yan,
>
> I am pleased to present the completed driver for review, please see:
>
> https://github.com/gnif/kvm-guest-drivers-windows

Awesome!

Feel free to open pull request, it should be easier to comment on.

* WoW considerations: It would be nice if the driver could detect that
the map request is coming from a 32-bit process and expect a different
layout of struct IVSHMEM_MMAP.

* It would be cleaner to use READ_REGISTER_* and WRITE_REGISTER_*
from/to IVSHMEMDeviceRegisters instead of plain memory accesses. Or at
the very least the accesses should be marked volatile.

* In ivshmem.inf: ManufacturerName="Red Hat, Inc." instead of "RedHat"

* Is any of the API used by the driver Win10-only? Just curious, it's
fine to build the driver only for Win10 for now even if it isn't.

Thanks!

> All issues previously mentioned have been addressed and all missing
> functionality has been added.
>
> Please note that this work has exposed a bug in the qemu ivshmem
> virtual device itself, it seems that if the MSI interrupts are enabled
> and the driver is unloaded twice an assertion is thrown due to what
> looks to be a double free, crashing out qemu.
>
> Once this driver has been finalized I will look into the cause of
> this problem and see if I can correct it also.
>
> Kind Regards,
> Geoffrey McRae

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

On 2017-10-19 19:35, Ladi Prosek wrote:
> On Wed, Oct 18, 2017 at 5:04 PM,  <geoff@hostfission.com> wrote:
>> Hi Ladi & Yan,
>> 
>> I am pleased to present the completed driver for review, please see:
>> 
>> https://github.com/gnif/kvm-guest-drivers-windows
> 
> Awesome!
> 
> Feel free to open pull request, it should be easier to comment on.

Great, I will do so after I have addressed the below. Thanks again.

> 
> * WoW considerations: It would be nice if the driver could detect that
> the map request is coming from a 32-bit process and expect a different
> layout of struct IVSHMEM_MMAP.

I did think of this but I am unsure as to how to detect this.

> 
> * It would be cleaner to use READ_REGISTER_* and WRITE_REGISTER_*
> from/to IVSHMEMDeviceRegisters instead of plain memory accesses. Or at
> the very least the accesses should be marked volatile.

I thought that mapping the IO space was enough for this since it is
mapped as non-cacheable. I can see the point of marking it volatile but
see no need to use the read/write register semantics. If this is what it
takes however I am happy to do so.

> 
> * In ivshmem.inf: ManufacturerName="Red Hat, Inc." instead of "RedHat"
> 

No worries.

> * Is any of the API used by the driver Win10-only? Just curious, it's
> fine to build the driver only for Win10 for now even if it isn't.

I have not tried to build it on anything older then win 10 build 10586
as I have nothing older, but AFAIK it should build on windows 8.1 or
later just fine. This is more due to my lack of familiarity with Visual
Studio, give me gcc and vim any day :).

> 
> Thanks!
> 
>> All issues previously mentioned have been addressed and all missing
>> functionality has been added.
>> 
>> Please note that this work has exposed a bug in the qemu ivshmem
>> virtual device itself, it seems that if the MSI interrupts are enabled
>> and the driver is unloaded twice an assertion is thrown due to what
>> looks to be a double free, crashing out qemu.
>> 
>> Once this driver has been finalized I will look into the cause of
>> this problem and see if I can correct it also.
>> 
>> Kind Regards,
>> Geoffrey McRae

Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

On Thu, Oct 19, 2017 at 10:44 AM,  <geoff@hostfission.com> wrote:
> On 2017-10-19 19:35, Ladi Prosek wrote:
>>
>> On Wed, Oct 18, 2017 at 5:04 PM,  <geoff@hostfission.com> wrote:
>>>
>>> Hi Ladi & Yan,
>>>
>>> I am pleased to present the completed driver for review, please see:
>>>
>>> https://github.com/gnif/kvm-guest-drivers-windows
>>
>>
>> Awesome!
>>
>> Feel free to open pull request, it should be easier to comment on.
>
>
> Great, I will do so after I have addressed the below. Thanks again.
>
>>
>> * WoW considerations: It would be nice if the driver could detect that
>> the map request is coming from a 32-bit process and expect a different
>> layout of struct IVSHMEM_MMAP.
>
>
> I did think of this but I am unsure as to how to detect this.

I don't think I ever used it but IoIs32bitProcess() looks promising.

>>
>> * It would be cleaner to use READ_REGISTER_* and WRITE_REGISTER_*
>> from/to IVSHMEMDeviceRegisters instead of plain memory accesses. Or at
>> the very least the accesses should be marked volatile.
>
>
> I thought that mapping the IO space was enough for this since it is
> mapped as non-cacheable. I can see the point of marking it volatile but
> see no need to use the read/write register semantics. If this is what it
> takes however I am happy to do so.

Code like this raises eyebrows:

  deviceContext->devRegisters->doorbell |= (UINT32)in->vector |
(in->peerID << 16);

Many readers will probably be wondering what exactly the compiler is
allowed to do with this statement. May it end up ORing the lower and
upper word separately, for example?

  OR [word ptr addr], in->vector
  OR [word ptr addr + 2], in->peerID

And, by the way, is OR really what we want here?

>>
>> * In ivshmem.inf: ManufacturerName="Red Hat, Inc." instead of "RedHat"
>>
>
> No worries.
>
>> * Is any of the API used by the driver Win10-only? Just curious, it's
>> fine to build the driver only for Win10 for now even if it isn't.
>
>
> I have not tried to build it on anything older then win 10 build 10586
> as I have nothing older, but AFAIK it should build on windows 8.1 or
> later just fine. This is more due to my lack of familiarity with Visual
> Studio, give me gcc and vim any day :).

Gotcha, no worries, other versions can be tested later.

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

On 2017-10-19 20:01, Ladi Prosek wrote:
> On Thu, Oct 19, 2017 at 10:44 AM,  <geoff@hostfission.com> wrote:
>> On 2017-10-19 19:35, Ladi Prosek wrote:
>>> 
>>> On Wed, Oct 18, 2017 at 5:04 PM,  <geoff@hostfission.com> wrote:
>>>> 
>>>> Hi Ladi & Yan,
>>>> 
>>>> I am pleased to present the completed driver for review, please see:
>>>> 
>>>> https://github.com/gnif/kvm-guest-drivers-windows
>>> 
>>> 
>>> Awesome!
>>> 
>>> Feel free to open pull request, it should be easier to comment on.
>> 
>> 
>> Great, I will do so after I have addressed the below. Thanks again.
>> 
>>> 
>>> * WoW considerations: It would be nice if the driver could detect 
>>> that
>>> the map request is coming from a 32-bit process and expect a 
>>> different
>>> layout of struct IVSHMEM_MMAP.
>> 
>> 
>> I did think of this but I am unsure as to how to detect this.
> 
> I don't think I ever used it but IoIs32bitProcess() looks promising.
> 
>>> 
>>> * It would be cleaner to use READ_REGISTER_* and WRITE_REGISTER_*
>>> from/to IVSHMEMDeviceRegisters instead of plain memory accesses. Or 
>>> at
>>> the very least the accesses should be marked volatile.
>> 
>> 
>> I thought that mapping the IO space was enough for this since it is
>> mapped as non-cacheable. I can see the point of marking it volatile 
>> but
>> see no need to use the read/write register semantics. If this is what 
>> it
>> takes however I am happy to do so.
> 
> Code like this raises eyebrows:
> 
>   deviceContext->devRegisters->doorbell |= (UINT32)in->vector |
> (in->peerID << 16);
> 
> Many readers will probably be wondering what exactly the compiler is
> allowed to do with this statement. May it end up ORing the lower and
> upper word separately, for example?
> 
>   OR [word ptr addr], in->vector
>   OR [word ptr addr + 2], in->peerID
> 
> And, by the way, is OR really what we want here?
> 

After double checking this you are dead right, the register is 
documented
as write only. I will fix this.

>>> 
>>> * In ivshmem.inf: ManufacturerName="Red Hat, Inc." instead of 
>>> "RedHat"
>>> 
>> 
>> No worries.
>> 
>>> * Is any of the API used by the driver Win10-only? Just curious, it's
>>> fine to build the driver only for Win10 for now even if it isn't.
>> 
>> 
>> I have not tried to build it on anything older then win 10 build 10586
>> as I have nothing older, but AFAIK it should build on windows 8.1 or
>> later just fine. This is more due to my lack of familiarity with 
>> Visual
>> Studio, give me gcc and vim any day :).
> 
> Gotcha, no worries, other versions can be tested later.

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

On 2017-10-19 20:07, geoff@hostfission.com wrote:
> On 2017-10-19 20:01, Ladi Prosek wrote:
>> On Thu, Oct 19, 2017 at 10:44 AM,  <geoff@hostfission.com> wrote:
>>> On 2017-10-19 19:35, Ladi Prosek wrote:
>>>> 
>>>> On Wed, Oct 18, 2017 at 5:04 PM,  <geoff@hostfission.com> wrote:
>>>>> 
>>>>> Hi Ladi & Yan,
>>>>> 
>>>>> I am pleased to present the completed driver for review, please 
>>>>> see:
>>>>> 
>>>>> https://github.com/gnif/kvm-guest-drivers-windows
>>>> 
>>>> 
>>>> Awesome!
>>>> 
>>>> Feel free to open pull request, it should be easier to comment on.
>>> 
>>> 
>>> Great, I will do so after I have addressed the below. Thanks again.
>>> 

I have created a PR, see:

https://github.com/virtio-win/kvm-guest-drivers-windows/pull/174

>>>> 
>>>> * WoW considerations: It would be nice if the driver could detect 
>>>> that
>>>> the map request is coming from a 32-bit process and expect a 
>>>> different
>>>> layout of struct IVSHMEM_MMAP.
>>> 
>>> 
>>> I did think of this but I am unsure as to how to detect this.
>> 
>> I don't think I ever used it but IoIs32bitProcess() looks promising.
>> 


Obviously PVOID will be 32bit which will mess with the struct size and
offset of vectors but I am not aware of a solution to this. If you have
any suggestions on how to rectify this it would be very much 
appreciated.

>>>> 
>>>> * It would be cleaner to use READ_REGISTER_* and WRITE_REGISTER_*
>>>> from/to IVSHMEMDeviceRegisters instead of plain memory accesses. Or 
>>>> at
>>>> the very least the accesses should be marked volatile.
>>> 
>>> 
>>> I thought that mapping the IO space was enough for this since it is
>>> mapped as non-cacheable. I can see the point of marking it volatile 
>>> but
>>> see no need to use the read/write register semantics. If this is what 
>>> it
>>> takes however I am happy to do so.
>> 
>> Code like this raises eyebrows:
>> 
>>   deviceContext->devRegisters->doorbell |= (UINT32)in->vector |
>> (in->peerID << 16);
>> 
>> Many readers will probably be wondering what exactly the compiler is
>> allowed to do with this statement. May it end up ORing the lower and
>> upper word separately, for example?
>> 
>>   OR [word ptr addr], in->vector
>>   OR [word ptr addr + 2], in->peerID
>> 
>> And, by the way, is OR really what we want here?
>> 
> 
> After double checking this you are dead right, the register is 
> documented
> as write only. I will fix this.

Done.

> 
>>>> 
>>>> * In ivshmem.inf: ManufacturerName="Red Hat, Inc." instead of 
>>>> "RedHat"
>>>> 
>>> 
>>> No worries.
>>> 
>>>> * Is any of the API used by the driver Win10-only? Just curious, 
>>>> it's
>>>> fine to build the driver only for Win10 for now even if it isn't.
>>> 
>>> 
>>> I have not tried to build it on anything older then win 10 build 
>>> 10586
>>> as I have nothing older, but AFAIK it should build on windows 8.1 or
>>> later just fine. This is more due to my lack of familiarity with 
>>> Visual
>>> Studio, give me gcc and vim any day :).
>> 
>> Gotcha, no worries, other versions can be tested later.

Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

On Thu, Oct 19, 2017 at 11:41 AM,  <geoff@hostfission.com> wrote:
> On 2017-10-19 20:07, geoff@hostfission.com wrote:
>>
>> On 2017-10-19 20:01, Ladi Prosek wrote:
>>>
>>> On Thu, Oct 19, 2017 at 10:44 AM,  <geoff@hostfission.com> wrote:
>>>>
>>>> On 2017-10-19 19:35, Ladi Prosek wrote:
>>>>>
>>>>>
>>>>> On Wed, Oct 18, 2017 at 5:04 PM,  <geoff@hostfission.com> wrote:
>>>>>>
>>>>>>
>>>>>> Hi Ladi & Yan,
>>>>>>
>>>>>> I am pleased to present the completed driver for review, please see:
>>>>>>
>>>>>> https://github.com/gnif/kvm-guest-drivers-windows
>>>>>
>>>>>
>>>>>
>>>>> Awesome!
>>>>>
>>>>> Feel free to open pull request, it should be easier to comment on.
>>>>
>>>>
>>>>
>>>> Great, I will do so after I have addressed the below. Thanks again.
>>>>
>
> I have created a PR, see:
>
> https://github.com/virtio-win/kvm-guest-drivers-windows/pull/174
>
>>>>>
>>>>> * WoW considerations: It would be nice if the driver could detect that
>>>>> the map request is coming from a 32-bit process and expect a different
>>>>> layout of struct IVSHMEM_MMAP.
>>>>
>>>>
>>>>
>>>> I did think of this but I am unsure as to how to detect this.
>>>
>>>
>>> I don't think I ever used it but IoIs32bitProcess() looks promising.
>>>
>
>
> Obviously PVOID will be 32bit which will mess with the struct size and
> offset of vectors but I am not aware of a solution to this. If you have
> any suggestions on how to rectify this it would be very much appreciated.

I was thinking something simple like:

#ifdef _WIN64
typedef struct IVSHMEM_MMAP_32
{
    ...
    UINT32 ptr;
    ...
}
IVSHMEM_MMAP_32, *PIVSHMEM_MMAP_32;
#endif

in a private header. Then in the IOCTL handler call IoIs32bitProcess()
and if it returns true, expect IVSHMEM_MMAP_32 instead of
IVSHMEM_MMAP.

>>>>>
>>>>> * It would be cleaner to use READ_REGISTER_* and WRITE_REGISTER_*
>>>>> from/to IVSHMEMDeviceRegisters instead of plain memory accesses. Or at
>>>>> the very least the accesses should be marked volatile.
>>>>
>>>>
>>>>
>>>> I thought that mapping the IO space was enough for this since it is
>>>> mapped as non-cacheable. I can see the point of marking it volatile but
>>>> see no need to use the read/write register semantics. If this is what it
>>>> takes however I am happy to do so.
>>>
>>>
>>> Code like this raises eyebrows:
>>>
>>>   deviceContext->devRegisters->doorbell |= (UINT32)in->vector |
>>> (in->peerID << 16);
>>>
>>> Many readers will probably be wondering what exactly the compiler is
>>> allowed to do with this statement. May it end up ORing the lower and
>>> upper word separately, for example?
>>>
>>>   OR [word ptr addr], in->vector
>>>   OR [word ptr addr + 2], in->peerID
>>>
>>> And, by the way, is OR really what we want here?
>>>
>>
>> After double checking this you are dead right, the register is documented
>> as write only. I will fix this.
>
>
> Done.
>
>
>>
>>>>>
>>>>> * In ivshmem.inf: ManufacturerName="Red Hat, Inc." instead of "RedHat"
>>>>>
>>>>
>>>> No worries.
>>>>
>>>>> * Is any of the API used by the driver Win10-only? Just curious, it's
>>>>> fine to build the driver only for Win10 for now even if it isn't.
>>>>
>>>>
>>>>
>>>> I have not tried to build it on anything older then win 10 build 10586
>>>> as I have nothing older, but AFAIK it should build on windows 8.1 or
>>>> later just fine. This is more due to my lack of familiarity with Visual
>>>> Studio, give me gcc and vim any day :).
>>>
>>>
>>> Gotcha, no worries, other versions can be tested later.
>
>

Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

On 2017-10-19 20:51, Ladi Prosek wrote:
> On Thu, Oct 19, 2017 at 11:41 AM,  <geoff@hostfission.com> wrote:
>> On 2017-10-19 20:07, geoff@hostfission.com wrote:
>>> 
>>> On 2017-10-19 20:01, Ladi Prosek wrote:
>>>> 
>>>> On Thu, Oct 19, 2017 at 10:44 AM,  <geoff@hostfission.com> wrote:
>>>>> 
>>>>> On 2017-10-19 19:35, Ladi Prosek wrote:
>>>>>> 
>>>>>> 
>>>>>> On Wed, Oct 18, 2017 at 5:04 PM,  <geoff@hostfission.com> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> Hi Ladi & Yan,
>>>>>>> 
>>>>>>> I am pleased to present the completed driver for review, please 
>>>>>>> see:
>>>>>>> 
>>>>>>> https://github.com/gnif/kvm-guest-drivers-windows
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> Awesome!
>>>>>> 
>>>>>> Feel free to open pull request, it should be easier to comment on.
>>>>> 
>>>>> 
>>>>> 
>>>>> Great, I will do so after I have addressed the below. Thanks again.
>>>>> 
>> 
>> I have created a PR, see:
>> 
>> https://github.com/virtio-win/kvm-guest-drivers-windows/pull/174
>> 
>>>>>> 
>>>>>> * WoW considerations: It would be nice if the driver could detect 
>>>>>> that
>>>>>> the map request is coming from a 32-bit process and expect a 
>>>>>> different
>>>>>> layout of struct IVSHMEM_MMAP.
>>>>> 
>>>>> 
>>>>> 
>>>>> I did think of this but I am unsure as to how to detect this.
>>>> 
>>>> 
>>>> I don't think I ever used it but IoIs32bitProcess() looks promising.
>>>> 
>> 
>> 
>> Obviously PVOID will be 32bit which will mess with the struct size and
>> offset of vectors but I am not aware of a solution to this. If you 
>> have
>> any suggestions on how to rectify this it would be very much 
>> appreciated.
> 
> I was thinking something simple like:
> 
> #ifdef _WIN64
> typedef struct IVSHMEM_MMAP_32
> {
>     ...
>     UINT32 ptr;
>     ...
> }
> IVSHMEM_MMAP_32, *PIVSHMEM_MMAP_32;
> #endif
> 
> in a private header. Then in the IOCTL handler call IoIs32bitProcess()
> and if it returns true, expect IVSHMEM_MMAP_32 instead of
> IVSHMEM_MMAP.

Ah that makes sense, thanks! This has been done.

> 
>>>>>> 
>>>>>> * It would be cleaner to use READ_REGISTER_* and WRITE_REGISTER_*
>>>>>> from/to IVSHMEMDeviceRegisters instead of plain memory accesses. 
>>>>>> Or at
>>>>>> the very least the accesses should be marked volatile.
>>>>> 
>>>>> 
>>>>> 
>>>>> I thought that mapping the IO space was enough for this since it is
>>>>> mapped as non-cacheable. I can see the point of marking it volatile 
>>>>> but
>>>>> see no need to use the read/write register semantics. If this is 
>>>>> what it
>>>>> takes however I am happy to do so.
>>>> 
>>>> 
>>>> Code like this raises eyebrows:
>>>> 
>>>>   deviceContext->devRegisters->doorbell |= (UINT32)in->vector |
>>>> (in->peerID << 16);
>>>> 
>>>> Many readers will probably be wondering what exactly the compiler is
>>>> allowed to do with this statement. May it end up ORing the lower and
>>>> upper word separately, for example?
>>>> 
>>>>   OR [word ptr addr], in->vector
>>>>   OR [word ptr addr + 2], in->peerID
>>>> 
>>>> And, by the way, is OR really what we want here?
>>>> 
>>> 
>>> After double checking this you are dead right, the register is 
>>> documented
>>> as write only. I will fix this.
>> 
>> 
>> Done.
>> 
>> 
>>> 
>>>>>> 
>>>>>> * In ivshmem.inf: ManufacturerName="Red Hat, Inc." instead of 
>>>>>> "RedHat"
>>>>>> 
>>>>> 
>>>>> No worries.
>>>>> 
>>>>>> * Is any of the API used by the driver Win10-only? Just curious, 
>>>>>> it's
>>>>>> fine to build the driver only for Win10 for now even if it isn't.
>>>>> 
>>>>> 
>>>>> 
>>>>> I have not tried to build it on anything older then win 10 build 
>>>>> 10586
>>>>> as I have nothing older, but AFAIK it should build on windows 8.1 
>>>>> or
>>>>> later just fine. This is more due to my lack of familiarity with 
>>>>> Visual
>>>>> Studio, give me gcc and vim any day :).
>>>> 
>>>> 
>>>> Gotcha, no worries, other versions can be tested later.
>> 
>>