pci: loosen PCIe hot-plug requirements
ClosedPublic
Actions

Authored by chuck on May 18 2020, 1:27 PM.

Details

Reviewers

jhb
scottl
mav

Commits

rS362027: pci: loosen PCIe hot-plug requirements

Summary

The original PCIe hot-plug code required a couple of things which cause
PCI probing errors on the qemu q35 system and possibly physical systems
(Dell R6515).

First, the code required the hot-plug IRQ's to be unique (i.e. not
shared). On qemu q35, all PCIe hot-plug IRQ's are identical. Fix by
allocating the IRQ as RF_SHAREABLE.

Second, the code required the Electromechanical Interlock (Slot Status
EIS) to be engaged if present (Slot Capability EIP). Several platforms
including qemu q35 set EIP but not EIS. Fix by deleting the check.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

chuck created this revision.May 18 2020, 1:27 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 18 2020, 1:27 PM

chuck requested review of this revision.May 18 2020, 1:27 PM

Harbormaster completed remote builds in B31147: Diff 71918.May 18 2020, 1:27 PM

chuck added reviewers: scottl, mav.May 26 2020, 3:13 PM

mav added inline comments.May 26 2020, 4:57 PM

sys/dev/pci/pci_pci.c
1083 ↗	(On Diff #71918)	I've never had hardware with interlock myself, but this seems like just removing the interlock support, that can make somebody unhappy.
1333 ↗	(On Diff #71918)	It seems the code in pcib_pcie_intr_hotplug() does not expect to be called for unrelated reasons, that are normal in case of shared interrupts.

imp added inline comments.May 26 2020, 8:45 PM

sys/dev/pci/pci_pci.c
1083 ↗	(On Diff #71918)	If I were doing it, I'd park this behind a sysctl that defaulted to 'don't block', but I don't know how relevant this actually is....

I would perhaps say "some" rather than several platforms. Lying about an EI being present doesn't appear to be super prevalent. The interrupt issue is also only seen on QEMU. I doubt it exists on any real hardware. I would perhaps word the paragraph about shared interrupts more like:

Allocate the hot plug interrupt shared to support INTx interrupts.  The hot plug
interrupt should normally be an MSI interrupt since PCI-e mandates MSI interrupt
support, but QEMU's Q35 bridge only provides INTx interrupts.

sys/dev/pci/pci_pci.c
1083 ↗	(On Diff #71918)	This doesn't remove the code to engage / disengage the EI, it simply doesn't require the EI to report a status of engaged before trying to use the card. Linux doesn't do this check, and the spec doesn't really say that you have to do this. I think this change is ok.
1333 ↗	(On Diff #71918)	This is only needed for the !MSI case, but I must have made that change in my branch before it was merged to head and missed it when adding the !MSI fallback. I'm really surprised that you have a bridge without MSI. The fact that QEMU doesn't do MSI is mind boggling. MSI is so much nicer for emulators than INTx, and PCI-e hot plug is PCI-e only, and PCI-e mandates MSI anyway, so the q35 bridge is arguably violating the spec. The interrupt should be fine though as the handler just inspects whatever the current state is and drives the state machine from there. It might reset a timer and miss a timeout due to spurious interrupts but probably work fine otherwise.