On 2017-10-19 20:07, ge...@hostfission.com wrote:
On 2017-10-19 20:01, Ladi Prosek wrote:
On Thu, Oct 19, 2017 at 10:44 AM, <ge...@hostfission.com> wrote:
On 2017-10-19 19:35, Ladi Prosek wrote:
On Wed, Oct 18, 2017 at 5:04 PM, <ge...@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.
