rust-vmm / vmm-sys-util

Helpers and utilities used by multiple rust-vmm components and VMMs
BSD 3-Clause "New" or "Revised" License
78 stars 64 forks source link

Allow building on 32bit platforms without AtomicU64 #187

Open mjt0k opened 1 year ago

mjt0k commented 1 year ago

Hi!

This code has src/metric.rs, which unconditionally uses std::sync::atomic::AtomicU64. Quite some platforms don't have 64-bit atomics with current rust, namely it is armel, mipsel, powerpc and x32 (heh). Maybe the metric module can be guarded with the right #[cfg target_has_atomic] ?

Trying to package some stuff for Debian, this crate come as a dependency...

Thanks!

mjt0k commented 1 year ago

Created a PR for this, !188

roypat commented 1 year ago

Hi! Thanks for the report. Out of curiosity, what crate with a vmm-sys-util dependency are you trying to compile for a 32bit target? The support section of the readme only mentions x86_64 and aarch64 as supported platforms, and as far as I'm aware, no hypervisors built on top of rust-vmm support 32 bit hosts (although @andreeaflorescu might know more here).
There are probably a lot of things that would have to be changed, especially wrt FFI bindings, for rust-vmm to support 32bit targets.

mjt0k commented 1 year ago

Yes I know the README says it's for two architectures (amd64 & aarch64). Unfortunately, other projects started to use parts of this code, mainly eventd/epoll stuff, and this is causing.. issues.

For the time being, the thing I'm trying to build is virtiofsd — https://gitlab.com/virtio-fs/virtiofsd — with its dependencies. This is needed for qemu (which works on many platforms), where, starting with version 8.0, they switched from internal C-language implementation of virtiofsd to external rust-language one. And it turned out quite some crates use stuff provided by vmm-sys-utils. Maybe it's a good idea to package some of those bits as separate crates, even including into the std rust runtime (as those are mainly wrappers for the syscalls).

roypat commented 1 year ago

Ah, I see. I'm a bit afraid here that vmm-sys-util is just the tip of the iceberg, and that you'll discover more issues further down the line (virtiofds pulls in 4 more rust-vmm dependencies, and all of the have the same supported platforms as vmm-sys-util). But for now, conditionally compiling out the parts that don't work on x32 (or other platforms) sounds reasonable to me.

Another thing to keep in mind though is that we only continuously test on x86_64 and aarch64 platforms currently, so even if everything compiles, there's no guarantee that the result is "correct" on other platforms. If 32bit support is desired, there'd need to be people willing to maintain it, and integrate it into our testing infrastructure to make sure that it doesn't get accidentally broken again further down the line.

Fabian-Gruenbichler commented 1 year ago

FWIW, also opened https://gitlab.com/virtio-fs/virtiofsd/-/issues/94 to see what virtiofsd upstream thinks about supported archs/targets

mjt0k commented 1 year ago

But for now, conditionally compiling out the parts that don't work on x32 (or other platforms) sounds reasonable to me.

I looked at this more closely. While I don't have preferences for other 32bit platforms which lacks 64bit atomics for now, I do have a word about x32. This one does not need extra efforts, - if it doesn't work and needs some special care to maintain, those efforts are better spent elsewhere. The c_long vs i64 (the other issue) is worth to fix anyway (which will bring x32 in line with the rest of 32bit arches)

Speaking of conditionally compiling 64bi atomics, unfortunately this wont help, as other crates (which are in deps of virtiofsd) use vmm_sys_utils::metrics anyway. It will let vmm-sys-utils itself to compile but not other more useful crates.