hermit-os / hermit-os/kernel

Code duplication between VIRTIO drivers

Open
#1,691 0 comments 0 reactions 1 assignee Claimed by @mkroening View on GitHub
Dominant language
Rust
Stars
1.5k
Forks
132
Avg merge
1d 14h
Merged PRs (30d)
39

Description

Basic functionality absolutely required for VIRTIO driver initialisation has been duplicated between VIRTIO net, fs, and vsock drivers.

Concrete examples of this are
* the `negotiate_features` helper function that given a fixed set of wanted features, checks feature set spec conformance (or is likely supposed to do so, see #1690) and if the device supports a superset, applies these features.
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/vsock/mod.rs#L297-L321
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/fs/virtio_fs.rs#L62-L85
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/net/virtio/mod.rs#L628-L652
* the extraction/mapping of PCI device configs from PCI capabilities and conversion of these to their respective Rust structs
* here the code snippets are not exactly identical, but functionally the same, with only the resulting driver type and device config type differing between instances of this code
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/vsock/pci.rs#L20-L97
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/fs/virtio_pci.rs#L13-L84
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/net/virtio/pci.rs#L21-L123

It should be possible and desirable to factor out this common code, given that errors (albeit only doc/logging errors) have already snuck in:
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/fs/virtio_fs.rs#L217
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/fs/virtio_pci.rs#L62

For the config mapping from PCI caps task, a shared
```rust
pub struct VirtioDriverConfig {
dev_cfg: DeviceConfig,
com_cfg: ComCfg,
notif_cfg: NotiCfg,
isr_status: IsrStatus,
irq: u8,
}
```
that implements this mapping generically might work and could then be used as a field to the drivers instead of the drivers having all of the above fields each.

For the `negotiate_features` function perhaps a function generic over the feature type might work, with a new error type that can be used by the drivers in their error types, instead of duplicating variants.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.