Skip to content
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.

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

[WIP] Add packed descriptor and MockPackedQueue #310

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

uran0sH
Copy link
Contributor

@uran0sH uran0sH commented Sep 25, 2024

Summary of the PR

Please summarize here why the changes in this PR are needed.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@uran0sH uran0sH changed the title Add packed descriptor and MockPackedQueue [WIP] Add packed descriptor and MockPackedQueue Sep 25, 2024
@uran0sH uran0sH marked this pull request as draft September 25, 2024 16:17
Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

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

Thanks for working on that!

I left some small comments, but I'd suggest splitting in multiple commits, for example:

  1. Add SplitDescriptor
  2. Use SplitDescriptor
  3. remove old stuff in virtio-queue/src/descriptor.rs
  4. Add PackedDescriptor
  5. 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.

pub mod packed;
pub mod split;

/// A virtio descriptor constraints with C representation.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a bit more detail here?

At first glance, it's not clear what it's for.

@@ -0,0 +1,46 @@
//! Descriptor types for virtio queue.
Copy link
Member

Choose a reason for hiding this comment

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

Please update also this

Comment on lines +20 to +61
/// A virtio descriptor constraints with C representation.
///
/// # Example
///
/// ```rust
/// # use virtio_bindings::bindings::virtio_ring::{VRING_DESC_F_NEXT, VRING_DESC_F_WRITE};
/// # use virtio_queue::mock::MockSplitQueue;
/// use virtio_queue::{Descriptor, Queue, QueueOwnedT};
/// use vm_memory::{GuestAddress, GuestMemoryMmap};
///
/// # fn populate_queue(m: &GuestMemoryMmap) -> Queue {
/// # let vq = MockSplitQueue::new(m, 16);
/// # let mut q = vq.create_queue().unwrap();
/// #
/// # // We have only one chain: (0, 1).
/// # let desc = Descriptor::new(0x1000, 0x1000, VRING_DESC_F_NEXT as u16, 1);
/// # vq.desc_table().store(0, desc);
/// # let desc = Descriptor::new(0x2000, 0x1000, VRING_DESC_F_WRITE as u16, 0);
/// # vq.desc_table().store(1, desc);
/// #
/// # vq.avail().ring().ref_at(0).unwrap().store(u16::to_le(0));
/// # vq.avail().idx().store(u16::to_le(1));
/// # q
/// # }
/// let m = &GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap();
/// // Populate the queue with descriptor chains and update the available ring accordingly.
/// let mut queue = populate_queue(m);
/// let mut i = queue.iter(m).unwrap();
/// let mut c = i.next().unwrap();
///
/// // Get the first descriptor and access its fields.
/// let desc = c.next().unwrap();
/// let _addr = desc.addr();
/// let _len = desc.len();
/// let _flags = desc.flags();
/// let _next = desc.next();
/// let _is_write_only = desc.is_write_only();
/// let _has_next = desc.has_next();
/// let _refers_to_ind_table = desc.refers_to_indirect_table();
/// ```
// Note that the `ByteValued` implementation of this structure expects the `Descriptor` to store
// only plain old data types.
Copy link
Member

Choose a reason for hiding this comment

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

Can you check this, talking about split virtqueue and also checking the example?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants