Fixes & Refactoring Galore #9
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
birkelund
force-pushed
the
iommufd
branch
5 times, most recently
from
6b0f701 to
94c27f6
Compare
birkelund
commented
Oct 30, 2023
birkelund
force-pushed
the
iommufd
branch
5 times, most recently
from
402168c to
2071e98
Compare
| { | ||
| struct stat sb; | ||
|
|
||
| if (stat("/dev/vfio/devices", &sb) || !S_ISDIR(sb.st_mode)) { |
There was a problem hiding this comment.
The "devices" directory is actually not present when all the devices are attached to a kernel driver even if you have CONFIG_VFIO_DEVICE_CDEV=y. You have to detach the device and then the "devices" directory will show up. This can potentially be a problem if you run __check_iommufd_broken before detaching.
What I'm saying here is that we should make sure that __check_iommufd_broken runs on a system that has a device attached/bound to vfio.
There was a problem hiding this comment.
Yes, and CI actually points this out when we detach. vfntool is linked with libvfn in full, so __check_iommufd_broken runs and complains. I'm not sure how to properly improve on this.
There was a problem hiding this comment.
Indeed. and searching for the configuration value in /proc does not work in general as the kernel does not always have that available
There was a problem hiding this comment.
How about we just do this :
Joelgranados@c190243
Move into src/iommu. The intent is to consolidate both vfio and iommufd backends there. Signed-off-by: Klaus Jensen <[email protected]>
Rename iommu_state to iova_map (since that is what it really is). The iova map (and naive allocator) is not dependent on any of the iommu or vfio code. Signed-off-by: Klaus Jensen <[email protected]>
Add log_fatal() and log_fatal_if() macros. Signed-off-by: Klaus Jensen <[email protected]>
The function to get the device vfio id does not depend on any iommufd/vfio functionality. Move it to pci/util. Signed-off-by: Klaus Jensen <[email protected]>
Move the backend independent mapping functions into iommu/dma. Signed-off-by: Klaus Jensen <[email protected]>
Move the remaining functions from vfio/core into vfio/container. Signed-off-by: Klaus Jensen <[email protected]>
Add a small helpers to grab the iommu context from the derived device. In subsequent patches, this will result in a backend independent iommu context, but for now it is just the struct vfio_container that is grabbed. Signed-off-by: Klaus Jensen <[email protected]>
struct vfio_group.path member is not const. Remove the qualifier. Signed-off-by: Klaus Jensen <[email protected]>
We either return directly or jump to free_group_path. Since free() does not set errno in any case, there is no need to save errno temporarily. Remove the logic. Signed-off-by: Klaus Jensen <[email protected]>
All of the container/group stuff belongs in iommu/ as well, leaving only generic vfio stuff like irq and pci configuration in vfio/. Signed-off-by: Klaus Jensen <[email protected]>
libvfn uses "/* ... */"-style comments and we do not need comments on endif's if they only span a few lines. Tidy this up. Signed-off-by: Klaus Jensen <[email protected]>
A particular iommu may not report any additional capabilities (as indicated by the VFIO_IOMMU_INFO_CAPS flag). That is not an error just because linux/vfio.h defines the flag. Signed-off-by: Klaus Jensen <[email protected]>
Do not define several __autofree variables on the same line. Split them up. Signed-off-by: Klaus Jensen <[email protected]>
Code movement to make the following patches easier on the eye. No functional change. Signed-off-by: Klaus Jensen <[email protected]>
Refactor iommufd iova range querying into separate function. Signed-off-by: Klaus Jensen <[email protected]>
With iommufd the struct vfio_container identifies an IOAS. Currently, it also holds the file descriptor from opening /dev/iommu. Since we only need to open it onces, store the fd in a global variable instead. This allows us to properly repurpose vfio_init_container to create an IOAS and remove it from get_device_fd(). Signed-off-by: Klaus Jensen <[email protected]>
Firstl, in general, libvfn (as a library) is not chatty unless built with debug. Replace log_error() with log_debug(). Secondly, remove the unneeded saved_errno logic. Finally, do not print the errno description. Leave that to the library consumer. Signed-off-by: Klaus Jensen <[email protected]>
Inline it. Signed-off-by: Klaus Jensen <[email protected]>
Code movement to make the following patches easier on the eye. No functional change. Signed-off-by: Klaus Jensen <[email protected]>
Introduce a general iommu context (struct iommu_ctx). This removes the last remnants of the vfio oriented public API and generalizes it. Signed-off-by: Klaus Jensen <[email protected]>
Fixup documentation. Signed-off-by: Klaus Jensen <[email protected]>
Even if we have a kernel with iommufd vfio features enabled, the kernel may not be configured for modern iommufd (i.e., no device cdevs). In that case we want to fall back to legacy vfio. Signed-off-by: Klaus Jensen <[email protected]>
Show if we are building support for iommufd. Signed-off-by: Klaus Jensen <[email protected]>
Switch to Fedora on our custom VM and run tests on both 38 (for now) and rawhide. Using rawhide makes sure we have the iommufd headers. We should be able to get rid of rawhide from Fedora 40. Signed-off-by: Klaus Jensen <[email protected]>
Fully linking vfntool with libvfn causes libvfn's various constructors to run, which for one, will, if enabled in the build, cause it to complain about iommufd being broken if no device has been bound to vfio-pci yet. Since vfntool only depend on some support sources and the pci/util parts of libvfn, just build against those compilation units instead and drop the full linkage. Signed-off-by: Klaus Jensen <[email protected]>
Use struct iommu_iova_range from linux/iommufd.h everywhere and define it if not available. Signed-off-by: Klaus Jensen <[email protected]>
Add trace events for each of iommufd and vfio. Signed-off-by: Klaus Jensen <[email protected]>
Move the core skiplist into util/skiplist and make it generic. The struct iova_map using it now provides an iova_cmp function that is used by the generic skiplist_find function. Signed-off-by: Klaus Jensen <[email protected]>
Embed the iova_map directly into iommu/context and iommu/dma. Signed-off-by: Klaus Jensen <[email protected]>
Add a helper that may be used to verify that n * sz does not overflow size_t. Abort if not. Signed-off-by: Klaus Jensen <[email protected]>
The iova_map does not to deal with iova range or iova allocation metadata. Move this into the outer struct iommu_ctx. Signed-off-by: Klaus Jensen <[email protected]>
Add iommu_translate_vaddr() that just looks up an iova mapping. Signed-off-by: Klaus Jensen <[email protected]>
Add helper to print an iova range. Signed-off-by: Klaus Jensen <[email protected]>
The iommufd backend uses the kernel iova allocation mechanism, so move the logic from iommu/dma to iommu/vfio. Signed-off-by: Klaus Jensen <[email protected]>
Add dma_unmap_all to the iommu context ops. Signed-off-by: Klaus Jensen <[email protected]>
The nvme_sync() helper function maps a buffer, issues the command, and unmaps the buffer. When we removed ephemeral IOVAs we relied on the user to handle these temporary mappings from a reserved set of IOVAs at the start of the IOVA space. While ephemeral IOVAs are used rarely, it is nicer to be able to just pass a buffer that we know will only be mapped temporarily. The new iommufd backend does not care about temporary mappings since it uses the kernel iova allocation that handles this just fine. The vfio backend still needs some kind of special treatment. We retain a reserved space (by default, 64k) at the start of the iova range and use that for temporary (ephemeral) mappings, only now, instead of requiring the user to map from that area explicitly, a new IOMMU_MAP_EPHEMERAL flags may be passed to indicate the temporary nature of the iova. Like before, we recycle the whole ephemeral space when no mappings are in use. Signed-off-by: Klaus Jensen <[email protected]>
Add missing log_fmt define on some files that logs. Also, do not default to the default debug level if LOGV is set above DEBUG. In that case, just remain at DEBUG level. Signed-off-by: Klaus Jensen <[email protected]>
Upload the testlog if a unit test fails. Signed-off-by: Klaus Jensen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's a lot of stuff here. All patch build up to introducing
struct iommu_ctxto generalize the backends.Within the
43 files changed, 1184 insertions(+), 1083 deletions(-), there are some fixes here and there as well.This is a shit-ton of code to review. My sincere apologies.