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

vmm: add pci_segment mmio aperture configs #6387

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

thomasbarrett
Copy link
Contributor

@thomasbarrett thomasbarrett commented Apr 14, 2024

When using multiple PCI segments, the 32-bit and 64-bit mmio aperture is split equally between each segment. Add an option to configure the 'weight'. For example, a PCI segment with a mmio32_aperture_weight of 2 will be allocated twice as much 32-bit mmio space as a normal PCI segment.

Alternatives Considered

I considered adding a mmio32_aperture_size config instead of mmio32_aperture_weight config to precisely configure the size of the mmio apertures for each segment. I chose to not implement this because the logic to implement that correctly was a lot more complicated. Also, allocating mmio space between PCI segments does not actually require that level of precision.

Use Case

Unfortunately, some devices actually use quite a bit of 32-bit MMIO space. For example, a NVIDIA NVSwitch device uses 32 MB of 32-bit mmio. When using a few PCI segments and a few NVSwitch VFIO devices, you can quickly overwhelm the default 32-bit mmio allocation for a PCI segment.

Example

The following example allocates 2 PCI segments. The first segment is allocated 1/4 of the available mmio32 space. The second segment is allocated 3/4 of the available mmio32 space.

--platform pci_segments=3
--pci-segment pci_segment=1,mmio32_aperture_weight=1
--pci-segment pci_segment=2,mmio32_aperture_weight=3

Backward Compatability

This is completely backwards compatible. By default, each PCI segment has a aperture weight of 1.

@thomasbarrett thomasbarrett requested a review from a team as a code owner April 14, 2024 23:15
@thomasbarrett thomasbarrett force-pushed the pci_segment_main branch 7 times, most recently from 899c697 to 4be3b0f Compare April 15, 2024 01:13
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

I guess this is fine (the code and solution are excellent - i'm just always loathe to increase our feature set / API interface as there is more to test and maintain). When adding the segment support I considered only supporting devices with 32-bit BARs on segment 0 - unfortunately the experience would be frustrating as there is no way to know a device needs 32-bit bars until we call allocate_bars which is relatively late in the device initialisation.

What about testing? Could do some unit tests - if you extracted a function create_mmio_allocators then that could be unit tested - config changes should be unit tested too. Any scope for integration testing?

Copy link
Contributor

@up2wing up2wing left a comment

Choose a reason for hiding this comment

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

The code LGTM.
Maybe it's better to add some description in docs/iommu.md.

@thomasbarrett
Copy link
Contributor Author

Now that the solution / UI has high level approval, I will go back today and add unit tests, integration tests, and documentation.

@thomasbarrett thomasbarrett force-pushed the pci_segment_main branch 8 times, most recently from bbba421 to 6435866 Compare April 17, 2024 21:39
@thomasbarrett
Copy link
Contributor Author

thomasbarrett commented Apr 17, 2024

Okay, I have added:

  1. unit tests for both create_mmio_allocators.
  2. unit tests for the config additions.
  3. documentation for the new config additions.

I can't think of a good integration test. If anyone has suggestions, I would be happy to add. Otherwise, I think this is g2g.

Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

I'm happy with unit tests - we should have more of those - we should aim to have more of those - they run much quicker and can also help structure the code for readability.

vmm/src/config.rs Outdated Show resolved Hide resolved
Copy link
Member

@likebreath likebreath left a comment

Choose a reason for hiding this comment

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

LGTM. Only couple of nits about documentation formatting. Thank you for the contributions.

docs/vfio.md Outdated Show resolved Hide resolved
docs/vfio.md Outdated Show resolved Hide resolved
When using multiple PCI segments, the 32-bit and 64-bit mmio
aperture is split equally between each segment. Add an option
to configure the 'weight'. For example, a PCI segment with a
`mmio32_aperture_weight` of 2 will be allocated twice as much
32-bit mmio space as a normal PCI segment.

Signed-off-by: Thomas Barrett <tbarrett@crusoeenergy.com>
@likebreath likebreath added this pull request to the merge queue Apr 24, 2024
Merged via the queue into cloud-hypervisor:main with commit e7e856d Apr 24, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants