Add Packed Descriptor #310
Conversation
uran0sH
requested review from
alexandruag,
andreeaflorescu,
jiangliu,
slp,
stsquad,
epilys and
stefano-garzarella
as code owners
uran0sH
changed the title
There was a problem hiding this comment.
Thanks for working on that!
I left some small comments, but I'd suggest splitting in multiple commits, for example:
- Add SplitDescriptor
- Use SplitDescriptor
- remove old stuff in
virtio-queue/src/descriptor.rs - Add PackedDescriptor
- etc
BTW I noticed that virtio-queue/src/descriptor.rs is intact, should we remove Descriptor from there?
Please also add a nice description in commits and PR to explain what we are changing/adding.
uran0sH
changed the title
Can you check this? ^ Also the CI seems not happy, can you check that? Thanks :-) |
uran0sH
force-pushed
the
mock-dev
branch
9 times, most recently
from
834766b to
4281589
Compare
|
There was a problem hiding this comment.
I have checked them. I don't know whether the commit message is clear. Please feel free to review it again. Thanks :-)
They look better, just some little notes:
Patch 1: I'd replace crate with module
Patch 2: You can remove "We", just start with "Use ..."
Patch 4: Add a little description with the stuff Added/Changed
About the changes visible to the user, do you have some examples on how user should update their crates. For example, will be nice to open a Draft PR in https://github.com/rust-vmm/vhost and https://github.com/rust-vmm/vhost-device with related changes.
uran0sH
force-pushed
the
mock-dev
branch
2 times, most recently
from
25d251c to
4bf3e75
Compare
A draft commit for vhost: uran0sH/vhost@54a8d90 |
stefano-garzarella
force-pushed
the
mock-dev
branch
from
e6aca7d to
456a767
Compare
Cool, so very minimal changes! Would be nice to open a draft PR for vhost-device fixing all the devices after we merge this PR. |
|
@uran0sH there are some conflicts, can you rebase? |
done |
|
@epilys ping |
Add a new module called desc for split and packed descriptor. split.rs represents the split descriptor and packed.rs will represent the packed descriptor. The Descriptor struct in mod.rs represents the memory layout of the split and packed descriptor. Signed-off-by: Wenyu Huang <[email protected]>
Use 'virtio_queue::desc::split::Descriptor' to replace 'virtio_queue::Descriptor'. Signed-off-by: Wenyu Huang <[email protected]>
Add the packed descriptor for support packed vring. Signed-off-by: Wenyu Huang <[email protected]>
Signed-off-by: Wenyu Huang <[email protected]>
|
@epilys thanks for the review! |
@stefano-garzarella I have opened a draft pr for vhost-device(rust-vmm/vhost-device#834). But this pr only replaces the API for two types of devices. So, do we need to replace it for all devices? Or do you have a better suggestion? |
I think will be great to update it in all devices. Do you see any issue doing that? |
Summary of the PR
This PR is primarily aimed at preparing for the support of packed vring.
The struct Descriptor in desc/mod.rs is a unified memory layout representation.
The struct Descriptor in desc/split.rs represents the split descriptor.
The struct Descriptor in desc/packed.rs represents the packed descriptor.
And use the desc/split.rs and remove old stuff in descriptor.rs.
issue: #241
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.