Re: [PATCH v2 2/2] rust: virtio: add virtio support

[Daniel Almeida via Virtualization]

Hi Wedson, Martin,

First of all, thanks for the review.


> > +    /// VirtIO driver remove.
> > +    ///
> > +    /// Called when a virtio device is removed.
> > +    /// Implementers should prepare the device for complete
> > removal here.
> > +    ///
> > +    /// In particular, implementers must ensure no buffers are
> > left over in the
> > +    /// virtqueues in use by calling [`virtqueue::get_buf()`]
> > until `None` is
> > +    /// returned.
> 
> What happens if implementers don't do this?
> 
> If this is a safety requirement, we need to find a different way to
> enforce it.
> 
> > 

This is the worst part of this patch by far, unfortunately. If one
doesn't do this, then s/he will leak the `data` field passed in through
into_foreign() here:



> +// SAFETY: `self.ptr` is valid as per the type invariant.
> +let res = unsafe {
> +bindings::virtqueue_add_sgs(
> +self.ptr,
> +sgs.as_mut_ptr(),
> +num_out as u32,
> +num_in as u32,
> +data.into_foreign() as _,
> +gfp,
> +)
> +};
> +

Note the comment here:


> +// SAFETY: if there is a buffer token, it came from
> +// `into_foreign()` as called in `add_sgs()`.
> +::from_foreign(buf)


To be honest, I tried coming up with something clever to solve this,
but couldn't. Ideally this should happen when this function is called:

> +extern "C" fn remove_callback(virtio_device: *mut
bindings::virtio_device) {


But I did not find a way to iterate over the the `vqs` member from the
Rust side, i.e.:

```

struct virtio_device {
int index;
bool failed;
bool config_enabled;
bool config_change_pending;
spinlock_t config_lock;
spinlock_t vqs_list_lock; /* Protects VQs list access */
struct device dev;
struct virtio_device_id id;
const struct virtio_config_ops *config;
const struct vringh_config_ops *vringh_config;
struct list_head vqs; <--
```

Is there any wrappers over list_for_each_entry(), etc, to be used from
Rust? If so, I could not find them.

Doing this cleanup from Virtqueue::Drop() is not an option either:
since we wrap over a pointer owned by the kernel, there's no guarantee
that the actual virtqueue is going away when drop is called on the
wrapper. In fact, this is never the case, as virtqueues are deleted
through this call:


> +void rust_helper_virtio_del_vqs(struct virtio_device *vdev)
> +{
> +   vdev->config->del_vqs(vdev);
> +}
> +



Suggestions welcome,

-- Daniel

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v2 2/2] rust: virtio: add virtio support

[Daniel Almeida via Virtualization]

This patch adds virtIO support to the rust crate. This includes the
capability to create a virtIO driver (through the module_virtio_driver
macro and the respective Driver trait) as well as initial virtqueue
support.

A sample virtIO module is included for conveninence.

Signed-off-by: Daniel Almeida 
---
 rust/bindings/bindings_helper.h |   3 +
 rust/helpers.c  |  25 +++
 rust/kernel/lib.rs  |   2 +
 rust/kernel/virtio.rs   | 261 
 rust/kernel/virtio/virtqueue.rs | 126 +++
 samples/rust/Kconfig|  10 ++
 samples/rust/Makefile   |   1 +
 samples/rust/rust_virtio.rs | 195 
 8 files changed, 623 insertions(+)
 create mode 100644 rust/kernel/virtio.rs
 create mode 100644 rust/kernel/virtio/virtqueue.rs
 create mode 100644 samples/rust/rust_virtio.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 048bae2197ac..001a492e0554 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -35,6 +35,9 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 /* `bindgen` gets confused at certain things. */
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
diff --git a/rust/helpers.c b/rust/helpers.c
index bf790f46c763..a80b0ab8ca64 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -39,6 +39,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -655,6 +657,29 @@ int rust_helper_fs_parse(struct fs_context *fc,
 }
 EXPORT_SYMBOL_GPL(rust_helper_fs_parse);
 
+struct virtqueue *rust_helper_virtio_find_single_vq(struct virtio_device *vdev,
+   vq_callback_t *c,
+   const char *n)
+{
+   return virtio_find_single_vq(vdev, c, n);
+}
+
+EXPORT_SYMBOL_GPL(rust_helper_virtio_find_single_vq);
+
+void rust_helper_virtio_del_vqs(struct virtio_device *vdev)
+{
+   vdev->config->del_vqs(vdev);
+}
+
+EXPORT_SYMBOL_GPL(rust_helper_virtio_del_vqs);
+
+void rust_helper_virtio_device_ready(struct virtio_device *vdev)
+{
+   virtio_device_ready(vdev);
+}
+
+EXPORT_SYMBOL_GPL(rust_helper_virtio_device_ready);
+
 /*
  * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
  * as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index b23be69919cc..5441b545c5f2 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -74,6 +74,8 @@ pub mod revocable;
 pub mod scatterlist;
 pub mod security;
 pub mod task;
+#[cfg(CONFIG_VIRTIO)]
+pub mod virtio;
 pub mod workqueue;
 
 pub mod linked_list;
diff --git a/rust/kernel/virtio.rs b/rust/kernel/virtio.rs
new file mode 100644
index ..a95a32605728
--- /dev/null
+++ b/rust/kernel/virtio.rs
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Virtio abstractions
+//!
+//! C header: [`include/linux/virtio.h`](../../../../include/media/virtio.h)
+
+use crate::{
+device, driver,
+error::{self, from_kernel_result},
+prelude::*,
+str::CStr,
+to_result, ForeignOwnable, ThisModule,
+};
+
+pub mod virtqueue;
+
+/// A VirtIO Device ID entry. Safe wrapper over `struct virtio_device_id`.
+#[derive(Copy, Clone, Debug, PartialEq, PartialOrd)]
+pub struct DeviceId {
+/// The device ID in the virtIO protocol.
+pub device: u32,
+/// The vendor ID, usually set to VIRTIO_DEV_ANY_ID.
+pub vendor: u32,
+}
+
+// SAFETY: we ensure that `RawDeviceId::ZERO` is actually a zeroed out version 
of
+// the raw device id. Offset is not needed, because there is no `data` pointer.
+unsafe impl const crate::driver::RawDeviceId for DeviceId {
+type RawType = bindings::virtio_device_id;
+
+const ZERO: Self::RawType = bindings::virtio_device_id {
+device: 0,
+vendor: 0,
+};
+
+fn to_rawid(&self, _offset: isize) -> Self::RawType {
+bindings::virtio_device_id {
+device: self.device,
+vendor: self.vendor,
+}
+}
+}
+
+/// Defines a VirtIO ID table consisting of [`DeviceId`] entries.
+#[macro_export]
+macro_rules! define_virtio_id_table {
+($data_type:ty, $($t:tt)*) => {
+$crate::define_id_table!(ID_TABLE, $crate::virtio::DeviceId, 
$data_type, $($t)*);
+};
+}
+
+/// A registration of a virtio driver.
+pub type Registration = driver::Registration>;
+
+/// An adapter for the registration of virtio drivers.
+pub struct Adapter(T);
+
+impl driver::DriverOps for Adapter {
+type RegType = bindings::virtio_driver;
+
+unsafe fn register(
+reg: *mut bindings::virtio_driver,
+name: &'static CStr,
+_module: &'static ThisModule,
+) -> Result {
+// SAFETY: By the safety requirements of this function `reg` is 
non-null
+// and valid.
+let virtio_driver = unsafe { &mut *reg };
+
+virtio_driver.driver