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

Add support for indirect descriptors in VirtQueue with alloc feature #102

Merged
merged 5 commits into from
Jul 24, 2023

Conversation

qwandor
Copy link
Collaborator

@qwandor qwandor commented Jul 13, 2023

This is based on some of the work in #96, but fixes some issues there with how allocation and DMA memory is handled, adds tests for the indirect virtqueue itself, and also adds support to the fake device support so that device tests will work with indirect queues.

Devices can choose when constructing a VirtQueue whether it should support indirect descriptors or not, based on their preference and the device features. For now only the VirtIOBlk driver uses indirect descriptors (if the underlying device supports it). If a VirtQueue is constructed with indirect descriptor support then it will use them whenever more than one buffer is added to the queue. There's no point using indirect descriptors for a single buffer so we don't bother.

Space for the indirect descriptor lists is allocated (and shared with the device via the HAL) dynamically. This means that it depends on the alloc feature, which is enabled by default. If the alloc feature is disabled, then indirect descriptors will never be used.

@qwandor qwandor mentioned this pull request Jul 13, 2023
@qwandor qwandor force-pushed the indirect branch 2 times, most recently from 4d8a220 to ada5eb0 Compare July 21, 2023 12:36
Copy link
Member

@equation314 equation314 left a comment

Choose a reason for hiding this comment

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

LGTM

@qwandor qwandor merged commit 7cbbef1 into master Jul 24, 2023
@qwandor qwandor deleted the indirect branch July 24, 2023 14:05
Copy link
Member

@equation314 equation314 left a comment

Choose a reason for hiding this comment

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

Maybe we should add a new feature "indirect-desc"?

@qwandor
Copy link
Collaborator Author

qwandor commented Jul 24, 2023

Maybe we should add a new feature "indirect-desc"?

You mean a cargo feature? Is there any case where you have an allocator available, the VirtIO device implementation supports indirect descriptors, but you wouldn't want to use them? I can't think of one, but if so we could add a flag to the device driver constructor so it could be decided at runtime.

@equation314
Copy link
Member

The conditional compilation attribute #[cfg(feature = "alloc")] is confused, I think use #[cfg(feature = "indirect-desc")] is more better.

@qwandor
Copy link
Collaborator Author

qwandor commented Jul 24, 2023

The reason that indirect descriptor support is guarded by the alloc feature is that the way it's implemented here it requires heap allocation, i.e. a global allocator. In particular, it uses alloc::boxed::Box to allocate the array of indirect descriptors on the heap. If someone wants to use virtio-drivers in an environment where they don't have a heap allocator, then they will lose indirect descriptor support, along with some of the drivers that use heap allocation (currently console, GPU, input and net).

In an environment where heap allocation is available, I don't see any reason why someone would want to disable just indirect descriptor support at compile-time.

@equation314
Copy link
Member

Okay, I mean we can enable it by default and make it dependent on alloc

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