Re: [Qemu-devel] ivshmem Windows Driver

[Ladi Prosek]

On Thu, Oct 19, 2017 at 10:44 AM,  <address@hidden> wrote:
> On 2017-10-19 19:35, Ladi Prosek wrote:
>>
>> On Wed, Oct 18, 2017 at 5:04 PM,  <address@hidden> 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.