> On Wed, Feb 19, 2020 at 11:40 AM Lu Baolu <[email protected]> wrote:
> > With respect, this is problematical. The parent and all subdevices share
> > a single translation entry. The DMA mask should be consistent.
> >
> > Otherwise, for example, subdevice A has 64-bit DMA capability and uses
> > an identity domain for DMA translation. While subdevice B has 32-bit DMA
> > capability and is forced to switch to DMA domain. Subdevice A will be
> > impacted without any notification.
Looking closer, this problematic codepath may already be happening for VMD,
under intel_iommu_add_device(). Consider this function running for a VMD
subdevice, we hit:
domain = iommu_get_domain_for_dev(dev);
I can't quite grasp the code flow here, but domain->type now always seems
to return the domain type of the real DMA device, which seems like pretty
reasonable behaviour.
if (domain->type == IOMMU_DOMAIN_DMA) {
and as detailed in previous mails, the real VMD device seems to be in a DMA
domain by default, so we continue.
if (device_def_domain_type(dev) == IOMMU_DOMAIN_IDENTITY) {
Now we checked the default domain type of the subdevice. This seems rather
likely to return IDENTITY because that's effectively the default type...
ret = iommu_request_dm_for_dev(dev);
if (ret) {
dmar_remove_one_dev_info(dev);
dmar_domain->flags |= DOMAIN_FLAG_LOSE_CHILDREN;
domain_add_dev_info(si_domain, dev);
dev_info(dev,
"Device uses a private identity domain.\n");
}
}
and now we're doing the bad stuff that Lu pointed out: we only have one
mapping shared for all the subdevices, so if we end up changing it for one
subdevice, we're likely to be breaking another.
In this case iommu_request_dm_for_dev() returns -EBUSY without doing anything
and the following private identity code fortunately seems to have no
consequential effects - the real DMA device continues to operate in the DMA
domain, and all subdevice DMA requests go through the DMA mapping codepath.
That's probably why VMD appears to be working fine anyway, but this seems
fragile.
The following changes enforce that the real DMA device is in the DMA domain,
and avoid the intel_iommu_add_device() codepaths that would try to change
it to a different domain type. Let me know if I'm on the right lines...
---
drivers/iommu/intel-iommu.c | 16 ++++++++++++++++
drivers/pci/controller/intel-nvme-remap.c | 6 ++++++
2 files changed, 22 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9644a5b3e0ae..8872b8d1780d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2911,6 +2911,9 @@ static int device_def_domain_type(struct device *dev)
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
+ if (pci_real_dma_dev(pdev) != pdev)
+ return IOMMU_DOMAIN_DMA;
+
if (device_is_rmrr_locked(dev))
return IOMMU_DOMAIN_DMA;
@@ -5580,6 +5583,7 @@ static bool intel_iommu_capable(enum iommu_cap cap)
static int intel_iommu_add_device(struct device *dev)
{
+ struct device *real_dev = dev;
struct dmar_domain *dmar_domain;
struct iommu_domain *domain;
struct intel_iommu *iommu;
@@ -5591,6 +5595,17 @@ static int intel_iommu_add_device(struct device *dev)
if (!iommu)
return -ENODEV;
+ if (dev_is_pci(dev))
+ real_dev = &pci_real_dma_dev(to_pci_dev(dev))->dev;
+
+ if (real_dev != dev) {
+ domain = iommu_get_domain_for_dev(real_dev);
+ if (domain->type != IOMMU_DOMAIN_DMA) {
+ dev_err(dev, "Real DMA device not in DMA domain; can't
handle DMA\n");
+ return -ENODEV;
+ }
+ }
+
iommu_device_link(&iommu->iommu, dev);
if (translation_pre_enabled(iommu))
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
