Re: [PATCH v2 2/2] rust: virtio: add virtio support
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
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
[PATCH v2 1/2] rust: add scatterlist support
This patch adds a scatterlist abstraction. It is then used and tested
by the new rust virtio sample module.
Signed-off-by: Daniel Almeida
---
rust/kernel/lib.rs | 1 +
rust/kernel/scatterlist.rs | 40 ++
2 files changed, 41 insertions(+)
create mode 100644 rust/kernel/scatterlist.rs
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c20b37e88ab2..b23be69919cc 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -71,6 +71,7 @@ pub mod net;
pub mod pages;
pub mod power;
pub mod revocable;
+pub mod scatterlist;
pub mod security;
pub mod task;
pub mod workqueue;
diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
new file mode 100644
index ..fe036af13995
--- /dev/null
+++ b/rust/kernel/scatterlist.rs
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Scatterlist abstractions.
+//!
+//! C header:
[`include/linux/virtio.h`](../../../../include/linux/scatterlist.h)
+
+use core::cell::UnsafeCell;
+
+/// Scatterlist abstraction over the C side `struct scatterlist`.
+pub struct Scatterlist {
+/// The C side `struct scatterlist`.
+inner: UnsafeCell,
+}
+
+impl Scatterlist {
+/// A wrapper over the C-side `sg_init_one()`. Initialize a single entry sg
+/// list.
+///
+/// # Safety
+///
+/// Caller must ensure that `buf` is valid and `buflen` really
+/// represents the size of `buf`.
+pub unsafe fn init_one(buf: *const core::ffi::c_void, buflen: u32) -> Self
{
+// SAFETY: There are no references or function pointers in this
+// `Scatterlist`.
+let mut sg: Scatterlist = unsafe { core::mem::zeroed() };
+// SAFETY: we pass a valid sg entry, which points to stack-allocated
+// variable above. `buf` and `buflen` are presumed valid as per this
+// function's safety requirements.
+unsafe {
+bindings::sg_init_one(sg.inner.get_mut(), buf, buflen);
+}
+
+sg
+}
+
+pub(crate) fn inner(&self) -> &UnsafeCell {
+&self.inner
+}
+}
--
2.40.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 0/2] rust: virtio: add virtio support
This used to be a single patch, but I split it into two with the addition of struct Scatterlist. Again a bit new with Rust submissions. I was told by Gary Guo to rebase on top of rust-next, but it seems *very* behind? The first patch does not build on its own due to a dead_code warning. It is hard to not have dead code when one is adding infrastructure to be used by others at a later opportunity. Let me know if you would like to see the patches squashed into one to fix this. As suggested by Bjorn, I added a small virtio-entropy based sample. It is a very bare-bones clone of virtio-rng.c that only sends a single request. Changes from v1: - Addressed review comments by Miguel and Bjorn. - Added a scatterlist abstraction - Added a virtio-rng based sample Daniel Almeida (2): rust: add scatterlist support rust: virtio: add virtio support rust/bindings/bindings_helper.h | 3 + rust/helpers.c | 25 +++ rust/kernel/lib.rs | 3 + rust/kernel/scatterlist.rs | 40 + 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 9 files changed, 664 insertions(+) create mode 100644 rust/kernel/scatterlist.rs create mode 100644 rust/kernel/virtio.rs create mode 100644 rust/kernel/virtio/virtqueue.rs create mode 100644 samples/rust/rust_virtio.rs -- 2.40.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] rust: virtio: add virtio support
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
---
Ok so this is my first Rust contribution here. It's part of a virtIO
driver I was originally writing. Both the probing and the virtqueue
support in here were confirmed as working in said prototype driver, and
the pieces were picked separately into this patch.
Feel free to point me to the best practices around Rust patch
submission, as the C stuff like checkpatch etc probably does not apply
yet. I did take care to run clippy though.
---
rust/bindings/bindings_helper.h | 2 +
rust/helpers.c | 18 +++
rust/kernel/lib.rs | 2 +
rust/kernel/virtio.rs | 217
rust/kernel/virtio/virtqueue.rs | 110
samples/rust/Kconfig| 10 ++
samples/rust/Makefile | 1 +
samples/rust/rust_virtio.rs | 54
8 files changed, 414 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..e5bf6f4c3188 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -34,6 +34,8 @@
#include
#include
#include
+#include
+#include
#include
/* `bindgen` gets confused at certain things. */
diff --git a/rust/helpers.c b/rust/helpers.c
index bf790f46c763..28cd97228c14 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,22 @@ 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);
+
/*
* 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 c20b37e88ab2..324ff45f825d 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -73,6 +73,8 @@ pub mod power;
pub mod revocable;
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 ..57894af4f5ba
--- /dev/null
+++ b/rust/kernel/virtio.rs
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0
+#![allow(missing_docs)]
+
+///! 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;
+
+#[derive(Copy, Clone, Debug, PartialEq, PartialOrd)]
+pub struct DeviceId {
+pub device: u32,
+pub vendor: u32,
+}
+
+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,
+};
+
+// No `data` pointer.
+fn to_rawid(&self, _offset: isize) -> Self::RawType {
+bindings::virtio_device_id {
+device: self.device,
+vendor: self.vendor,
+}
+}
+}
+
+#[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.name = name.as_char_ptr();
+virtio_driver.probe = Some(Self::probe_callback);
+virtio_driver.remove = Some(Self::remove_callback);
+
