From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35749) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e57Kv-0004d8-Mk for qemu-devel@nongnu.org; Thu, 19 Oct 2017 05:41:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e57Kr-0005O6-M8 for qemu-devel@nongnu.org; Thu, 19 Oct 2017 05:41:57 -0400 Received: from mail1.hostfission.com ([139.99.139.48]:34634) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e57Kr-0005N9-1x for qemu-devel@nongnu.org; Thu, 19 Oct 2017 05:41:53 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 19 Oct 2017 20:41:47 +1100 From: geoff@hostfission.com In-Reply-To: <59a147491e11c4c692e9f762bcd9fb21@hostfission.com> References: <5548e41a5668ec0cba9543139327e035@hostfission.com> <3A7697A0-1AF3-46E8-8BCC-45A8127A2638@redhat.com> <286ea2bbfb5ec77d962300c9996c8688@hostfission.com> <92FEAF6E-3002-4F0F-B845-4EC3D29381A4@redhat.com> <34b4df5da848d69b213f538e5751c0d3@hostfission.com> <0f911b72c94a0d0f1bedf1c6385b045e@hostfission.com> <4d790f860a3cf21bd7e33e7c3f53b58b@hostfission.com> <59a147491e11c4c692e9f762bcd9fb21@hostfission.com> Message-ID: Subject: Re: [Qemu-devel] ivshmem Windows Driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ladi Prosek Cc: Yan Vugenfirer , qemu-devel 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, wrote: >>> On 2017-10-19 19:35, Ladi Prosek wrote: >>>> >>>> On Wed, Oct 18, 2017 at 5:04 PM, 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.