Re: [win-pv-devel] [PATCH] Add a user mode library wrapper for XENIFACE IOCTLs

[Marek Marczykowski-GÃrecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Dec 22, 2015 at 09:02:28AM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: RafaÅ WojdyÅa [mailto:omeg@xxxxxxxxxxxxxxxxxxxxxx]
> > Sent: 21 December 2015 22:30
> > To: Paul Durrant; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Marek Marczykowski-GÃrecki
> > Subject: Re: [win-pv-devel] [PATCH] Add a user mode library wrapper for
> > XENIFACE IOCTLs
> > 
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On 2015-11-05 22:12, RafaÅ WojdyÅa wrote:
> > > On 2015-11-05 17:55, Paul Durrant wrote:
> > >>> -----Original Message----- From:
> > >>> win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx
> > >>> [mailto:win-pv-devel- bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> > >>> Of Rafal Wojdyla Sent: 05 November 2015 14:22 To:
> > >>> win-pv-devel@xxxxxxxxxxxxxxxxxxxx Subject: [win-pv-devel]
> > >>> [PATCH] Add a user mode library wrapper for XENIFACE IOCTLs
> > >>>
> > >>> Signed-off-by: Rafal Wojdyla <omeg@xxxxxxxxxxxxxxxxxxxxxx> ---
> > >>> include/xencontrol.h                         | 342 ++++++++++
> > >>> src/xencontrol/xencontrol.c                  | 915
> > >>> +++++++++++++++++++++++++++ src/xencontrol/xencontrol.rc
> > >>> |  24 + src/xencontrol/xencontrol_private.h          |  49 ++
> > >>> vs2013/xencontrol.props                      |  84 +++
> > >>> vs2013/xencontrol/xencontrol.vcxproj         |  62 ++
> > >>> vs2013/xencontrol/xencontrol.vcxproj.filters |  13 +
> > >>> vs2013/xeniface.sln                          |  38 ++ 8 files
> > >>> changed, 1527 insertions(+) create mode 100644
> > >>> include/xencontrol.h create mode 100644
> > >>> src/xencontrol/xencontrol.c create mode 100644
> > >>> src/xencontrol/xencontrol.rc create mode 100644
> > >>> src/xencontrol/xencontrol_private.h create mode 100644
> > >>> vs2013/xencontrol.props create mode 100644
> > >>> vs2013/xencontrol/xencontrol.vcxproj create mode 100644
> > >>> vs2013/xencontrol/xencontrol.vcxproj.filters
> > >>>
> > >>
> > >> I also notice that there's no update to the xeniface package to
> > >> deliver the new DLL. Did you omit that for a reason? (I have no
> > >> objection to you adding the DLL to the package... I'm happy to
> > >> fix up the VS2012 vcxproj files if you don't have the older tools
> > >> to hand).
> > >>
> > >> Paul
> > >>
> > > Yeah, that's another thing I forgot about since I don't have VS2012
> > > at hand.
> > >
> > I got sidetracked a bit lately but would like to finish "upstreaming"
> > this if possible. I have a new discussion point though, regarding code
> > duplication with the Linux xenvchan implementation.
> > 
> > It turns out that my xenvchan implementation has a subtle bug that was
> > fixed some time ago in the Xen implementation. I had based my version
> > off the current Linux xenvchan which has the bug fixed, but in the
> > process of making it MSVC compatible I reintroduced the bug. My
> > colleague Marek (cc) suggested that we maybe could somehow use the
> > original Linux source with just some patches that make it Windows
> > compatible. I think you already pull some Xen sources (mainly
> > includes) for pvdrivers, so maybe it's possible.
> > 
> 
> Hi Rafal,
> 
>   I only pull public includes from Xen. They require post-processing (to 
> substitute 'long' for 'LONG_PTR' mainly) but no hand editing. I think that 
> pulling in actual source modules would be a lot harder (and you'd probably 
> have to turn the warning level in MSVC way down to make it happy with all the 
> implicit type conversion that usually goes on in Linux source code).

In previous version of this library (based on old PV drivers) we had
exactly the same vchan library compiling for both Linux and Windows.
There were few #ifdefs for vchan initialization, but that's all.

The current upstream version is almost good enough (besides
initialization). Yes, some implicit type conversions needs fixing, but
it isn't that bad. Basically this commit:
https://github.com/QubesOS/qubes-vmm-xen-windows-pvdrivers/commit/c5ead56568677fbeae130ae070610db060935c7e

And I think it would be accepted upstream.

That old vchan version, which worked in both Linux and Windows (and also
mini-os in stubdomain) is here:
https://github.com/QubesOS/qubes-core-vchan-xen/tree/release2/vchan

> > This would require that the usermode xeniface bindings were as close
> > to Linux libxc as possible. I'm not liking the idea that a Windows DLL
> > would only export Linux-flavored APIs, but I could easily make
> > additional exports that look mostly like libxc. The main difference is
> > that Linux APIs don't use event objects like Windows, and use
> > different return values.
> > 
> > I admit I have some concerns that identically named functions *will*
> > be different at least in some places (Linux libxc vs Windows). We
> > can't make the API 100% identical due to fundamental OS differences,
> > but I agree with Marek that code duplication is an issue and should be
> > avoided when possible. Thoughts? :)
> 
>   It's a tricky one. There's already way more code duplication than I would 
> like between the drivers themselves (e.g. a lot of the PnP code is completely 
> identical between XENBUS and XENVIF but I keep it separate since the drivers 
> do need to be able to evolve separately) so code duplication between 
> different OS implementations of an API/protocol is something that I think we 
> just have to live with. There is definitely merit in keeping the Windows and 
> Linux APIs 'aligned' for familiarity when porting an application but, as you 
> say, the OS are fundamentally different and trying to use common source is 
> probably going to end up mean too much abstraction away from both OS.

Yes, in some cases keeping the same code for different OSes is hard, but
I think it worth some effort, because managing duplicated code is much
more pain. For every commit you need to think about whether it should be
also included in the other copy/copies or not (and also know about
existence of all the copies!). And surely you will forget about it
sometimes. The case RafaÅ mentioned was a security bug!

The upstream libxc library already have an abstraction for os-specific
code. Not sure if that is flexible enough to use also for Windows, but
IMHO it worth consideration. For example places which return FD for
select/poll could return Event object etc.
If not possible to use exactly this one (which would make API 100%
compatible), IMO it worth pursuing the state where as few functions as
possible are different. Then, in case of vchan, it would mean separate
initialization functions (for example init_gnt_evt_posix,
init_evt_srv_windows), but most of the other code would be the same.

Some another idea would be to plug the current interface library into
libxc OS abstraction. So Windows build of libxc would use (be a wrapper
of) the current xencontrol library. But still, it would be good idea to
design the API that way to make it as easy as possible.

-- 
Best Regards,
Marek Marczykowski-GÃrecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: pgpIANUf5HaSc.pgp
Description: PGP signature

_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel