Hi, On Tue, Jun 22, 2021 at 4:35 AM Robin Murphy <robin.murphy@xxxxxxx> wrote: > > Hi Doug, > > On 2021-06-22 00:52, Douglas Anderson wrote: > > > > This patch attempts to put forward a proposal for enabling non-strict > > DMA on a device-by-device basis. The patch series requests non-strict > > DMA for the Qualcomm SDHCI controller as a first device to enable, > > getting a nice bump in performance with what's believed to be a very > > small drop in security / safety (see the patch for the full argument). > > > > As part of this patch series I am end up slightly cleaning up some of > > the interactions between the PCI subsystem and the IOMMU subsystem but > > I don't go all the way to fully remove all the tentacles. Specifically > > this patch series only concerns itself with a single aspect: strict > > vs. non-strict mode for the IOMMU. I'm hoping that this will be easier > > to talk about / reason about for more subsystems compared to overall > > deciding what it means for a device to be "external" or "untrusted". > > > > If something like this patch series ends up being landable, it will > > undoubtedly need coordination between many maintainers to land. I > > believe it's fully bisectable but later patches in the series > > definitely depend on earlier ones. Sorry for the long CC list. :( > > Unfortunately, this doesn't work. In normal operation, the default > domains should be established long before individual drivers are even > loaded (if they are modules), let alone anywhere near probing. The fact > that iommu_probe_device() sometimes gets called far too late off the > back of driver probe is an unfortunate artefact of the original > probe-deferral scheme, and causes other problems like potentially > malformed groups - I've been forming a plan to fix that for a while now, > so I for one really can't condone anything trying to rely on it. > Non-deterministic behaviour based on driver probe order for multi-device > groups is part of the existing problem, and your proposal seems equally > vulnerable to that too. Doh! :( I definitely can't say I understand the iommu subsystem amazingly well. It was working for me, but I could believe that I was somehow violating a rule somewhere. I'm having a bit of a hard time understanding where the problem is though. Is there any chance that you missed the part of my series where I introduced a "pre_probe" step? Specifically, I see this: * really_probe() is called w/ a driver and a device. * -> calls dev->bus->dma_configure() w/ a "struct device *" * -> eventually calls iommu_probe_device() w/ the device. * -> calls iommu_alloc_default_domain() w/ the device * -> calls iommu_group_alloc_default_domain() * -> always allocates a new domain ...so we always have a "struct device" when a domain is allocated if that domain is going to be associated with a device. I will agree that iommu_probe_device() is called before the driver probe, but unless I missed something it's after the device driver is loaded. ...and assuming something like patch #1 in this series looks OK then iommu_probe_device() will be called after "pre_probe". So assuming I'm not missing something, I'm not actually relying the IOMMU getting init off the back of driver probe. > FWIW we already have a go-faster knob for people who want to tweak the > security/performance compromise for specific devices, namely the sysfs > interface for changing a group's domain type before binding the relevant > driver(s). Is that something you could use in your application, say from > an initramfs script? We've never had an initramfs script in Chrome OS. I don't know all the history of why (I'm trying to check), but I'm nearly certain it was a conscious decision. Probably it has to do with the fact that we're not trying to build a generic distribution where a single boot source can boot a huge variety of hardware. We generally have one kernel for a class of devices. I believe avoiding the initramfs just keeps things simpler. I think trying to revamp Chrome OS to switch to an initramfs type system would be a pretty big undertaking since (as I understand it) you can't just run a little command and then return to the normal boot flow. Once you switch to initramfs you're committing to finding / setting up the rootfs yourself and on Chrome OS I believe that means a whole bunch of dm-verity work. ...so probably the initramfs is a no-go for me, but I'm still crossing my fingers that the pre_probe() might be legit if you take a second look at it? -Doug
