Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

On 2017-10-19 20:07, address@hidden wrote:
> On 2017-10-19 20:01, Ladi Prosek wrote:
> > 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.

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.