[VFIO] Chapter 2, Section 3: BAR areas and mappings - #6114
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/vfio #6114 +/- ##
================================================
- Coverage 82.85% 82.77% -0.09%
================================================
Files 280 280
Lines 31115 31419 +304
================================================
+ Hits 25781 26006 +225
- Misses 5334 5413 +79
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7cfcfd7 to
3acd6b7
Compare
Make `arrayvec` non optional as it will be used in the future VFIO commits. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
3acd6b7 to
695c68d
Compare
Add new utility functions for dealing with alignment of addresses. These will be used in the following commits. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
695c68d to
15ec203
Compare
ShadowCurse
added
the
Status: Awaiting review
15ec203 to
9239cd9
Compare
| } | ||
| } | ||
|
|
||
| /// Information about the location of the hole in the bar |
There was a problem hiding this comment.
Can you explain for future readers what a hole in the bar is?
There was a problem hiding this comment.
Would VfioBarEmulatedRegion be a more descriptive name?
There was a problem hiding this comment.
switched to VfioBarEmulatedArea
| region_offset: u64, | ||
| /// Offset within BAR | ||
| offset: u64, | ||
| /// Size |
There was a problem hiding this comment.
Can we please get rid of these useless comments? We can disable the directive that requires them in this module.
There was a problem hiding this comment.
removed redundant ones
| ); | ||
| } else { | ||
| let can_mmap = region_info.flags & VFIO_REGION_INFO_FLAG_MMAP != 0; | ||
| if can_mmap || sparse_mmap_cap.is_some() { |
There was a problem hiding this comment.
Can we not extract all this logic into a separate function? It's hard to follow the code when functions are so long.
| /// KVM slot assigned to the mapping | ||
| slot: u32, | ||
| /// Guest physical address | ||
| iova: u64, |
There was a problem hiding this comment.
The comment says GPA and the name says IOVA. How can it be IOVA if we don't map the BARs into the IOMMU?
| return Err(VfioError::Mmap); | ||
| } | ||
|
|
||
| let iova = area.bar_gpa + area.offset; |
| struct BarArea { | ||
| /// BAR gpa | ||
| bar_gpa: u64, | ||
| /// Offset into VFIO region |
There was a problem hiding this comment.
Pointing to the beginning of the BAR?
And perhaps vfio_region_offset for the variable?
There was a problem hiding this comment.
Or just add the two when creating the object and store a single offset.
There was a problem hiding this comment.
changed that code a bit and merged 2 offsets
| }; | ||
|
|
||
| if hva_ptr == libc::MAP_FAILED { | ||
| return Err(VfioError::Mmap); |
There was a problem hiding this comment.
can we return the errno?
| "Error on unmapping host memory on VFIO device creation failure: {r:?}. \ | ||
| Continuing with other regions removal." |
There was a problem hiding this comment.
it would be useful to log errno here as well
| "Error on unmapping host memory for BAR in a VFIO device: {r:?}. Continuing \ | ||
| with other regions removal." |
| // Merge into it so we don't register the same MMIO range | ||
| // twice. | ||
| if let Some(last) = bar_hole_infos.last_mut() | ||
| && last.gpa == pba_gpa |
There was a problem hiding this comment.
is it possible for these to span multiple pages? if so, this check is insufficient and there may be an overlap in the mappings. When we compute the areas below, the code already takes care of this, we could adopt the same logic here of deferring the addition only after collecting table and pba and sorting them.
iow something like
// Both structures are rounded out to whole host pages, so their page
// ranges can overlap even though the structures themselves cannot.
// Emit their union: one hole per contiguous run, tagged with what it
// covers.
let mut holes = [
(
msix_table_offset,
msix_table_size,
VfioBarHoleUsageFlags::TABLE,
),
(msix_pba_offset, msix_pba_size, VfioBarHoleUsageFlags::PBA),
];
holes.sort_unstable_by_key(|(offset, _, _)| *offset);
for (offset, size, usage) in holes {
if size == 0 {
continue;
}
let gpa = bar_gpa + offset;
match bar_hole_infos.last_mut() {
Some(last) if gpa < last.gpa + last.size => {
last.usage |= usage;
last.size = (last.gpa + last.size).max(gpa + size) - last.gpa;
}
_ => bar_hole_infos.push(VfioBarHole { gpa, size, usage }),
}
}There was a problem hiding this comment.
added a separate vfio_add_emulated_area for this. It should handle any overlap
| region_size, | ||
| )); | ||
| } | ||
| areas.push(BarArea { |
There was a problem hiding this comment.
should we carve the msix/pba holes as well? is it possible for them to land here? iirc the CH code does that but i'm not sure if it's needed or if we can trust the vfio driver to always give us valid mmap-able areas.
There was a problem hiding this comment.
expanded the checks to verify these areas do not overlap with msix table/pba
| let mut first_gap_size = msix_table_size; | ||
| let mut second_gap_offset = msix_pba_offset; | ||
| let mut second_gap_size = msix_pba_size; | ||
| if second_gap_offset < first_gap_offset { |
There was a problem hiding this comment.
is there a unit test covering the swap?
|
|
||
| /// Internal type to store areas needed to be mmaped for the device | ||
| #[derive(Debug, Clone, Copy)] | ||
| struct BarArea { |
There was a problem hiding this comment.
nit: a more descriptive name would help. This is only the mmappable area of a BAR, right? MappableBarArea?
| let mut first_gap_offset = msix_table_offset; | ||
| let mut first_gap_size = msix_table_size; | ||
| let mut second_gap_offset = msix_pba_offset; | ||
| let mut second_gap_size = msix_pba_size; | ||
| if second_gap_offset < first_gap_offset { | ||
| second_gap_offset = msix_table_offset; | ||
| second_gap_size = msix_table_size; | ||
| first_gap_offset = msix_pba_offset; | ||
| first_gap_size = msix_pba_size; | ||
| } | ||
| let mut offset = 0; | ||
| if first_gap_size != 0 { | ||
| let area_size = first_gap_offset - offset; | ||
| if area_size != 0 { | ||
| areas.push(BarArea { | ||
| bar_gpa, | ||
| region_offset: region_info.offset, | ||
| offset, | ||
| size: area_size, | ||
| prot, | ||
| }); | ||
| } | ||
| offset = first_gap_offset + first_gap_size; | ||
| } | ||
| if second_gap_size != 0 { | ||
| if offset < second_gap_offset { | ||
| let area_size = second_gap_offset - offset; | ||
| if area_size != 0 { | ||
| areas.push(BarArea { | ||
| bar_gpa, | ||
| region_offset: region_info.offset, | ||
| offset, | ||
| size: area_size, | ||
| prot, | ||
| }); | ||
| } | ||
| } | ||
| offset = offset.max(second_gap_offset + second_gap_size); | ||
| } | ||
| let area_size = region_size - offset; | ||
| if area_size != 0 { | ||
| areas.push(BarArea { | ||
| bar_gpa, | ||
| region_offset: region_info.offset, | ||
| offset, | ||
| size: area_size, | ||
| prot, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think this is more readable as a loop. I had to stare at it for a while to understand what was going on.
| let mut first_gap_offset = msix_table_offset; | |
| let mut first_gap_size = msix_table_size; | |
| let mut second_gap_offset = msix_pba_offset; | |
| let mut second_gap_size = msix_pba_size; | |
| if second_gap_offset < first_gap_offset { | |
| second_gap_offset = msix_table_offset; | |
| second_gap_size = msix_table_size; | |
| first_gap_offset = msix_pba_offset; | |
| first_gap_size = msix_pba_size; | |
| } | |
| let mut offset = 0; | |
| if first_gap_size != 0 { | |
| let area_size = first_gap_offset - offset; | |
| if area_size != 0 { | |
| areas.push(BarArea { | |
| bar_gpa, | |
| region_offset: region_info.offset, | |
| offset, | |
| size: area_size, | |
| prot, | |
| }); | |
| } | |
| offset = first_gap_offset + first_gap_size; | |
| } | |
| if second_gap_size != 0 { | |
| if offset < second_gap_offset { | |
| let area_size = second_gap_offset - offset; | |
| if area_size != 0 { | |
| areas.push(BarArea { | |
| bar_gpa, | |
| region_offset: region_info.offset, | |
| offset, | |
| size: area_size, | |
| prot, | |
| }); | |
| } | |
| } | |
| offset = offset.max(second_gap_offset + second_gap_size); | |
| } | |
| let area_size = region_size - offset; | |
| if area_size != 0 { | |
| areas.push(BarArea { | |
| bar_gpa, | |
| region_offset: region_info.offset, | |
| offset, | |
| size: area_size, | |
| prot, | |
| }); | |
| } | |
| // Map the region minus the MSI-X holes: walk them in | |
| // ascending order and map whatever precedes each one. | |
| let mut holes = [ | |
| (msix_table_offset, msix_table_size), | |
| (msix_pba_offset, msix_pba_size), | |
| ]; | |
| holes.sort_unstable_by_key(|(offset, _)| *offset); | |
| let mut offset = 0; | |
| for (hole_offset, hole_size) in holes { | |
| if hole_size == 0 { | |
| continue; | |
| } | |
| if offset < hole_offset { | |
| areas.push(BarArea { | |
| bar_gpa, | |
| region_offset: region_info.offset, | |
| offset, | |
| size: hole_offset - offset, | |
| prot, | |
| }); | |
| } | |
| offset = offset.max(hole_offset + hole_size); | |
| } | |
| if offset < region_size { | |
| areas.push(BarArea { | |
| bar_gpa, | |
| region_offset: region_info.offset, | |
| offset, | |
| size: region_size - offset, | |
| prot, | |
| }); | |
There was a problem hiding this comment.
ok, applied the suggestion
| let mut bar_idx: u8 = 0; | ||
| while bar_idx < NUM_BAR_REGS { | ||
| let bar_gpa = bars.get_bar_addr(bar_idx); | ||
| if bar_gpa != 0 { |
There was a problem hiding this comment.
should we first check if the bar is used and then get the address like in the loop above (line 531)? get_bar_addr has mutlitple preconditions which make it panic and which are not verified here
| if bar_gpa != 0 { | |
| if bars.bars[bar_idx as usize].used() { | |
| let bar_gpa = bars.get_bar_addr(bar_idx); |
Calculate areas of BARs which either should be mmapped and provided to the guest or fully emulated. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
9239cd9 to
a62fe44
Compare
After calculating the DMA areas of BARs, map them into the guest. This involves `mmap`ing the device BAR into Firecracker virtual space, and creating KVM slots for them. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
a62fe44 to
476cade
Compare
3 participants
Changes
Another set of commits taken from #6055 PR. Here we add logic for calculating mappable areas of BARs (taking into account the holes left by the MSIX tables) and actually doing the actual map operations.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.