Closed roypat closed 10 months ago
Could you show how this would work with vm-virtio with a draft PR there? How would you handle the flush in vm-console
? By requiring Write + WriteVolatile?
Could you show how this would work with vm-virtio with a draft PR there? How would you handle the flush in
vm-console
? By requiring Write + WriteVolatile?
Ah sorry, I thought you mentioned in your other comment that you don't need flush
after all. The Write + WriteVolatile
approach should work, I think the diff for that should be
diff --git a/crates/devices/virtio-console/src/console.rs b/crates/devices/virtio-console/src/console.rs
index c06cadd..f0324a4 100644
--- a/crates/devices/virtio-console/src/console.rs
+++ b/crates/devices/virtio-console/src/console.rs
@@ -81,7 +81,7 @@ impl Display for Error {
/// Console struct that implements the abstraction of virtio console descriptor chain handling.
#[derive(Clone, Debug)]
-pub struct Console<T: Write> {
+pub struct Console<T: Write + WriteVolatile> {
/// Buffer that stores data to be sent to the driver.
input_buffer: Arc<Mutex<Vec<u8>>>,
/// Capacity of the input buffer.
@@ -110,7 +110,7 @@ impl Default for Console<Stdout> {
impl<T> Console<T>
where
- T: Write,
+ T: Write + WriteVolatile,
{
/// Create new console object with the default `capacity`.
///
@@ -206,7 +206,7 @@ where
}
desc_chain
.memory()
- .write_to(desc.addr(), &mut self.output, desc.len() as usize)
+ .write_volatile_to(desc.addr(), &mut self.output, desc.len() as usize)
.map_err(Error::WriteToOutputFailed)?;
self.output.flush().map_err(Error::OutputSinkFlushFailed)?;
I briefly looked at adding flush
to WriteVolatile
, but wasn't really sure how to provide sensible implementations for things like OwnedFd
and BorrowedFd
Yeah, if that works, then we do not need flush :).
Hm. WriteVolatile for Vec<u8>
also may be interesting? vm-virtio seems to use that in tests.
I ran a quick test and:
impl WriteVolatile for Vec<u8> {
fn write_volatile<B: BitmapSlice>(
&mut self,
buf: &VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
let len = buf.len();
self.reserve(len);
let data = self.as_mut_ptr();
// SAFETY: reserving above guarantees that we have at least buf.len() extra space
// after self.len(), we construct a slice for that region and fill it entirely.
// Then we call set_len() after all new elements are initialized.
unsafe {
let slice = &mut *slice_from_raw_parts_mut(data.add(self.len()), len);
buf.copy_to(slice);
self.set_len(self.len() + len);
}
Ok(len)
}
}
and specifying Write + WriteVolatile
is all that is needed to fix vm-virtio.
Hm.
WriteVolatile for Vec<u8>
also may be interesting? vm-virtio seems to use that in tests.I ran a quick test and:
impl WriteVolatile for Vec<u8> { fn write_volatile<B: BitmapSlice>( &mut self, buf: &VolatileSlice<B>, ) -> Result<usize, VolatileMemoryError> { let len = buf.len(); self.reserve(len); let data = self.as_mut_ptr(); // SAFETY: reserving above guarantees that we have at least buf.len() extra space // after self.len(), we construct a slice for that region and fill it entirely. // Then we call set_len() after all new elements are initialized. unsafe { let slice = &mut *slice_from_raw_parts_mut(data.add(self.len()), len); buf.copy_to(slice); self.set_len(self.len() + len); } Ok(len) } }
and specifying
Write + WriteVolatile
is all that is needed to fix vm-virtio.
Ah, yeah, that would make sense. I think when I made my RFC I updated all the Vec-based tests to just pre-allocated correctly sized Vecs and use the slice impls, but I don't really remember why I did that. Your impl looks good to me, I'll add it to this PR and give you Co-Authored-By credit if you're okay with that?
Ah, yeah, that would make sense. I think when I made my RFC I updated all the Vec-based tests to just pre-allocated correctly sized Vecs and use the slice impls, but I don't really remember why I did that. Your impl looks good to me, I'll add it to this PR and give you Co-Authored-By credit if you're okay with that?
Fine with me. But if there is your other change is also simple enough - and avoids needing this impl - then going that route also sounds fine with me.
Ah, yeah, that would make sense. I think when I made my RFC I updated all the Vec-based tests to just pre-allocated correctly sized Vecs and use the slice impls, but I don't really remember why I did that. Your impl looks good to me, I'll add it to this PR and give you Co-Authored-By credit if you're okay with that?
Fine with me. But if there is your other change is also simple enough - and avoids needing this impl - then going that route also sounds fine with me.
Let's just add it anyway, it makes writing test code (and the upgrade) easier after all :)
@roypat: Looks like you CI complains about a small whitespace issue there :)
@roypat: Looks like you CI complains about a small whitespace issue there :)
ooops, fixed, thanks!
Summary of the PR
When upgrading other rust-vmm crates to use the new traits introduced in https://github.com/rust-vmm/vm-memory/pull/247, it was noticed that we a missing a few
ReadVolatile
andWriteVolatile
implementations. This PR tries to supply these.Requirements
Before submitting your PR, please make sure you addressed the following requirements:
git commit -s
), and the commit message has max 60 characters for the summary and max 75 characters for each description line.unsafe
code is properly documented.