Re: [Qemu-devel] ivshmem Windows Driver

[geoff@hostfission.com]

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.