On Thu, Oct 19, 2017 at 11:41 AM, <ge...@hostfission.com> wrote:
> 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.
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.
>
>
