Moves specification-required checks to setters #112
Conversation
crates/virtio-device/src/mmio.rs
Outdated
| { | ||
| if device.check_device_status(status::FEATURES_OK, status::DRIVER_OK | status::FAILED) { | ||
| if let Some(queue) = device.selected_queue_mut() { | ||
| f(queue); | ||
| if let Err(e) = f(queue) { | ||
| device.set_device_status(status::FAILED); |
There was a problem hiding this comment.
From the experiments that I did last week, setting the device status to FAILED doesn't seem to actually make a difference. Sorry for not coming with a response on the issue, but I had a chat only today with @andreeaflorescu and @alexandruag to decide on the path forward here, and we were thinking to keep the setter proposal, but in case of a misconfiguration to set the value of that field to a default. This seems to be how qemu handles this issue as well. So for the queue size, we would keep the max size if the value is invalid, and for the descriptor table address and the others, we would use address 0. What do you think?
I think with this approach, we could get rid of the actual_size() method as well, but I'll double-check.
crates/virtio-queue/src/lib.rs
Outdated
| fn actual_size(&self) -> u16 { | ||
| min(self.size, self.max_size) | ||
| } | ||
| fn set_size(&mut self, size: u16) -> Result<(), Error> { |
There was a problem hiding this comment.
Sorry for not being explicit enough in my previous comment :(. The way we were thinking to address the issue is to not return an error anymore in case of a misconfiguration, and just ignore the value and log an error, something like:
fn set_size(&mut self, size: u16) {
if size > self.max_size() || size == 0 || (size & (size - 1)) != 0 {
error!("virtio queue with invalid size: {}", size);
return;
}
self.size = size;
}It's not necessary to explicitly set the value to the default one, because the Queue constructor is already initializing the size and addresses to the default ones. I would though define constants for the default addresses like you already did here, but one for each queue part:
const DEFAULT_DESC_TABLE_ADDR: u64 = 0x0;
const DEFAULT_AVAIL_RING_ADDR: u64 = 0x0;
...
With this approach, we won't need to define other errors, and the logic here won't be needed anymore.
We should also update the negative tests for these setters, i.e. try to set an invalid value, and check that the value of size/addr remains the default one.
Moves the mandatory checks, according to the virtio specification, from Queue::is_valid() to the corresponding Queue setters. closes: rust-vmm#94 Signed-off-by: German Maglione <[email protected]>
Remove actual_size() since Queue::set_size() checks that the size cannot be greater than max_size. Signed-off-by: German Maglione <[email protected]>
| @@ -251,8 +251,9 @@ pub(crate) mod tests { | |||
|
|
|||
| impl Dummy { | |||
| pub fn new(device_type: u32, features: u64, config_space: Vec<u8>) -> Self { | |||
| let mem = | |||
| Arc::new(GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x1000_0000)]).unwrap()); | |||
| let mem = Arc::new( | |||
No problem, I'm still learning, so thanks for your patience :) |
Moves specification-required checks to setters
Moves the mandatory checks, according to the virtio specification,
from Queue::is_valid() to the corresponding Queue setters.
closes: #94
Signed-off-by: German Maglione [email protected]