Xen project Mailing List

On Tue, Jul 19, 2016 at 5:19 PM, Thomas Leonard <talex5@xxxxxxxxx> wrote:

On 19 July 2016 at 16:47, Martin Lucina <martin@xxxxxxxxxx> wrote:
> Hi all,
>
> the mirage-solo5 package (i.e. the Solo5 "platform bindings") currently
> bundle C stubs for unrelated packages which get linked into a
> libsolo5camlbindings.a.
>
> Based on some discussion on Slack and also ongoing in a PR Mindy pointed me
> to (https://github.com/mirage/io-page/pull/34), is the general plan to move
> as much C stubs as possible directly into the packages that actually use
> them?

Yes. Otherwise, we tend to fix bugs in only one copy, or change the
signature in only one place (and the typechecker can't help you here,
because they're C interfaces).

We should certainly do this when we can share the same source C file
across platforms. Currently the build tooling isn't very good. We have
a hacky oasis script that gets the Xen cflags. Hopefully this is
better with topkg, but I haven't tried that.

It's less clear what to do if the code is both platform and package
specific. In that case we should probably make a separate opam package
for it and avoid using the linker hack.

> If so, I have a bunch of work ahead of me and will need to postpone
> releasing Mirage/Solo5 to opam.ocaml.org until this is complete, since if I
> release what we have now then cleaning this up afterwards will be very
> painful.
>
> The following is an inventory of C stubs we've "accumulated" in
> mirage-solo5 so far:
>
> alloc_pages_stubs.c:
> caml_alloc_pages(), for io-page even though Solo5 devices do not
> *currently* require page-aligned buffers.
>
> barrier_stubs.c:
> caml_memory_barrier(): Not sure who is the downstream consumer of this?

This is used internally by mirage-platform, so I'd expect this to live
in mirage-platform-solo5.
Hmm, but it's also used by shared-memory-ring. I think the platform
should expose an OCaml interface to this.

In mirage-platform it looks like it's called by OCaml function Xenctrl.xen_mb(), but I think this function might be dead. I suspect the only user is the shared-memory-ring code and we forgot to remove this when we moved shared-memory-ring out of the platform?

> caml_cstruct_unsafe_load_uint32(), caml_cstruct_unsafe_save_uint32():
> Presumably consumed by cstruct?

Looks like it's actually shared-memory-ring using it. Odd.

IIRC the shared memory ring protocol requires the use of atomic 32-bit loads and stores, or else bad stuff happens. Cstruct uses ocplib-endian normally, but the ocplib-endian functions use (used to use?) a series of byte-sized loads and stores to cope with different possible alignments. This was not a fun bug to work on :-)

> checksum_stubs.c:
> A copy of the checksum stubs from tcpip. Should be built out of that
> package.

I think tcpip already builds this. We should remove it from mirage-platform now.

> clock_stubs.c:
> unix_gettimeofday(), caml_get_monotonic_time(): These are used by (at
> least) mirage-clock-xen, which we re-used for Solo5 since it has the
> same interfaces.

They're also used by the platform itself:
https://github.com/mirage/mirage-platform/blob/9f1a5db1b3681f4e3b4b747931903d7085cce042/xen/lib/time.ml#L34

> cstruct_stubs.c:
> caml_blit_bigstring_to_string(), caml_blit_string_to_bigstring(),
> caml_blit_bigstring_to_bigstring(), caml_compare_bigstring(),
> caml_fill_bigstring(), caml_check_alignment_bigstring().
>
> Based on the name (came from the original Xen bindings), I presume
> these are for cstruct?

Yes, should move to cstruct.

> main.c:
> caml_poll(): Used by the Mirage/Solo5 scheduler, this actually belongs
> here though could go in a better-named file.
>
> caml_get_cmdline(): Belongs in mirage-bootvar-solo5.
>
> mm.c:
> Unimplemented versions of stub_heap_get_pages_total(),
> stub_heap_get_pages_used(). These never get called, not sure who's
> supposed to be using them.

OS.MM (in mirage-platform) uses these:

https://github.com/mirage/mirage-platform/blob/9f1a5db1b3681f4e3b4b747931903d7085cce042/xen/lib/mM.ml

> solo5_{block,console,net}_stubs.c:
> These landed here as a shortcut while we were doing the port, they
> belong in the respective mirage-{block,console,net}-solo5 packages.
>
> Thoughts? Also any comments / corrections to the above inventory much
> appreciated.

I wonder if we could simplify things a little bit by standardising and documenting the C-level interface a little more? For example in the Io_page caml_alloc_pages we've got some #ifdefs to use `_xmalloc` on `__MINIOS__`, `malloc` (should really be `aligned_malloc`) on `_WIN32` and `posix_memalign` on the rest. Perhaps minios and solo5 should provide a `posix_memalign` function?

Cheers,

Dave

Re: [MirageOS-devel] Mirage/Solo5: What to do about C stubs?