Page MenuHomeFreeBSD

Bhyve VT-d IOMMU: Support capability detection for multiple translation units
ClosedPublic

Authored by callum_callumaitchison.uk on Jan 28 2019, 12:06 PM.

Details

Summary

When an attempt is made to passthrough a PCI device to a bhyve VM (causing initialisation of IOMMU) on certain Intel chipsets using VT-d the PCI bus stops working entirely. This issue occurs with the E3-1275 v5 processor on C236 chipset and has also been encountered by others with different hardware in the Skylake series.

The processor has two VT-d translation units. The issue is caused by an attempt to use the VT-d device-IOTLB capability that is supported by only the first unit for devices attached to the second unit which lacks that capability. In the present revision only the capabilities of the first unit are checked and are assumed to be the same for all units.

The patch rectifies this issue by determining which unit is responsible for the device being added to a domain and then checking that unit's device-IOTLB capability. In addition to this a few fixes have been made to other instances where the first unit's capabilities are assumed for all units for domains they share. In these cases a mutual set of capabilities is determined. The patch should hopefully fix any bugs for current/future hardware with multiple translation units supporting different capabilities.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Overall good, just style nits and a diff reduction.

sys/amd64/vmm/intel/vtd.c
52

This macro appears to be used exactly once, so not sure why it is a macro at all.

123

This block is mostly just white space changes, if we reduce those it becomes clear that this is simply adding *drhds

187

style(9) variables should be declared in descending size order, struct, int, char in the above case, and in alpha order within a size group, "i, pathremaining, remaining" in the above case

371

Here do the constants 64 and 1024 come from? Should they be #define s?

460

style(9) when wrapping lines the following line is indented 4 spaces from the prior and no alignment attempts are made.

689

Wrapped text is indented 4 spaces

Question regarding comments

sys/amd64/vmm/intel/vtd.c
187

How should this apply to "ACPI_DMAR_PCI_PATH"? Technically it is a struct, but it's a struct of two UINT8s and is therefore actually smaller than an int

371

64 * 1024 is the maximum possible return value of the existing "vtd_max_domains" function, the maximum number of supported domains allowed in the spec v3 ("Hardware supports 16-bit domain-ids with support for up to 64K domains" (hence 64*1024)).

Since it's only a placeholder value for finding the minimum and should never be used I think it would be better to just replace this with a non-specific value.

I'll replace it for "vtd_max_domains(vtdmaps[0])" since using INT_MAX would involve including sys/limits.h.

rgrimes added inline comments.Jan 28 2019, 5:50 PM
sys/amd64/vmm/intel/vtd.c
187

Its a struct, struct of anything goes first, struct containing a single int8 still goes first. These rules are about stack alignment mostly, structs are going to end up being a pointer on the local stack frame which is usually the longest type so style(9) has you put those first.

371

I would change it to 65536 and put a comment above it:
/* V3 spec 16-bit domain-ids support for 64K domains"
so that someone looking at the code knows why that is there

I do not break out things into n * 1024 when the spec is for a single constant, however that is not a global or style(9) type issue.

Changes made as suggested by previous comments

callum_callumaitchison.uk marked 8 inline comments as done.Jan 28 2019, 7:38 PM
egypcio added a subscriber: egypcio.May 3 2019, 7:02 AM
rgrimes accepted this revision as: rgrimes.May 3 2019, 8:44 AM

@jhb it would be good to not have this issue in 11.3

This revision is now accepted and ready to land.May 3 2019, 8:44 AM

This is a ping. The pr https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229852 has had another success report. This really should get in tree and in release, what is holding it up?

emaste added a subscriber: emaste.Jun 20 2019, 1:22 AM