Re: 3.6-rc7 boot crash + bisection

[Alex Williamson]

On Wed, 2012-09-26 at 10:21 -0600, Alex Williamson wrote:
> On Wed, 2012-09-26 at 17:10 +0200, Roedel, Joerg wrote:
> > On Wed, Sep 26, 2012 at 08:35:59AM -0600, Alex Williamson wrote:
> > > Hmm, that throws a kink in iommu groups.  So perhaps we need to make an
> > > alias interface to iommu groups.  Seems like this could just be an extra
> > > parameter to iommu_group_get and iommu_group_add_device (empty in the
> > > typical case).  Then we have the problem of what's the type for an
> > > alias?  For AMI-Vi, it's a u16, but we need to be more generic than
> > > that.  Maybe iommu groups should just treat it as a void* so iommus can
> > > use a pointer to some structure or a fixed value like a u16 bus:slot.
> > > Thoughts?
> > 
> > Good question. The iommu-groups are part of the IOMMU-API, with an
> > interface to the IOMMU drivers and one to the users of IOMMU-API. So the
> > alias handling itself should be a function of the interface to the IOMMU
> > driver. In general the interface should not be bus specific.
> > 
> > So a void pointer seems the only logical choice then. But I would not
> > limit its scope to alias handling. How about making it a bus-private
> > pointer where IOMMU driver store bus-specific information. That way we
> > make sure that there is one struct per bus-type for this pointer, and
> > not one structure per IOMMU driver.
> 
> I thought of another approach that may actually be more 3.6 worthy.
> What if we just make the iommu driver handle it?  For instance,
> amd_iommu can walk the alias table looking for entries that use the same
> alias and get the device via pci_get_bus_and_slot.  If it finds a device
> with an iommu group, it attaches the new device to the same group,
> hiding anything about aliases from the group layer.  It just groups all
> devices within the range.  I think the only complication is making sure
> we're safe around device hotplug while we're doing this.  Thanks,

I think this could work.  Instead of searching for other devices, check
for or allocate an iommu group on the alias dev_data, any "virtual"
aliases use that iommu group.  Florian, could you test this as well?
Thanks,

Alex

Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
---

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index b64502d..22879ed 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -126,6 +126,8 @@ static void free_dev_data(struct iommu_dev_data *dev_data)
 
 	spin_lock_irqsave(&dev_data_list_lock, flags);
 	list_del(&dev_data->dev_data_list);
+	if (dev_data->group)
+		iommu_group_put(dev_data->group);
 	spin_unlock_irqrestore(&dev_data_list_lock, flags);
 
 	kfree(dev_data);
@@ -256,6 +258,37 @@ static bool check_device(struct device *dev)
 	return true;
 }
 
+/*
+ * Sometimes there's no actual device for an alias.  When that happens
+ * we allocate an iommu group on the iommu_dev_data so that it gets used
+ * by anything with the same alias.  We keep the reference from
+ * iommu_group_alloc so the group persists with the iommu_dev_data.
+ */
+static int dev_data_add_iommu_group(struct iommu_dev_data *dev_data,
+				    struct device *dev)
+{
+	unsigned long flags;
+	struct iommu_group *group;
+	int ret = 0;
+
+	spin_lock_irqsave(&dev_data_list_lock, flags);
+	if (!dev_data->group) {
+		group = iommu_group_alloc();
+		if (IS_ERR(group)) {
+			ret = PTR_ERR(group);
+			goto unlock;
+		}
+
+		dev_data->group = group;
+	} else
+		group = dev_data->group;
+
+	ret = iommu_group_add_device(group, dev);
+unlock:
+	spin_unlock_irqrestore(&dev_data_list_lock, flags);
+	return ret;
+}
+
 static void swap_pci_ref(struct pci_dev **from, struct pci_dev *to)
 {
 	pci_dev_put(*from);
@@ -264,38 +297,12 @@ static void swap_pci_ref(struct pci_dev **from, struct pci_dev *to)
 
 #define REQ_ACS_FLAGS	(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
 
-static int iommu_init_device(struct device *dev)
+static int pdev_add_iommu_group(struct pci_dev *pdev, struct device *dev)
 {
-	struct pci_dev *dma_pdev, *pdev = to_pci_dev(dev);
-	struct iommu_dev_data *dev_data;
+	struct pci_dev *dma_pdev = pdev;
 	struct iommu_group *group;
-	u16 alias;
 	int ret;
 
-	if (dev->archdata.iommu)
-		return 0;
-
-	dev_data = find_dev_data(get_device_id(dev));
-	if (!dev_data)
-		return -ENOMEM;
-
-	alias = amd_iommu_alias_table[dev_data->devid];
-	if (alias != dev_data->devid) {
-		struct iommu_dev_data *alias_data;
-
-		alias_data = find_dev_data(alias);
-		if (alias_data == NULL) {
-			pr_err("AMD-Vi: Warning: Unhandled device %s\n",
-					dev_name(dev));
-			free_dev_data(dev_data);
-			return -ENOTSUPP;
-		}
-		dev_data->alias_data = alias_data;
-
-		dma_pdev = pci_get_bus_and_slot(alias >> 8, alias & 0xff);
-	} else
-		dma_pdev = pci_dev_get(pdev);
-
 	/* Account for quirked devices */
 	swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
 
@@ -344,8 +351,61 @@ root_bus:
 
 	iommu_group_put(group);
 
-	if (ret)
-		return ret;
+	return ret;
+}
+
+static int iommu_init_device(struct device *dev)
+{
+	struct pci_dev *dma_pdev, *pdev = to_pci_dev(dev);
+	struct iommu_dev_data *dev_data;
+	u16 alias;
+	int ret;
+
+	if (dev->archdata.iommu)
+		return 0;
+
+	dev_data = find_dev_data(get_device_id(dev));
+	if (!dev_data)
+		return -ENOMEM;
+
+	alias = amd_iommu_alias_table[dev_data->devid];
+	if (alias != dev_data->devid) {
+		struct iommu_dev_data *alias_data;
+
+		alias_data = find_dev_data(alias);
+		if (alias_data == NULL) {
+			pr_err("AMD-Vi: Warning: Unhandled device %s\n",
+					dev_name(dev));
+			free_dev_data(dev_data);
+			return -ENOTSUPP;
+		}
+		dev_data->alias_data = alias_data;
+
+		/*
+		 * If the alias device exists, use it as the base dma
+		 * device.  This results in all devices aliasing to this
+		 * one to be in the same iommu group.  If it doesn't
+		 * actually exist, store the iommu group on the alias
+		 * dev_data and use that for all aliases.
+		 */
+		dma_pdev = pci_get_bus_and_slot(alias >> 8, alias & 0xff);
+		if (!dma_pdev) {
+			ret = dev_data_add_iommu_group(alias_data, dev);
+			if (ret) {
+				free_dev_data(dev_data);
+				return ret;
+			}
+		}
+	} else
+		dma_pdev = pci_dev_get(pdev);
+
+	if (dma_pdev) {
+		ret = pdev_add_iommu_group(dma_pdev, dev);
+		if (ret) {
+			free_dev_data(dev_data);
+			return ret;
+		}
+	}
 
 	if (pci_iommuv2_capable(pdev)) {
 		struct amd_iommu *iommu;
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index d0dab86..6597d6a 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -404,6 +404,7 @@ struct iommu_dev_data {
 	struct list_head dev_data_list;	  /* For global dev_data_list */
 	struct iommu_dev_data *alias_data;/* The alias dev_data */
 	struct protection_domain *domain; /* Domain the device is bound to */
+	struct iommu_group *group;	  /* IOMMU group for virtual aliases */
 	atomic_t bind;			  /* Domain attach reverent count */
 	u16 devid;			  /* PCI Device ID */
 	bool iommu_v2;			  /* Device can make use of IOMMUv2 */


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/