Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU

[Kevin Wolf]

Am 12.02.2025 um 10:28 hat Paolo Bonzini geschrieben:
> On 2/11/25 22:43, Kevin Wolf wrote:
> > +/// Use QEMU's event loops to run a Rust [`Future`] to completion and 
> > return its result.
> > +///
> > +/// This function must be called in coroutine context. If the future isn't 
> > ready yet, it yields.
> > +pub fn qemu_co_run_future<F: Future>(future: F) -> F::Output {
> > +    let waker = Arc::new(RunFutureWaker {
> > +        co: unsafe { bindings::qemu_coroutine_self() },
> > +    })
> > +    .into();
> 
> into what? :)  Maybe you can add the type to the "let" for clarity.

Into Waker. Do we have any idea yet how explicit we want to be with type
annotations that aren't strictly necessary? I didn't think of making it
explicit here because the only thing that is done with it is building a
Context from it, so it seemed obvious enough.

If you think that being explicit is better, then Waker::from() might
be better than adding a type name to let and keeping .into().

> > +    let mut cx = Context::from_waker(&waker);
> > +
> > +    let mut pinned_future = std::pin::pin!(future);
> > +    loop {
> > +        match pinned_future.as_mut().poll(&mut cx) {
> > +            Poll::Ready(res) => return res,
> 
> Alternatively, "break res" (matter of taste).
> 
> > +            Poll::Pending => unsafe {
> > +                bindings::qemu_coroutine_yield();
> > +            },
> > +        }
> > +    }
> > +}
> > +/// Wrapper around [`qemu_co_run_future`] that can be called from C.
> > +///
> > +/// # Safety
> > +///
> > +/// `future` must be a valid pointer to an owned `F` (it will be freed in 
> > this function).  `output`
> > +/// must be a valid pointer representing a mutable reference to an 
> > `F::Output` where the result can
> > +/// be stored.
> > +unsafe extern "C" fn rust_co_run_future<F: Future>(
> > +    future: *mut bindings::RustBoxedFuture,
> > +    output: *mut c_void,
> > +) {
> > +    let future = unsafe { Box::from_raw(future.cast::<F>()) };
> > +    let output = output.cast::<F::Output>();
> > +    let ret = qemu_co_run_future(*future);
> > +    unsafe {
> > +        *output = ret;
> 
> This should use output.write(ret), to ensure that the output is written
> without dropping the previous value.

Oops, thanks. I need to learn that unsafe Rust still isn't C. :-)

> Also, would qemu_co_run_future() and qemu_run_future() become methods on an
> Executor later?  Maybe it make sense to have already something like
> 
> pub trait QemuExecutor {
>     fn run_until<F: Future>(future: F) -> F::Output;
> }
> 
> pub struct Executor;
> pub struct CoExecutor;
> 
> and pass an executor to Rust functions (&Executor for no_coroutine_fn,
> &CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed).  Or would that
> be premature in your opinion?

If we could get bindgen to actually do that for the C interface, then
this could be interesting, though for most functions it also would
remain unused boilerplate. If we have to get the executor manually on
the Rust side for each function, that's probably the same function that
will want to execute the future - in which case it just can directly
call the correct function.

The long term idea should be anyway that C coroutines disappear from the
path and we integrate an executor into the QEMU main loop. But as long
as the block layer core is written in C and uses coroutines and we want
Rust futures to be able to call into coroutine_fns, that's still a long
way to go.

Kevin