VIRTIO feature negotiation docs/code mismatch
- Dominant language
- Rust
- Stars
- 1.5k
- Forks
- 132
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 39
Description
The comments/doc comments/info messages describing feature negotiation with a VIRTIO device don't seem to quite match the logic of the code.
The doc comment of the `negotiate_features` function could be more precise in that the function attempts to obtain _all_ features the driver wants, discarding features the device supports if necessary, but never the other way around. The latter is taken care of in the `init_dev` function for the VIRTIO network driver:
https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/net/virtio/mod.rs#L513-L580
For the vsock and VIRTIO filesystem drivers, no features are supported (beyond v1 spec compliance) so this part of the negotiation is left out. The comments for the `negotiate_features` function however are left unaltered, as they are in the VIRTIO network driver.
Within the context of the VIRTIO network driver implementation the comment
```rust
// If device supports subset of features write feature set to common config
```
makes sense, however it might suggest a flipped situation, where in `negotiate_features` _driver_ features are discarded to match the subset of features supported by the device, where the logic in this part of the code is the other way around. If the device supports a _superset_ of the (for the current call of the function) fixed driver features set, then we apply the driver features.
Unrelated to this, the info level log message talks of the feature set wanted by the _driver_ being conformant to the requirements posed by the spec, when the only thing being tested in this function is the conformance of the full feature set presented by the _device_. Our wanted subset for the driver might not actually be conformant, given that we don't check this.
The relevant `negotiate_features` code snippets:
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
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.