New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Qubes target #553

Merged
merged 5 commits into from Nov 15, 2016

Conversation

Projects
None yet
5 participants
@yomimono
Member

yomimono commented Jul 8, 2016

This PR adds an additional mode for Qubes. If invoked with -t qubes, mirage will include in the generated main.ml some invocations of the libraries in mirage-qubes necessary to boot in that environment.

Much like the MacOSX target has a lot in common with Unix, Qubes is mostly the same as Xen with the exception of these extra invocations.

@avsm

This comment has been minimized.

Member

avsm commented Jul 8, 2016

fantastic -- so for build testing this a normal x86_64 linux is sufficient?

@yomimono

This comment has been minimized.

Member

yomimono commented Jul 8, 2016

Yes, it should build fine on such a system.

On July 8, 2016 11:50:02 AM GMT+01:00, Anil Madhavapeddy notifications@github.com wrote:

fantastic -- so for build testing this a normal x86_64 linux is
sufficient?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#553 (comment)

Composed on a touchscreen keyboard; please forgive typos and brevity.

@yomimono

This comment has been minimized.

Member

yomimono commented Jul 8, 2016

A notable TODO here is network configuration. Qubes doesn't, by default, provide a DHCP server on the bridge to which it attaches VMs, but rather expects the VMs to retrieve network configuration information from QubesDB (see @talex5's https://github.com/talex5/qubes-mirage-skeleton for an example). I'm not sure how to implement this nicely in the current paradigm; I'd really like not to have to change anything in config.ml when testing on a vanilla Xen machine versus on my Qubes laptop.

@hannesm

This comment has been minimized.

Member

hannesm commented Jul 8, 2016

I don't know much about Qubes startup, but your comments sound that adding a target qubes to the mirage tool will lead to any unikernels of the mirage-skeleton project to work flawlessly and out-of-the-box as qubes guests? If this is the case, that is awesome! I suspect since you mention network configuration, that some minor code (hidden in a match (which we have for unix and xen nowadays anyways to support direct/socket stacks)) changes are necessary, which could be upstreamed since there is another backend (qubes, next to xen)?

Or do I misunderstand the aims of this PR and what the idea of the qubes target is (which initially felt slightly wrong since it is just a xen)?

Fmt.strf
"@[<v 2>\
%s.connect ~domid:0 () >>= fun qrexec ->@ \
Lwt.async (fun () ->@ \

This comment has been minimized.

@talex5

talex5 Jul 9, 2016

Contributor

Not sure about this (disconnecting rexec whenever we receive a shutdown request).
Seems more like something the main unikernel application should decide about
(qubes-mirage-skeleton just happens to respond to rexec finishing by exiting).

match Key.(get (Info.context i) target) with
| `Unix | `MacOSX | `Xen ->
Fmt.strf
"return (`Ok ())"

This comment has been minimized.

@talex5

talex5 Jul 9, 2016

Contributor

We should probably report an error if you ask for rexec on a non-Qubes platform.

This comment has been minimized.

@Drup

Drup Jul 21, 2016

Member

I agree, the best is to use the configure method for that purpose (until we have a better solution, see mirage/functoria#12).

In any cases, having the two connect implementation that returns different types is a bad idea.

@talex5

This comment has been minimized.

Contributor

talex5 commented Jul 9, 2016

I think we need to expose at least QubesDB and QRExec in the Mirage API (mirage.mli). Most Qubes unikernels will want access to these.
Networking can depend on default_qubesdb on Qubes by default, I think.

It's not clear what should call the listen functions though. e.g., if the unikernel wants to provide any qrexec command handlers then it needs to call listen itself. But a generic unikernel should get some default handlers.

There's a similar problem with shutdown events. If a unikernel doesn't provide any shutdown handler then it should probably just exit when requested. But if it does, we want to use that.

@yomimono

This comment has been minimized.

Member

yomimono commented Jul 10, 2016

@hannesm - yes, you've got the right sense of it (including the network configuration targets).

@talex5 - it does seem like a solution which gives default handlers, but also allows the impl for QRExec and QubesDB to be passed to the unikernel, is what's really required. In that case we do indeed need to expose these in the mli. The frontend tool could then pass the initialized impls along to the unikernel's start. The listen functions are still a problem there -- they could be passed along in an optional argument from config.ml if the user wants them, I suppose.

"@[<v 2>\
%s.connect ~domid:0 () >>= fun gui ->@ \
Lwt.async (fun () -> %s.listen gui);@ \
Lwt.return (`Ok gui)"

This comment has been minimized.

@Drup

Drup Jul 21, 2016

Member

You don't close your boxes. :)

@Drup

This comment has been minimized.

Member

Drup commented Jul 21, 2016

Did you forgot to commit the changes to Mirage_key ?

What should be the behavior of Key.is_xen ? For the Solo5 branch, all the occurrences of Key.is_xen have been removed. Given the proliferation of targets, maybe it's the best solution.

@yomimono

This comment has been minimized.

Member

yomimono commented Jul 21, 2016

Did you forgot to commit the changes to Mirage_key ?

I forgot to commit a bunch of changes, apparently. I definitely had this building at one point but have either trashed the rest of the state or hidden it from myself extremely effectively, so I'm reconstructing it now. :/

@yomimono

This comment has been minimized.

Member

yomimono commented Jul 21, 2016

Just like MacOSX is true for is_unix, I set Qubes to be true for is_xen. In the case of Qubes it's pretty much true; Qubes literally is Xen with extra bits added.

@Drup

This comment has been minimized.

Member

Drup commented Jul 21, 2016

Ok, the new part looks good to me. From what I can see, the old part mainly need proper failure in case of incompatibility.

@yomimono

This comment has been minimized.

Member

yomimono commented Jul 21, 2016

Thanks very much for looking at it so closely! I'm writing a patch that moves the target matching logic to configure as you suggested.

@yomimono yomimono changed the title from [RFC] add Qubes target to [RFC, WIP, BBQ] add Qubes target Jul 21, 2016

@yomimono yomimono self-assigned this Sep 15, 2016

@yomimono

This comment has been minimized.

Member

yomimono commented Nov 14, 2016

This rebase includes commits that enable autoconfiguration of network stacks. This was the last major TODO keeping mirage configure -t qubes from "just working" in most examples with no changes to config.ml or the application code.

However, most of @talex5's very reasonable comments upthread aren't quite addressed. I'd like to merge this first and then submit another patch implementing the optional designation of custom listeners and handlers; I'd appreciate another look at this from anyone who's still interested.

@yomimono yomimono changed the title from [RFC, WIP, BBQ] add Qubes target to add Qubes target Nov 14, 2016

(if_impl Key.(pure ((=) `Socket) $ net_key)
(socket_stackv4 ?group [Ipaddr.V4.any])
(direct_autochooser ?group ?config ?dhcp_key tap)
)

This comment has been minimized.

@hannesm

hannesm Nov 14, 2016

Member

at least I'm slightly confused by the nested conditionals here, which now call out to another function (only referenced once)... could we instead use a match_impl over several keys to make the code a bit clearer, please?

This comment has been minimized.

@yomimono

yomimono Nov 14, 2016

Member

It is pretty confusing :( the other function is there to attempt to make it a bit more readable. Using match_impl isn't very nice in this case because we want to consider several different values, and what goes in there isn't actually a pattern match. I'll think it over a bit more.

This comment has been minimized.

@hannesm

hannesm Nov 14, 2016

Member

it doesn't hurt to evaluate the keys, or does it? if not, then do some bindings and use a standard match on them, maybe?

This comment has been minimized.

@yomimono

yomimono Nov 14, 2016

Member

Hm, this reminds me quite a bit of a conversation @Drup and I had related to #643 , which I'm now trying to find... the conversation I recalled was irrelevant, but note we don't have a context when calling generic_stackv4 so we can't call eval on these values: https://github.com/mirage/functoria/blob/master/lib/functoria_key.mli#L270 . I think I can do something with map and a tuple and maybe make things more readable, though.

@@ -724,7 +810,7 @@ let udpv4_socket_conf ipv4_key = object
method libraries =
Key.match_ Key.(value target) @@ function
| `Unix | `MacOSX -> [ "tcpip.udpv4-socket" ]
| `Xen | `Virtio | `Ukvm -> failwith "No socket implementation available for unikernel"
| `Xen | `Virtio | `Ukvm | `Qubes -> failwith "No socket implementation available for unikernel"

This comment has been minimized.

@hannesm

hannesm Nov 14, 2016

Member

maybe if_unix?

This comment has been minimized.

@yomimono

yomimono Nov 15, 2016

Member

It seems the behavior of Key.match_ and a pattern is subtly different in the evaluation results from Key.if_ is_unix -- replacing this pattern match as suggested results in an immediate failure for any mirage configure with the "No socket implementation" message. Possibly both branches of the if are evaluated in that case (and only the one which matches the actual value used), but this isn't done with match_?

This comment has been minimized.

@hannesm

hannesm Nov 15, 2016

Member

then please ignore my comment and leave it as is for now... would also be nice to squash your commits here into a single one before merging... thx!

@@ -771,7 +857,7 @@ let tcpv4_socket_conf ipv4_key = object
method libraries =
Key.match_ Key.(value target) @@ function
| `Unix | `MacOSX -> [ "tcpip.tcpv4-socket" ]
| `Xen | `Virtio | `Ukvm -> failwith "No socket implementation available for unikernel"
| `Xen | `Virtio | `Ukvm | `Qubes -> failwith "No socket implementation available for unikernel"

This comment has been minimized.

@hannesm

hannesm Nov 14, 2016

Member

maybe if_unix

@@ -1142,7 +1254,7 @@ let mprof_trace ~size () =
MProf.Trace.Control.start trace_config@]"
Key.serialize_call (Key.abstract key)
unix_trace_file;
| `Xen | `Virtio | `Ukvm ->
| `Xen | `Virtio | `Ukvm | `Qubes ->

This comment has been minimized.

@hannesm

hannesm Nov 14, 2016

Member

here, Virtio | Ukvm should be removed and lead to an invalid_arg or similar.. (no tracing on solo5)

@hannesm

This comment has been minimized.

Member

hannesm commented Nov 14, 2016

I added some notes (skipped over the qubes-specific code) -- LGTM!

@yomimono

This comment has been minimized.

Member

yomimono commented Nov 14, 2016

Thanks @hannesm! :)

hannesm and others added some commits Nov 14, 2016

Merge pull request #676 from hannesm/sort-pkgconfig
pkg-config: share before lib, also only request mirage-solo5 (which h…
`Dhcp, dhcp_ipv4_stack ?group tap;
`Socket, socket_stackv4 ?group [Ipaddr.V4.any];
`Qubes, qubes_ipv4_stack ?group tap;
] ~default:(static_ipv4_stack ?config ?group tap)

This comment has been minimized.

@hannesm

hannesm Nov 15, 2016

Member

this looks much better! thanks!

yomimono added some commits Sep 26, 2016

provide qubes target; allow network autoconfigure
Add the mode `qubes`, which generally behaves the same as `Xen` with a few
exceptions noted below.

For the `qubes` target, provide an always-on qubes gui
listener and qrexec daemon, which are
initialized at unikernel start when the unikernel is configured with -t
qubes.  In other configurations they are not invoked.

Provide and expose a default_qubesdb in mirage.mli .

make argv_qubes, which passes only valid runtime_keys to bootvar.
This will only pass those runtime arguments which are expected to
the Functoria runtime, and therefore to Cmdliner.  Passing unexpected
arguments to Functoria results in runtime crashes, so this is needed
to reliably boot in environments where the supplied arguments can't be
easily controlled, such as Qubes or an external cloud hosting service.

make and expose qubes_ipv4_stack; generic_stackv4 chooses it when target is qubes.
`qubes_ipv4_stack` uses the Qubesdb_ipv4 sublibrary of `mirage-qubes`
along with `default_qubesdb` to
figure out the correct ipv4 settings and build a stackv4 based on them.
`generic_stackv4` chooses `qubes_ipv4_stack` rather than socket-based,
statically assigned, or dhcp-configured ipv4 if `mirage configure` is
invoked with `-t qubes`.

@yomimono yomimono merged commit b3669c5 into mirage:master Nov 15, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

yomimono added a commit to yomimono/mirage that referenced this pull request Nov 15, 2016

yomimono added a commit that referenced this pull request Nov 15, 2016

@hannesm

This comment has been minimized.

Member

hannesm commented on lib/mirage.ml in a7cff94 Nov 15, 2016

This may be reverted (according to your explanation in #553 (comment) this leads to failing behaviour)

@hannesm

This comment has been minimized.

Member

hannesm commented on lib/mirage_key.ml in a7cff94 Nov 15, 2016

wasn't is_xen on the list of things to not have in 3.0?

@yomimono

This comment has been minimized.

Member

yomimono commented Nov 15, 2016

I'd like to remove it, yes; feel free to PR if you get to it first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment