Skip to content

Fix support for big-endian architectures #118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andreeaflorescu merged 4 commits into from
Dec 3, 2021
Merged

Fix support for big-endian architectures #118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3827020
queue: Ensure the use of Descriptor accessors
slp Nov 26, 2021
990777c
queue: Add support for big-endian arches
slp Nov 26, 2021
bbc0f52
queue/tests: Fix endianess of memory operations
slp Nov 26, 2021
9cd2cdb
Merge branch 'main' into fix-endianess
slp Nov 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions crates/virtio-queue/src/iterator.rs
View file
Open in desktop
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));
Copy link
Member

@andreeaflorescu andreeaflorescu Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead update the store and load from the mock interface to write and read little endian values so we don't need to use the conversion in all tests?

Copy link
Collaborator Author

@slp slp Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, the problem is that we don't always want to do the endianess translation. For instance, we don't want to do it for Descriptor, as it's already in the right endianess. Adding a new dedicated method doesn't solve the problem, as the dev would still need if the conversion needs to be done. I think the only "right" solution would be revamping the mock objects to avoid naked loads/stores from/to memory, having dedicated methods for each field with the right types, but that seems like too huge of a change to me.

Being pragmatic, as this only affects tests and it's probably going to be very hard to integrate a big-endian machine into the CI, I think it's fine if they get broken from time to time. I'll give them a try on each new virtiofsd-rs release and fix them if needed.

Copy link
Member

@andreeaflorescu andreeaflorescu Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. We want to do some more improvements to the mock interface, do you mind opening an issue with this suggestion that you just mentioned?

Copy link
Collaborator Author

@slp slp Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've created #123 to track this. Thanks!

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
Open in desktop
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
Open in desktop
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