libkrun X11 pipewire support

[dorindabassey]

This series adds cross-domain IPC support for X11 and PipeWire, enabling better graphics with enhanced file descriptor and futex sharing capabilities.

[gurchetansingh]

O_o ... this is a tricky one. I think the core thing you must try to do here is try to fit the cross-platform abstraction we're trying to create.

• You should use rustix, not libc or nix
• For EVENT_FD, we already have an abstraction for mesa3d_util::Event. Use that.
• For futex, you need to create that. The good thing is:

https://github.com/m-ou-se/atomic-wait

has a pretty solid API for this over Linux (futex), Windows (WaitOnAddress), and MacOS. Do the Linux implementation, but we'll need stubs for Windows/MacOS (hopefully eventually we'll get to them too). Use rustix instead of libc (most of your changes will be mesa3d_util). We of course need to properly attribute atomic-wait for the adaptation, so I think the resultant code needs to be BSD-2 licensed.

And also, can you test this change with muvm + libkrun + latest rutabaga_gfx? That'll help us have confidence the change works before cut the release.

Thank you!

[dorindabassey]

O_o ... this is a tricky one. I think the core thing you must try to do here is try to fit the cross-platform abstraction we're trying to create.

* You should use `rustix`, not `libc` or `nix`

* For `EVENT_FD`, we already have an abstraction for `mesa3d_util::Event`.  Use that.

* For `futex`, you need to create that.  The good thing is:

https://github.com/m-ou-se/atomic-wait

has a pretty solid API for this over Linux (futex), Windows (WaitOnAddress), and MacOS. Do the Linux implementation, but we'll need stubs for Windows/MacOS (hopefully eventually we'll get to them too). Use rustix instead of libc (most of your changes will be mesa3d_util). We of course need to properly attribute atomic-wait for the adaptation, so I think the resultant code needs to be BSD-2 licensed.

And also, can you test this change with muvm + libkrun + latest rutabaga_gfx? That'll help us have confidence the change works before cut the release.

Thank you!

Thank you for the feedback, I have addressed the issues and incorporated your suggestions to use rustix. I also tested these changes with muvm from (git clone -b val/kqloytrpuwkm https://github.com/valpackett/muvm) + libkrun from (git clone -b fixkrun https://github.com/dorindabassey/libkrun) + latest rutabaga_gfx(this PR) by running xeyes in the vm.

[gurchetansingh]

Sorry for the late response (holiday season). Needs to pass the CI, and try to make it more OS-agnostic: i.e, less (target_os=linux)

CC: @valpackett for any other review comments.

[gurchetansingh]

should we be using descriptor.determine_type()

[dorindabassey]

I changed it to use descriptor.determine_type() but i guess I'll have to upstream this to mesa aswell, is that right?

[gurchetansingh]

Okay, finally wrapping my head around what this is trying to do and have more substantive feedback. I think we should create a:

third_party/mesa3d/src/util/rust/atomic_memory_sentinel.rs file.

and that would be the access point to futex and WaitOnAddress like APIs.

use crate::sys::platform::futex as futex

AtomicMemorySentinel {
     memory_mapping: MemoryMapping,
     atomic_ptr: *mut AtomicU32,
     value: ..
}

impl Drop for AtomicMemorySentinel {
fn drop(&mut self) {
futex::wake_all(...)
}
}

impl AtomicMemorySentinel {
pub fn new(mapping: MemoryMapping) -> MesaResult
pub fn wait() -> MesaResult<()>;
pub fn signal() -> MesaResult<()>

}

And then in the CrossDomain, code you can work in terms of Arc<Mutex<AtomicMemorySentinel>. I feel like the current CrossDomainMemoryWatcher is mixing thread operations, atomic memory operations.

Plus, AtomicMemorySentinel sounds coller than MemoryWatcher, does it not?!?

Also, cross_domain.rs is getting rather large. Feel free to split into seperate files: I.e, cross_domain_worker.rs, cross_domain_atomic_memory_sentinel_worker.rs. Thanks again for your patience!

[dorindabassey]

Okay, finally wrapping my head around what this is trying to do and have more substantive feedback. I think we should create a:

third_party/mesa3d/src/util/rust/atomic_memory_sentinel.rs file.

and that would be the access point to futex and WaitOnAddress like APIs.

use crate::sys::platform::futex as futex

AtomicMemorySentinel {
     memory_mapping: MemoryMapping,
     atomic_ptr: *mut AtomicU32,
     value: ..
}

impl Drop for AtomicMemorySentinel { fn drop(&mut self) { futex::wake_all(...) } }

impl AtomicMemorySentinel { pub fn new(mapping: MemoryMapping) -> MesaResult pub fn wait() -> MesaResult<()>; pub fn signal() -> MesaResult<()>

}

And then in the CrossDomain, code you can work in terms of Arc<Mutex<AtomicMemorySentinel>. I feel like the current CrossDomainMemoryWatcher is mixing thread operations, atomic memory operations.

Plus, AtomicMemorySentinel sounds coller than MemoryWatcher, does it not?!?

Also, cross_domain.rs is getting rather large. Feel free to split into seperate files: I.e, cross_domain_worker.rs, cross_domain_atomic_memory_sentinel_worker.rs. Thanks again for your patience!

Thanks for the detailed feedback! I've addressed all the other points. Regarding splitting cross_domain/mod.rs into separate files (cross_domain_worker.rs, cross_domain_atomic_memory_sentinel_worker.rs): I'd prefer to defer this to a follow-up change since I need to rebase this series first, and the file split would make the rebase messy.
Also I don't know what the last CI error is about. I thought I removed linux_raw_sys

[gurchetansingh]

While you don't need to refactor existing code, I still you should encapsulate the new functionality into it's own file to the extent possibility, since it does introduce complex spread throughout the place.

I suggest the creation of atomic_memory_sentinel_manager.rs in the src/cross_domain dir. That can encapsulate management of the AtomicMemorySentinelThread (s) and access to them. You don't have prefix everything with CrossDomain in src/cross_domain.rs -- I think we'll refactor that pattern later.

mod.rs

 struct CrossDomainContext {
     watcher_manager: Arc<Mutex<AtomicMemorySentinelManager>>,
     worker_thread: Option<thread::JoinHandle<()>>,
 }
 
 impl CrossDomainContext {
     fn memory_watcher_new(&mut self, cmd: &CreateNewWatcherCmd) -> Result<()> {
         // Lock the mutex to get mutable access to the manager
         let mut manager = self.watcher_manager.lock().unwrap();
         let (id, eventfd_descriptor) = manager.create_watcher(cmd.fs_id, cmd.handle)?;
 
         self.state.add_job(CrossDomainJob::AddPolledDescriptor(id, eventfd_descriptor));
         Ok(())
     }
 
     fn memory_watcher_destroy(&mut self, cmd: &DestroyWatcherCmd) -> Result<()> {
         let mut manager = self.watcher_manager.lock().unwrap();
         manager.destroy_watcher(cmd.id)?;
 
         self.state.add_job(CrossDomainJob::RemovePolledDescriptor(cmd.id));
         Ok(())
     }
 }
 
 
 struct CrossDomainWorker {
     watcher_manager: Arc<Mutex<AtomicMemorySentinelManager>>,
     wait_ctx: WaitContext,
 }
 
 impl CrossDomainWorker {
     fn handle_fence(&mut self) -> Result<()> {
         let events = self.wait_ctx.wait()?;
         if let Some(event) = events.first() {
             if event.id >= MEMORY_WATCHER_ID_START {
                 let manager = self.watcher_manager.lock().unwrap();
 
                 let ring_message = manager.handle_event(event.id, &event.descriptor)?;
 
                 self.state.write_to_ring(RingWrite::Write(ring_message, None), ...)?;
             }
         }
         Ok(())
}

atomic_memory_sentinel_manager.rs
 pub struct AtomicMemorySentinelManager {
     watchers: Map<u32, SentinenlInstance>,
     next_id: u32,
     virtiofs_table: VirtioFsTable, // A reference to the required dependency
 }
 
 impl AtomicMemorySentinelManager {
     pub fn new(virtiofs_table: VirtioFsTable) -> Self {
         Self {
             watchers: Map::new(),
             next_id: MEMORY_WATCHER_ID_START,
             virtiofs_table,
         }
     }
 
     pub fn create_watcher(&mut self, fs_id: u64, handle: u64) -> Result<(u32, OwnedDescriptor)> {
         let eventfd = Event::new()?;
         let thread_eventfd = eventfd.try_clone()?;
 
         let join_handle = thread::spawn(move || {
             // Thread logic that waits on the sentinel and signals `thread_eventfd`.
         });
 
         Ok((id, eventfd.into()))
     }
 
     pub fn handle_event(&self, id: u32, event_fd: &Event) -> Result<RingMessage> {
        .. 
         Ok(message)
     }
 
     pub fn destroy_watcher(&mut self, id: u32) -> Result<()> {
        ...
     }
 }
 
 impl Drop for AtomicMemorySentinelManager {
     fn drop(&mut self) {
         for (_, watcher) in self.watchers.drain() {
             watcher.shutdown();
         }
     }
 }

[gurchetansingh]

hmm, this will introduce a linux-raw-sys dependency in Android, which the Rust team will not be happy about. I say keep this, and we'll have to get another patch through rustix before merged, but don't add a linux-raw-sys dep in the meson builds.

[gurchetansingh]

Mostly nits, but I think we need to get rid of the double-atomicity and leaking a reference to the atomic value.

I'll also prepare a CL to solve the Rustix/Android issue..

[dorindabassey]

Mostly nits, but I think we need to get rid of the double-atomicity and leaking a reference to the atomic value.

I'll also prepare a CL to solve the Rustix/Android issue..

Thanks! I've fixed the pending issues now.

[gurchetansingh]

Apologies once again for the delay! Rustix patch almost complete, undergoing testing internally. In the meantime, can you send the mesa3d_util change to upstream Mesa?

ETA for new rutabaga crates.io release at this pace: ~3 weeks

Thank you!

[dorindabassey]

Apologies once again for the delay! Rustix patch almost complete, undergoing testing internally. In the meantime, can you send the mesa3d_util change to upstream Mesa?

ETA for new rutabaga crates.io release at this pace: ~3 weeks

Thank you!

NO worries! Thank you! Submitted the PR here - https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/39653

[gurchetansingh]

bytecodealliance/rustix#1577 <-- Rustix pull request here, could take a few weeks to get a review tho

[gurchetansingh]

@dorindabassey the Rustix MR is taking more time than expected.

As a stopgap to make sure you're not blocked, I created a libkrun specific branch and associated crates.io releases:

https://github.com/magma-gpu/rutabaga_gfx/commits/libkrun
https://crates.io/crates/mesa3d_util/0.1.76-libkrun.0
https://crates.io/crates/rutabaga_gfx/0.1.76-libkrun.0

This is similar to the tricks used to support chromeos too. Definitely will merge this into main once we have rustix issues fixed, but in the meantime feel free to use the libkrun crates.io releases to land the rest of the changes you need.

[dorindabassey]

@dorindabassey the Rustix MR is taking more time than expected.

As a stopgap to make sure you're not blocked, I created a libkrun specific branch and associated crates.io releases:

https://github.com/magma-gpu/rutabaga_gfx/commits/libkrun https://crates.io/crates/mesa3d_util/0.1.76-libkrun.0 https://crates.io/crates/rutabaga_gfx/0.1.76-libkrun.0

This is similar to the tricks used to support chromeos too. Definitely will merge this into main once we have rustix issues fixed, but in the meantime feel free to use the libkrun crates.io releases to land the rest of the changes you need.

Thanks! This would be helpful, also the Mesa MR is still pending.

[gurchetansingh]

also the Mesa MR is still pending

Ack, but merging that would also break Android unfortunately, and it's much less easier/useful to create branches for Mesa.

[valpackett]

Sorry for dragging this along even more :) but one extra thing I'm currently working on (for libkrun) is D-Bus over virtgpu, so we could have D-Bus integration with FD-passing (for XDG portals that require it). Camera and screen capture pass PipeWire remotes, but printing and file transfer for drag&drop/copy&paste pass regular file fds. Well, printing does, but file transfer passes O_PATH descriptors (despite the document-portal preferring "normal" ones now). And that's what I wanted to discuss here:

this current type VirtioFsTable = Arc<Mutex<Map<VirtioFsKey, File>>> won't work for O_PATH fds, as in that case there's no handle at all, just the inode number. So I'd have to add a different map/thingy to be able to look up inodes.

But anyway also this whole Mutex<Map<…>> is straight out of libkrun's implementation details and a different VMM might use a different data structure to store the FDs. (And maybe message-passing instead of mutex-based sharing for concurrency!)

Can we instead make a trait like

pub trait VirtioFsLookup {
    async fn get_open_file(fs_id: u64, handle: u64) -> Result<File>;
}

and then store an Option<dyn VirtioFsLookup + Send + Sync> or something…?

(And then conveniently I'll be able to extend it with a method that returns an O_PATH "File" based on an inode instead of a handle)

[dorindabassey]

Sorry for dragging this along even more :) but one extra thing I'm currently working on (for libkrun) is D-Bus over virtgpu, so we could have D-Bus integration with FD-passing (for XDG portals that require it). Camera and screen capture pass PipeWire remotes, but printing and file transfer for drag&drop/copy&paste pass regular file fds. Well, printing does, but file transfer passes O_PATH descriptors (despite the document-portal preferring "normal" ones now). And that's what I wanted to discuss here:

this current type VirtioFsTable = Arc<Mutex<Map<VirtioFsKey, File>>> won't work for O_PATH fds, as in that case there's no handle at all, just the inode number. So I'd have to add a different map/thingy to be able to look up inodes.

But anyway also this whole Mutex<Map<…>> is straight out of libkrun's implementation details and a different VMM might use a different data structure to store the FDs. (And maybe message-passing instead of mutex-based sharing for concurrency!)

Can we instead make a trait like

pub trait VirtioFsLookup {
    async fn get_open_file(fs_id: u64, handle: u64) -> Result<File>;
}

and then store an Option<dyn VirtioFsLookup + Send + Sync> or something…?

(And then conveniently I'll be able to extend it with a method that returns an O_PATH "File" based on an inode instead of a handle)

I'm currently working on implementing the trait-based approach you suggested, though I made some adjustments from your proposal:
pub trait VirtioFsLookup: Send + Sync {
fn get_open_file(&self, fs_id: u64, handle: u64) -> RutabagaResult;
}
I decided to make it sync because The codebase has no async runtime, and the call site in AtomicMemorySentinelManager::create_watcher is synchronous. Is this ok for you? or do you prefer the async version?
For the O_PATH support, we can extend the trait with an optional method for inode lookups.
but I wonder if this is something to do in this PR, or create a new PR for it?

[valpackett]

@dorindabassey sync is fine for everything right now, the possibility of using async is something I suggested for the very abstract possibility of other/future VMMs wanting to use async :)

For the inode method, just the possibility that I could add it to the trait in a future PR would be excellent.

[mtjhrc]

Just a small suggestion, since this is switching to a more general trait anyway:

I think the get_open_file should return OwnedDescriptor instead of File. That seems more semantically correct and the code converts it to an OwnedDescriptor immediately anyway. And maybe call it something like get_exported_fd?

[dorindabassey]

Just a small suggestion, since this is switching to a more general trait anyway:

I think the get_open_file should return OwnedDescriptor instead of File. That seems more semantically correct and the code converts it to an OwnedDescriptor immediately anyway. And maybe call it something like get_exported_fd?

Right, Thanks! the PR is opened on #44