fix: Set the member variables of the virtio queue to volatile. #127
Conversation
src/queue.rs
Outdated
| avail_idx: u16, | ||
| last_used_idx: u16, | ||
| avail_idx: Volatile<u16>, | ||
| last_used_idx: Volatile<u16>, |
There was a problem hiding this comment.
These fields are just internal state, not MMIO or even shared memory. It doesn't make sense for them to be volatile. I think the fact that this fixes the bug you're seeing must be a side-effect of the increased synchronisation, and the real issue is probably a missing fence somewhere.
There was a problem hiding this comment.
and the real issue is probably a missing fence somewhere.
I've tried to add fence(Ordering::SeqCst) in can_pop(), but cannot solve the problem. After I set the last_used_idx and used.idx to Volatile, the problem solved.
There was a problem hiding this comment.
Oh, I realized the problem. We should set all fields in usedring and avail ring Volatile, but not the VirtQueue. I'll fix this.
| #[inline(always)] | ||
| pub fn write_volatile(&mut self, value: T) { | ||
| unsafe { self.self_ptr().as_ptr().write_volatile(value) } | ||
| } |
There was a problem hiding this comment.
These methods don't make sense. If a memory location must be accessed through volatile reads or writes, e.g. because it is MMIO, then it's not sound to hold a reference to it, because the Rust memory model allows the compiler to dereference references whenever it wants. So the only case where these methods can be called is when Volatile is being used incorrectly. The intention of this type is that it is only ever referred to via a pointer, and read and written via the VolatileReadable and VolatileWritable traits, possibly via the volread! and volwrite! macros.
There was a problem hiding this comment.
If I do not implement this method, then when changing the volatile variables of itself in the used ring and avail ring, I would also need to implement a method similar to self_ref(). However, that would lead to rather redundant code, so that's why I wrote it like this.
…lve the issue of the virtioblk test case hanging under RISC-V.
fslongjin
force-pushed
the
patch-set-virtio-queue-members-to-volatile
branch
from
c0bf4a0 to
e04e9a8
Compare
|
Merged #125 instead. |
fix: Set the member variables of the virtio queue to volatile to resolve the issue of the virtioblk test case hanging under RISC-V.
This PR fixed issue: #125