Messages in this thread |  | | Date | Thu, 21 Nov 2013 14:21:38 +0800 | From | Yijing Wang <> | Subject | Re: [PATCH 1/1] IOMMU: Save pci device id instead of pci_dev* pointer for DMAR devices |
| |
On 2013/11/20 23:59, David Woodhouse wrote: > On Fri, 2013-11-08 at 08:46 -0700, Bjorn Helgaas wrote: >> >> I don't know the IOMMU drivers well either, but it seems like they >> rely on notifications of device addition and removal (see >> iommu_bus_notifier()). It doesn't seem right for them to also use the >> generic PCI interfaces like pci_get_domain_bus_and_slot() because the >> IOMMU driver should already know what devices exist and their >> lifetimes. It seems like confusion to mix the two. But I don't have >> a concrete suggestion. >
Hi David, Thanks for your review and comment!
> The generic IOMMU code has a notifier, and calls through to an > ->add_device() method in the specific IOMMU driver's iommu_ops. > > The Intel IOMMU driver predates that, and its scheme for mapping devices > to the correct DMAR unit is different. It happens entirely within the > get_domain_for_dev() function, which happens when we're first asked to > set up a mapping for a given device (when we don't already have the > answer stashed in dev->archdata). > > I think we should add an ->add_device() method to the Intel IOMMU > driver, and make it do much of what's in get_domain_for_dev() right now > — finding the "proxy" device (the upstream PCIe bridge or whatever), and > then looking through the ACPI DMAR table to find which DMAR unit that's > attached to. Then we stash that information (dmar, devfn) in > dev->archdata, and get_domain_for_dev() still has *some* work to do, > actually allocating a logical domain on the IOMMU in question, but not > as much. And refcount the damn domain instead of playing the horrid > tricks we currently do to hang it off the upstream proxy device *too*.
Intel IOMMU driver has an ->add_device() method already, .add_device = intel_iommu_add_device, this method was used to update iommu group info. Since Intel IOMMU driver has its own notifier, so maybe it's a nice candidate to do something. Currently, dmar driver parse DMAR table and find the pci device id under a specific DRHD. But only save the device pci_dev * pointer in devices array. So if this pci device was removed, this info became stale info. In the last version patch, I use pci device id intead of pci_dev * pointer array completely. This maybe introduce some unsafe issues. Because pci device maybe destroyed during process device dma mapping etc.
So, I have rework the patch and try to save pci device id as well as pci_dev *pointer, like:
struct dmar_device { u16 segment; u8 bus; u8 devfn; ----------->these tree will be used only when pci device add or remove, we will use them to update pci_dev * pointer in intel iommu driver notifier. struct list_head list; -->add to DRHD device list. struct pci_dev *pdev; --->use to hold the pci device }
What do you think about ?
In this new patch, we won't change the Intel iommu driver much, just enhance Intel driver iommu notifier to make DRHD device list always effect, not stale info.
I will send out this new patch soon.
> > My main concern here is that the DMAR table contains the PCI bus numbers > at boot time. Doing the lookup later will only work if we don't renumber > busses. Or if we have a way to look things up based on the *original* > bus number.
If we won't remove the pci device, the occupied buses won't be change, I think. And because in the new patch, we still use pci_dev *pointer to find match DRHD, so this is not a regression. Since DMAR also use pci device id to identify the support device, I have not found anything instead of device id.
In AMD IOMMU driver, it seems to use pci device id to identify drhd too, although I just take a quickly glanced at it, maybe not correctly.
> > The Intel IOMMU also has a bus notifier of its own which it only uses to > know when a driver is *detached*, so it can tear down the logical domain > for the corresponding device. Would be nice to have the generic IOMMU > notifier call a callback for us then too, perhaps.
Update the device info in Intel IOMMU driver is a good point.
Thanks! Yijing.
> >
-- Thanks! Yijing
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |