Re: [Qemu-devel] ivshmem Windows Driver

[geoff]

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.

> * 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.

> * 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 :).

> Thanks!
>
> > All issues previously mentioned have been addressed and all missing
> > functionality has been added.
> >
> > Please note that this work has exposed a bug in the qemu ivshmem
> > virtual device itself, it seems that if the MSI interrupts are enabled
> > and the driver is unloaded twice an assertion is thrown due to what
> > looks to be a double free, crashing out qemu.
> >
> > Once this driver has been finalized I will look into the cause of
> > this problem and see if I can correct it also.
> >
> > Kind Regards,
> > Geoffrey McRae