virtio-devices: iommu: allow limiting maximum address width in bits #6900

edigaryev · 2025-01-04T10:32:24Z

Currently, Cloud Hypervisor does not set a VIRTIO_IOMMU_F_INPUT_RANGE feature bit for the VirtIO IOMMU device, which, according to spec, means that the guest may use the whole 64-bit address space is for IOMMU purposes:

If the feature is not offered, virtual mappings span over the whole 64-bit address space (start = 0, end = 0xffffffff ffffffff)

As far as I am aware, there are currently no host platforms on the market capable of addressing the whole 64-bit address space.

For example, I am currently working with a host platform that reports 39-bit address space for IOMMU purposes:

DMAR: Host address width 39

When running a VFIO pass-through guest on such a platform, NVIDIA driver in guest gets DMA mapping failures when working with large data, and this results in Cloud Hypervisor exiting with the following error:

cloud-hypervisor: 1501.220535s: <__iommu> ERROR:virtio-devices/src/thread_helper.rs:53 -- Error running worker: HandleEvent(Failed to process request queue : ExternalMapping(Custom { kind: Other, error: "failed to map memory for VFIO container, iova 0x7fff00000000, gpa 0x24ce25000, size 0x1000: IommuDmaMap(Error(22))" }))

Passing --platform iommu_address_width=39 to Cloud Hypervisor built with this change fixes this.

rbradford · 2025-01-04T14:13:09Z

tests/integration.rs

+            .args(["--memory", "size=4G"])
+            .args(["--kernel", fw_path(FwType::RustHypervisorFirmware).as_str()])
+            .args(["--device", format!("path={NVIDIA_VFIO_DEVICE}").as_str()])
+            .args(["--platform", "iommu_address_width=42"])


I don't think this test will have a virtio-iommu device as that is only added if any device has iommu=on

Good catch!

Should I keep the test, modifying it to use iommu=on, or have it somewhere else where it's more appropriate in the test suite?

Added iommu=on, but not sure if the host running this test is actually configured to support this option.

edigaryev · 2025-01-04T14:53:07Z

Force-pushed to fix the formatting.

russell-islam · 2025-01-07T20:46:20Z

Is it possible to break down the single commit into multiple like below?

Main changes (vmm, if possible make virtio changes in different commit)
fuzz changes
test scenario

edigaryev · 2025-01-07T21:24:53Z

Is it possible to break down the single commit into multiple like below?

@russell-islam would you mind sharing your rationale behind this?

The PR itself is pretty small (+102 -5), so I'm curious about your goal in splitting it further 🤔

russell-islam · 2025-01-07T21:39:17Z

Is it possible to break down the single commit into multiple like below?

@russell-islam would you mind sharing your rationale behind this?

The PR itself is pretty small (+102 -5), so I'm curious about your goal in splitting it further 🤔

Typically we try to separate the changes to each crate/components if it is in dependent. It's fine if the commit is small. In the commit we always try to follow the format crate: commit message. At least I got these type of impression before for my PRs.

edigaryev · 2025-01-07T22:00:03Z

Typically we try to separate the changes to each crate/components if it is in dependent.

@russell-islam please take a look at change for fuzz/fuzz_targets/iommu.rs again.

It's inherently related to the "main changes", because virtio_devices::Iommu::new()'s signature was changed, and so we account for that by providing a missing argument.

What would be the point of splitting this into a separate commit?

russell-islam · 2025-01-07T22:20:58Z

Is it possible to break down the single commit into multiple like below?

@russell-islam would you mind sharing your rationale behind this?
The PR itself is pretty small (+102 -5), so I'm curious about your goal in splitting it further 🤔

Typically we try to separate the changes to each crate/components if it is in dependent. It's fine if the commit is small. In the commit we always try to follow the format crate: commit message. At least I got these type of impression before for my PRs.

Just add a tiny commit for this. That's okay. Then make another commit for test related changes.

Please try to see if the following split is possible.

vmm changes(just config related)
virtio-devices changes along with vmm(device_manager)
fuzz
tests

It's okay to have small commits with independent changes.

edigaryev · 2025-01-08T08:31:55Z

vmm changes(just config related)

virtio-devices changes along with vmm(device_manager)

fuzz

tests

I've performed the split and force pushed the changes, please take a look.

It's okay to have small commits with independent changes.

I agree, but fuzz changes, for example, are dependent on virtio-devices changes (signature change), yet you ask to split them 🤷

rbradford · 2025-01-08T10:17:40Z

vmm changes(just config related)

virtio-devices changes along with vmm(device_manager)

fuzz

tests

I've performed the split and force pushed the changes, please take a look.

It's okay to have small commits with independent changes.

I agree, but fuzz changes, for example, are dependent on virtio-devices changes (signature change), yet you ask to split them 🤷

If the fuzz changes are tightly dependent on the virtio device changes (due to function signature changes) then you were correct to merge them. The CI checks that each commit is independently compilable (however I don't think it spans across the the main and separate fuzz workspace.)

edigaryev · 2025-01-08T12:01:06Z

If the fuzz changes are tightly dependent on the virtio device changes (due to function signature changes) then you were correct to merge them.

Force pushed a new set of commits.

russell-islam · 2025-01-08T17:55:53Z

vmm changes(just config related)

virtio-devices changes along with vmm(device_manager)

fuzz

tests

I've performed the split and force pushed the changes, please take a look.

It's okay to have small commits with independent changes.

I agree, but fuzz changes, for example, are dependent on virtio-devices changes (signature change), yet you ask to split them 🤷

Thats fine.

russell-islam

LGTM

russell-islam · 2025-01-08T22:15:12Z

fuzz/fuzz_targets/iommu.rs

        SeccompAction::Allow,
        EventFd::new(EFD_NONBLOCK).unwrap(),
        ((MEM_SIZE - IOVA_SPACE_SIZE) as u64, (MEM_SIZE - 1) as u64),
+        64,


Use something like, const MAX_IOMMU_ADDRESS_WIDTH_BITS: u8 = 64;

Since you've asked me to split the commits above, and @rbradford stated that commits should be independently compilable (atomic), this would not be possible to achieve because:

virtio-devices: iommu: allow limiting maximum address width in bits contains this line

vmm: introduce platform option to limit maximum IOMMU address width contains the MAX_IOMMU_ADDRESS_WIDTH_BITS constant

rbradford · 2025-01-14T12:54:19Z

Please rebase these changes and force push - I want to ensure we're running atop the latest tree as there have been some changes related to the kernel CI.

Signed-off-by: Nikolay Edigaryev <[email protected]>

Currently, Cloud Hypervisor does not set a VIRTIO_IOMMU_F_INPUT_RANGE feature bit for the VirtIO IOMMU device, which, according to spec[1], means that the guest may use the whole 64-bit address space is for IOMMU purposes: >If the feature is not offered, virtual mappings span over the whole >64-bit address space (start = 0, end = 0xffffffff ffffffff) As far as I am aware, there are currently no host platforms on the market capable of addressing the whole 64-bit address space. For example, I am currently working with a host platform that reports 39-bit address space for IOMMU purposes: >DMAR: Host address width 39 When running a VFIO pass-through guest on such a platform, NVIDIA driver in guest gets DMA mapping failures when working with large data, and this results in Cloud Hypervisor exiting with the following error: >cloud-hypervisor: 1501.220535s: <__iommu> >ERROR:virtio-devices/src/thread_helper.rs:53 -- Error running worker: >HandleEvent(Failed to process request queue : ExternalMapping(Custom >{ kind: Other, error: "failed to map memory for VFIO container, iova >0x7fff00000000, gpa 0x24ce25000, size 0x1000: IommuDmaMap(Error(22))" >})) Passing "--platform iommu_address_width=39" to Cloud Hypervisor built with this change fixes this. [1]: https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/ virtio-v1.3-csd01.html#x1-5420006 Signed-off-by: Nikolay Edigaryev <[email protected]>

edigaryev · 2025-01-14T13:00:52Z

Please rebase these changes and force push - I want to ensure we're running atop the latest tree as there have been some changes related to the kernel CI.

@rbradford done 👌

rbradford

I was going to ask if 64 bits is a sensible default - and what QEMU does. I can see that QEMU defaults to 64-bits too.

rbradford · 2025-01-14T15:11:56Z

@edigaryev Your newly added test failed - I think it's because there is no virtio-iommu device added as no devices are placed behind an iommu (i.e. with iommu=on or with an iommu segment --platform num_pci_segments=2,iommu_segments=1 - the latter might be better than placing the NIVIDIA behind another layer of IOMMU.)

edigaryev · 2025-01-14T20:02:07Z

@rbradford thanks for the heads up and a good catch!

I've updated the test in f8aa0e5 to additionally specify num_pci_segments=2,iommu_segments=1 in --platform's arguments.

Signed-off-by: Nikolay Edigaryev <[email protected]>

edigaryev · 2025-01-14T20:38:27Z

Fixed code formatting for test.

edigaryev · 2025-01-14T21:20:23Z

@rbradford any idea what's wrong with Clippy?

These 4 checks are failing:

rbradford · 2025-01-14T21:22:41Z

@rbradford any idea what's wrong with Clippy?

Yes - this is an issue with beta clippy - rust-lang/rust-clippy#13953 affecting all our PRs. I've not disabled beta as I think (hope?) this will be backporting soon. Failures of clippy beta does not block merging.

edigaryev requested a review from a team as a code owner January 4, 2025 10:32

edigaryev force-pushed the iommu-address-width-bits branch from 84ad1ae to fb89231 Compare January 4, 2025 10:34

rbradford reviewed Jan 4, 2025

View reviewed changes

edigaryev force-pushed the iommu-address-width-bits branch 2 times, most recently from 7d9bb66 to d6d5c69 Compare January 4, 2025 14:43

edigaryev requested a review from rbradford January 4, 2025 14:44

edigaryev force-pushed the iommu-address-width-bits branch from d6d5c69 to 4e05c8f Compare January 4, 2025 14:52

edigaryev mentioned this pull request Jan 6, 2025

Reduce VirtIO IOMMU address width from 64 to 39 bits on amd64 cirruslabs/vetu#61

Merged

edigaryev force-pushed the iommu-address-width-bits branch from 4e05c8f to 7d6efea Compare January 8, 2025 08:29

edigaryev force-pushed the iommu-address-width-bits branch from 7d6efea to 38b38e1 Compare January 8, 2025 12:00

russell-islam approved these changes Jan 8, 2025

View reviewed changes

edigaryev added 2 commits January 14, 2025 17:00

vmm: introduce platform option to limit maximum IOMMU address width

782a105

Signed-off-by: Nikolay Edigaryev <[email protected]>

edigaryev force-pushed the iommu-address-width-bits branch from 38b38e1 to 31c3780 Compare January 14, 2025 13:00

rbradford approved these changes Jan 14, 2025

View reviewed changes

rbradford enabled auto-merge January 14, 2025 14:08

rbradford added this pull request to the merge queue Jan 14, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 14, 2025

edigaryev force-pushed the iommu-address-width-bits branch from 31c3780 to f8aa0e5 Compare January 14, 2025 20:00

rbradford enabled auto-merge January 14, 2025 20:36

tests: ensure that IOMMU maximum address width in bits is respected

957f4ea

Signed-off-by: Nikolay Edigaryev <[email protected]>

auto-merge was automatically disabled January 14, 2025 20:38
Head branch was pushed to by a user without write access

edigaryev force-pushed the iommu-address-width-bits branch from f8aa0e5 to 957f4ea Compare January 14, 2025 20:38

rbradford enabled auto-merge January 14, 2025 20:45

rbradford added this pull request to the merge queue Jan 14, 2025

Merged via the queue into cloud-hypervisor:main with commit 5a8df36 Jan 14, 2025
34 of 38 checks passed

edigaryev deleted the iommu-address-width-bits branch January 16, 2025 08:39

rbradford mentioned this pull request Apr 17, 2025

pci: vfio: make DMA mapping errors non-fatal #7030

Closed

virtio-devices: iommu: allow limiting maximum address width in bits #6900

virtio-devices: iommu: allow limiting maximum address width in bits #6900

Uh oh!

Conversation

edigaryev commented Jan 4, 2025

Uh oh!

rbradford Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

edigaryev Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

edigaryev Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

edigaryev commented Jan 4, 2025

Uh oh!

russell-islam commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edigaryev commented Jan 7, 2025

Uh oh!

russell-islam commented Jan 7, 2025

Uh oh!

edigaryev commented Jan 7, 2025

Uh oh!

russell-islam commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edigaryev commented Jan 8, 2025

Uh oh!

rbradford commented Jan 8, 2025

Uh oh!

edigaryev commented Jan 8, 2025

Uh oh!

russell-islam commented Jan 8, 2025

Uh oh!

russell-islam left a comment

Choose a reason for hiding this comment

Uh oh!

russell-islam Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edigaryev Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbradford commented Jan 14, 2025

Uh oh!

edigaryev commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbradford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rbradford commented Jan 14, 2025

Uh oh!

edigaryev commented Jan 14, 2025

Uh oh!

edigaryev commented Jan 14, 2025

Uh oh!

edigaryev commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbradford commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

russell-islam commented Jan 7, 2025 •

edited

Loading

russell-islam commented Jan 7, 2025 •

edited

Loading

russell-islam Jan 8, 2025 •

edited

Loading

edigaryev Jan 9, 2025 •

edited

Loading

edigaryev commented Jan 14, 2025 •

edited

Loading

edigaryev commented Jan 14, 2025 •

edited

Loading

rbradford commented Jan 14, 2025 •

edited

Loading