Skip to content

Commit

Permalink
queue/tests: Fix endianess of memory operations
Browse files Browse the repository at this point in the history 
In the tests, the driver behavior is emulated by writting directly
into the memory. According to the specification, the driver must write
virtqueue-related data in little-endian, even if it's running on a
big-endian machine.

Fix those accesses by using u16::to_le() and u16::from_le() where
needed. These operations become no-ops on little-endian machines.

With this change, the unit tests pass on big-endian machines (tested
on s390x).

Signed-off-by: Sergio Lopez <[email protected]>
  • Loading branch information
@slp
slp committed Nov 26, 2021
1 parent 990777c commit bbc0f52
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 36 deletions.
14 changes: 7 additions & 7 deletions crates/virtio-queue/src/iterator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@ mod tests {
vq.desc_table().store(j, desc);
}

vq.avail().ring().ref_at(0).store(0);
vq.avail().ring().ref_at(1).store(2);
vq.avail().ring().ref_at(2).store(5);
vq.avail().idx().store(3);
vq.avail().ring().ref_at(0).store(u16::to_le(0));
vq.avail().ring().ref_at(1).store(u16::to_le(2));
vq.avail().ring().ref_at(2).store(u16::to_le(5));
vq.avail().idx().store(u16::to_le(3));

let mut i = q.iter().unwrap();

Expand Down Expand Up @@ -206,9 +206,9 @@ mod tests {
vq.desc_table().store(j, desc);
}

vq.avail().ring().ref_at(0).store(0);
vq.avail().ring().ref_at(1).store(2);
vq.avail().idx().store(2);
vq.avail().ring().ref_at(0).store(u16::to_le(0));
vq.avail().ring().ref_at(1).store(u16::to_le(2));
vq.avail().idx().store(u16::to_le(2));

let mut i = q.iter().unwrap();

Expand Down
53 changes: 29 additions & 24 deletions crates/virtio-queue/src/queue.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,16 +255,16 @@ mod tests {
let vq = MockSplitQueue::new(m, 16);
let mut q = vq.create_queue(m);

assert_eq!(vq.used().idx().load(), 0);
assert_eq!(u16::from_le(vq.used().idx().load()), 0);

// index too large
assert!(q.add_used(16, 0x1000).is_err());
assert_eq!(vq.used().idx().load(), 0);
assert_eq!(u16::from_le(vq.used().idx().load()), 0);

// should be ok
q.add_used(1, 0x1000).unwrap();
assert_eq!(q.state.next_used, Wrapping(1));
assert_eq!(vq.used().idx().load(), 1);
assert_eq!(u16::from_le(vq.used().idx().load()), 1);

let x = vq.used().ring().ref_at(0).load();
assert_eq!(x.id(), 1);
Expand Down Expand Up @@ -323,8 +323,11 @@ mod tests {
assert_eq!(q.needs_notification().unwrap(), true);
}

m.write_obj::<u16>(4, avail_addr.unchecked_add(4 + qsize as u64 * 2))
.unwrap();
m.write_obj::<u16>(
u16::to_le(4),
avail_addr.unchecked_add(4 + qsize as u64 * 2),
)
.unwrap();
q.state.set_event_idx(true);

// Incrementing up to this value causes an `u16` to wrap back to 0.
Expand Down Expand Up @@ -372,26 +375,28 @@ mod tests {
assert_eq!(q.state.event_idx_enabled, false);

q.enable_notification().unwrap();
let v = m.read_obj::<u16>(used_addr).unwrap();
let v = m.read_obj::<u16>(used_addr).map(u16::from_le).unwrap();
assert_eq!(v, 0);

q.disable_notification().unwrap();
let v = m.read_obj::<u16>(used_addr).unwrap();
let v = m.read_obj::<u16>(used_addr).map(u16::from_le).unwrap();
assert_eq!(v, VIRTQ_USED_F_NO_NOTIFY);

q.enable_notification().unwrap();
let v = m.read_obj::<u16>(used_addr).unwrap();
let v = m.read_obj::<u16>(used_addr).map(u16::from_le).unwrap();
assert_eq!(v, 0);

q.set_event_idx(true);
let avail_addr = vq.avail_addr();
m.write_obj::<u16>(2, avail_addr.unchecked_add(2)).unwrap();
m.write_obj::<u16>(u16::to_le(2), avail_addr.unchecked_add(2))
.unwrap();

assert_eq!(q.enable_notification().unwrap(), true);
q.state.next_avail = Wrapping(2);
assert_eq!(q.enable_notification().unwrap(), false);

m.write_obj::<u16>(8, avail_addr.unchecked_add(2)).unwrap();
m.write_obj::<u16>(u16::to_le(8), avail_addr.unchecked_add(2))
.unwrap();

assert_eq!(q.enable_notification().unwrap(), true);
q.state.next_avail = Wrapping(8);
Expand Down Expand Up @@ -419,13 +424,13 @@ mod tests {
vq.desc_table().store(i, desc);
}

vq.avail().ring().ref_at(0).store(0);
vq.avail().ring().ref_at(1).store(2);
vq.avail().ring().ref_at(2).store(5);
vq.avail().ring().ref_at(3).store(7);
vq.avail().ring().ref_at(4).store(9);
vq.avail().ring().ref_at(0).store(u16::to_le(0));
vq.avail().ring().ref_at(1).store(u16::to_le(2));
vq.avail().ring().ref_at(2).store(u16::to_le(5));
vq.avail().ring().ref_at(3).store(u16::to_le(7));
vq.avail().ring().ref_at(4).store(u16::to_le(9));
// Let the device know it can consume chains with the index < 2.
vq.avail().idx().store(2);
vq.avail().idx().store(u16::to_le(2));
// No descriptor chains are consumed at this point.
assert_eq!(q.next_avail(), 0);

Expand All @@ -450,7 +455,7 @@ mod tests {
// The next chain that can be consumed should have index 2.
assert_eq!(q.next_avail(), 2);
// Let the device know it can consume one more chain.
vq.avail().idx().store(3);
vq.avail().idx().store(u16::to_le(3));
i = 0;

loop {
Expand All @@ -465,7 +470,7 @@ mod tests {
// ring. Ideally this should be done on a separate thread.
// Because of this update, the loop should be iterated again to consume the new
// available descriptor chains.
vq.avail().idx().store(4);
vq.avail().idx().store(u16::to_le(4));
if !q.enable_notification().unwrap() {
break;
}
Expand All @@ -476,7 +481,7 @@ mod tests {

// Set an `idx` that is bigger than the number of entries added in the ring.
// This is an allowed scenario, but the indexes of the chain will have unexpected values.
vq.avail().idx().store(7);
vq.avail().idx().store(u16::to_le(7));
loop {
q.disable_notification().unwrap();

Expand Down Expand Up @@ -514,11 +519,11 @@ mod tests {
vq.desc_table().store(i, desc);
}

vq.avail().ring().ref_at(0).store(0);
vq.avail().ring().ref_at(1).store(2);
vq.avail().ring().ref_at(2).store(5);
vq.avail().ring().ref_at(0).store(u16::to_le(0));
vq.avail().ring().ref_at(1).store(u16::to_le(2));
vq.avail().ring().ref_at(2).store(u16::to_le(5));
// Let the device know it can consume chains with the index < 2.
vq.avail().idx().store(3);
vq.avail().idx().store(u16::to_le(3));
// No descriptor chains are consumed at this point.
assert_eq!(q.next_avail(), 0);

Expand All @@ -541,7 +546,7 @@ mod tests {

// Decrement `idx` which should be forbidden. We don't enforce this thing, but we should
// test that we don't panic in case the driver decrements it.
vq.avail().idx().store(1);
vq.avail().idx().store(u16::to_le(1));

loop {
q.disable_notification().unwrap();
Expand Down
12 changes: 7 additions & 5 deletions crates/virtio-queue/src/state_sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,26 +263,28 @@ mod tests {

assert_eq!(q.lock_state().event_idx_enabled, false);
q.enable_notification(mem).unwrap();
let v = m.read_obj::<u16>(used_addr).unwrap();
let v = m.read_obj::<u16>(used_addr).map(u16::from_le).unwrap();
assert_eq!(v, 0);

q.disable_notification(m.memory()).unwrap();
let v = m.read_obj::<u16>(used_addr).unwrap();
let v = m.read_obj::<u16>(used_addr).map(u16::from_le).unwrap();
assert_eq!(v, VIRTQ_USED_F_NO_NOTIFY);

q.enable_notification(mem).unwrap();
let v = m.read_obj::<u16>(used_addr).unwrap();
let v = m.read_obj::<u16>(used_addr).map(u16::from_le).unwrap();
assert_eq!(v, 0);

q.set_event_idx(true);
let avail_addr = q.lock_state().avail_ring;
m.write_obj::<u16>(2, avail_addr.unchecked_add(2)).unwrap();
m.write_obj::<u16>(u16::to_le(2), avail_addr.unchecked_add(2))
.unwrap();

assert_eq!(q.enable_notification(mem).unwrap(), true);
q.lock_state().next_avail = Wrapping(2);
assert_eq!(q.enable_notification(mem).unwrap(), false);

m.write_obj::<u16>(8, avail_addr.unchecked_add(2)).unwrap();
m.write_obj::<u16>(u16::to_le(8), avail_addr.unchecked_add(2))
.unwrap();

assert_eq!(q.enable_notification(mem).unwrap(), true);
q.lock_state().next_avail = Wrapping(8);
Expand Down

0 comments on commit bbc0f52

Please sign in to comment.