bind minios function virt_to_mfn

[cfcs]

This is currently needed for speaking to Qubes GUId, suspect it could be useful for other applications dealing with RPC between Xen domU too.

ping @hannesm who complained this wasn't upstreamed :)

[hannesm]

thanks @cfcs. I'm curious about naming (since mfn is not obvious to me, should we rather write its full name?). any other opinions on naming?

[cfcs]

It wasn't obvious to me either, but mfn is the Xen term, and virt_to_mfn is the name of the C function bound. This function is only of use if you're doing something Xen-related, in which case you'll go looking for virt_to_mfn - there's no "civilian" use case.

I think it's better to align with Xen and use the upstream terminology (and function names, where we are just wrapping their API).
Doing so makes it easier to reason about code if you know what the C code does, and if you don't know what it does then it's easier to look up information. The PR includes some links in the doc-string, and a brief explanation in the C stub also, for when you're browsing the C code - I feel that should be enough. Again, this function is only used when confronted with the explicit need to communicate MFNs, in which case whatever you're communicating with will also be talking about MFNs.

[hannesm]

Thanks for the clarification, @cfcs. This is IMHO good to go, but I'd be happy if someone with more C-OCaml knowledge would approve the changes (esp. regarding required allocations).

//cc @djs55 @samoht @yallop @talex5

[cfcs]

FWIW I have no idea if it's correct; I've been using it for ~six months without obvious problems, but I have no good way to tell if it is leaking memory, or is potentially returning wrong results. I tried to read the instructions in caml/memory.h, but it made me none the wiser, and doesn't mention the need to free() any of these variables. It is my impression that the CAMLreturn macro takes care of marking stuff for collection, but this is not an empirical assertion.

Looking at the defintions of CAMLreturn and CAMLlocal I wonder if using the local variable is overkill.

I also wonder if Nativeint_val may be wrong - is Nativeint aligned with uintptr_t?
Maybe. See config.h I guess. Is it a problem that intnat is a signed integer? Maybe, probably not, probably depends on the architecture. Thank god Xen doesn't run on that many.
virt_to_mfn(x) seems to end up in a macro casting to (unsigned long)(x) before processing, and it also returns an unsigned long, so that seems fine for all architectures where intnat is long and sizeof(unsigned long) == sizeof(long) == sizeof(intnat).
My guess is that nativeint will fit a pointer on any platform Xen runs on, today, but I have no idea how to confirm it.

These anxieties are kind of the reason for not upstreaming earlier, I've yet to find a guide on how to deal with things like this.

EDIT: Seems like OCaml doesn't use pointers for its values, but uses signed ints instead.s/pointer/sint/g above.

[yallop]

My guess is that nativeint will fit a pointer on any platform Xen runs on, today, but I have no idea how to confirm it.

Being pointer-width is part of the interface to nativeint. The nativeint documentation says:

This integer type has exactly the same width as that of a pointer type in the C compiler.

[mato]

@cfcs I didn't know OCaml had a nativeint, that does indeed look like the correct type to use here. As for whether or not you need the CAMLlocal1; there are places in the Solo5 bindings where I do a direct CAMLreturn(caml_copy_int64(...)) so I guess not.

On a more general note, I realise we don't have any hard guidelines on what goes in OS but I'd be happier if this Xen-specific interface were exposed as OS.Xen.virt_to_mfn rather than going in OS.Heap_pages. See mirage/mirage-solo5@61925cf for an example of a Solo5-specific interface done this way.

[cfcs]

@mato I think that makes sense, to have backend-specific modules separate from the rest. Ideally I guess there'd be some kind of common signatures for these shared modules to avoid the different backends growing apart?

[mato]

On Wednesday, 21.03.2018 at 09:08, cfcs wrote: @mato I think that makes sense, to have backend-specific modules separate from the rest. Ideally I guess there'd be some kind of common signatures for these shared modules to avoid the different backends growing apart?

You mean for the parts of OS.mli which are supposed to be provided by all backends? Ideally yes, but there is no such thing at the moment -- this is also why things like mirage-os-shim exist. Hopefully we can fix this as part of the mirage-platform refactor into -xen and -unix.

[hannesm]

@cfcs could you move the virt_to_mfn functionality into OS.Xen.virt_to_mfn as proposed by @mato -- then we should be good to go! :)

[cfcs]

I moved to a new Xen module and force-pushed (IMHO everything Xen-only should have been under a Xen module to begin with, but it doesn't make sense to break everything by moving now, so ... ) .

[hannesm]

@mato since you requested the last change (now known as OS.Xen.virt_to_mfn), do you approve this PR?

[mato]

@hannesm Yes, the change looks fine, however looking at the raw commit, the Author: is Your Name <[email protected]> -- is that OK for committing to MirageOS?

[cfcs]

@mato That's my moniker, what harm could it do?

[mato]

@mato That's my moniker, what harm could it do?

@cfcs I don't mind pseudonyms but the author email goes in the Git history, and I'd prefer that we have a valid-in-good-faith email as a point of contact in case we ever need to contact contributors. I very much doubt that [email protected] is indeed your email address :-)

[hannesm]

this looks reasonable to me, and has been reviewed by several people. I don't see a reason to require a (valid!?!) email address for maybe contacting contributors -- this is ISC licensed anyways, and email addresses are getting invalid over time (if you leave an organisation / company / university).