Re: [win-pv-devel] Porting libvchan to use the Windows PV Drivers

[RafaÅ WojdyÅa <omeg@xxxxxxxxxxxxxxxxxxxxxx>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2015-05-06 18:47, Paul Durrant wrote:
>> -----Original Message----- From: RafaÅ WojdyÅa 
>> [mailto:omeg@xxxxxxxxxxxxxxxxxxxxxx] Sent: 06 May 2015 06:11 To:
>>  Paul Durrant; win-pv-devel@xxxxxxxxxxxxxxxxxxxx Subject: Re: 
>> [win-pv-devel] Porting libvchan to use the Windows PV Drivers
>> 
> It turned out that our fork of the GPL PV drivers didn't work on 
> Qubes r3, I'm not really sure why. Generally the OS failed to boot
>  because Windows saw incorrect partition layout on the disks for
> some reason. The vbd driver didn't report any errors but I saw that
> the boot disk had 2 instead of 3 partitions, and a second, totally
>  empty/unitialized disk was showing up with 2 partitions. Our fork
>  wasn't really kept up to date before and I didn't want to work
> with the old code, so I decided to use the new drivers after all.
> 
> 
>> Cool.
> 
> I have some question regarding eventual patches to send and how to
>  best structure the new code. I have a prototype working that 
> implements event channel and grant IOCTLs, including mapping
> foreign pages.
> 
> 
>> Sounds good.
> 
> - Mapping foreign pages requires adding new APIs to xenbus. I
> assume it's best to add them to the existing gnttab interface (in a
> v2 interface version). That functionality doesn't really touch
> guest grant tables but it's grouped in one public Xen header so
> that probably makes the most sense. Does such approach require
> changes to the coinstaller?
> 
> 
>> Bumping the GNTTAB version and adding in your extra calls is the
>>  right thing to do. You should not need to make any changes to
>> the co-installer directly - just the gnttab_interface header.
> 
> - All IOCTL handling is implemented in xeniface. Required
> interfaces (evtchn, gnttab) should be subscribed to by the
> coinstaller but I didn't see any code for removing the
> subscription. Is that automatic on driver uninstall?
> 
> 
>> Yes. Uninstallation of a driver should blow subscriptions away; 
>> maybe that's missing.

Right, I wrote xenbus but meant the xeniface co-installer.

> 
> - For event channels I just accept an event handle from user mode 
> instead of a weird I/O construct the GPL drivers did. Event channel
>  callbacks are basically IRQ handlers so that's mildly inconvenient
>  but I just fire a DPC and signal the event from there.
> 
> 
>> Sounds right. I assume you do an ObReferenceObjectByHandle and 
>> then KeSetEvent the object from your DPC? I don't think there's 
>> any alternative to using a DPC for this.

Yeah, that's exactly what I'm doing.
> 
> - For tracking purposes I assume that I can rely on local ports
> being unique (so that the port is an index/key for my internal
> state list).
> 
> 
>> Yes, ports are unique per domain but they will be recycled
>> eagerly so you just need to make sure you're not holding onto
>> stale state after the channel is closed. You can rely on all
>> pending events having been processed before a channel number is
>> recycled, but you cannot rely on all pending events having been
>> processed before the close operation returns (since there is no
>> good way of synchronizing a close on 1 cpu with a pending event
>> on another).

Alright, I'll take a look at my logic and see if that's a problem.
> 
> - Event channels don't have any security applied to them so in
> theory any process can signal or close any other channel because
> xeniface doesn't track device opens. Should something be done with
> that, like keeping track of the process that opens the specific
> channel?
> 
> 
>> Yes, I would say so. You need to track things like grant maps or
>>  open event channels against the file object so that they can be
>>  destroyed if the process terminates abnormally anyway.
> 
> - Granting pages isn't very complicated: allocate some pool memory,
>  build a MDL to map physical pages, call PermitForeignAccess, map
> to user space. User gets the address and a reference list.
> 
> - For mapping foreign pages I allocate address space by 
> FdoAllocateIoSpace() and the rest is pretty much the same as with 
> granting. User gets just the address and a handle to 
> driver-maintained bookkeeping context.
> 
> 
>> Yep. Sounds right.
> 
> - If the hypervisor returns an error during unmapping/ungranting 
> that's pretty bad news since we can't free such memory (foreign 
> domain still has access to it). I just ASSERT that since I assume 
> it's not an issue during normal system operation.
> 
> 
>> Indeed. I don't think there's anything else that can be done
>> apart from retrying forever.
> 
> I only tested this on 64-bit Windows 7 so far but it seems to work
>  fine. I'll be doing more testing after I have libvchan working on 
> top of the new drivers. And to close, a screen shot of my test 
> program sharing memory on Qubes R3:
> 
> http://i.imgur.com/xhfDkhl.png
> 
> 
>> Looks impressive, although I don't have much of a clue what some 
>> of the output means : -)

The server (on the left) printfs some text into the shared memory every
second and the client (right) reads it. "S:1 C:0" shows two shared
variables, S representing the server being active and C client. They are
set to 1 after the respective peer maps the memory and the driver resets
them to 0 when the memory is being unmapped (so unexpected peer
termination can be easily detected, in addition to using an event
channel). The client initially maps one page which contains the event
port and the rest of grant references, then remaps the whole
multiple-page region with notification-un-unmap enabled. Just a quick
and dirty test but it seems to work like expected :)
> 
>> BTW, if you want to post RFC patches I'm happy to look them
>> over.

Sure. My current code lies here: https://github.com/omeg in winpv-*
repos. Some commits are not very clean and as said, it's not tested
extensively. One thing I don't like is the need to have a global Fdo
pointer for the process notification routine since it doesn't get any
context parameters. I might change the cleanup model to use pending
IOs as discussed, we'll see.
> 
>> Cheers,
> 
>> Paul
> 

- -- 
RafaÅ WojdyÅa
Qubes Tools for Windows developer
-----BEGIN PGP SIGNATURE-----

iQEcBAEBAgAGBQJVSlmzAAoJEIWi9rB2GrW7VoAH/RCrLC3IKwOMR7Q89v6QXW9S
xp4zrPwyDGYruhQfd4AfM/Dc3MY6AEc8T0v+/sSftVu+EMcL6nw831NgigPnkjVJ
z61GXhezGrk7Is6OqMmhnt6/Fhh3lQEuyx6pNefCaQQ/ytxrgmOi/SmM6YBgtr5N
WkyQ2XyXciSTH3YR3zzYgcNv4SrRdU3m1W3neBcLscm+DQ3MoTPqoqkx4GWea8xa
X7lsMrgRbtl/YR1Xxbb8pBi8mCebUl0UNr5CdFkOISimPO20ynrmOOtnhjNmdDUL
qexx8ddQ7SXZg7YqMqEBtXDDU9v/O+LPVNaDDe51ovGj95vC+GhF/n3BYQAzMx8=
=3zD9
-----END PGP SIGNATURE-----

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