Fix support for big-endian architectures #118
Conversation
slp
force-pushed
the
fix-endianess
branch
2 times, most recently
from
2183f1d to
39b7896
Compare
|
Since there's no agreement about #111, I'm marking this one as ready to review to move things forward. |
slp
changed the title
|
Turning this PR back into a draft. I want to make use of vm_memory:{Le16, Le32, Le64}, and make sure that all tests that do assertions of |
Ensure that we're using the Descriptor accessors everywhere, instead of accessing its fields directly. This is needed for the next commit, where we'll be fixing the endianess of each field on its accessor. Signed-off-by: Sergio Lopez <[email protected]>
According to the virtio specification, on non-legacy devices the values in the virtqueue are represented in little-endian, no matter the host nor the guest actual endianess. If needed, fix the endianess of values that have been read from memory or will be written to it. The methods "from_le" and "to_le" are a no-op on little-endian machines, so this shouldn't have a performance impact on those. Fixes rust-vmm#117 Signed-off-by: Sergio Lopez <[email protected]>
slp
force-pushed
the
fix-endianess
branch
2 times, most recently
from
3479085 to
cfd7369
Compare
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]>
|
Rebased and tests working in big-endian, it's now ready to be reviewed. |
slp
changed the title
|
Has this patch been tested on any big-endian platform? |
Yes, on s390x. |
great, feel much more confident:) |
|
@jiangliu @alexandruag How would you feel about merging this one? We really need it for virtiofsd. |
andreeaflorescu
requested review from
andreeaflorescu
and removed request for
alexandruag
|
@slp I will take a look at this one today/tomorrow. |
| 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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure, I've created #123 to track this. Thanks!
This is an attempt at fixing support for big-endian architectures by:
DescriptorandVirtqUsedElemfields are stored with little-endian ordering.newmethods, and from little-endian on their accessors.The
::from_leand::to_lemethods are no-ops on little-endian machines, so their introduction shouldn't have any impact on those architectures.I'm marking this PR as a draft, as I don't think it makes sense to merge it until we've reached a final decision about the API change in #111