Re: [PATCH RFC v3 02/12] iommu: Add a flag to indicate immutable singleton group
From: Lu Baolu
Date: Tue Apr 12 2022 - 01:09:07 EST
Hi Kevin,
Thanks for your time.
On 2022/4/12 11:15, Tian, Kevin wrote:
From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
Sent: Sunday, April 10, 2022 6:25 PM
Some features require that a single device must be immutably isolated,
even when hot plug is supported.
This reads confusing, as hotplug cannot be allowed in a singleton group.
What you actually meant suppose to be:
"Enabling certain device features require the device in a singleton iommu
group which is immutable in fabric i.e. not affected by hotplug"
Yours is better. Thank you.
For example, the SVA bind()/unbind()
interfaces require that the device exists in a singleton group. If we
have a singleton group that doesn't have ACS (or similar technologies)
and someone hotplugs in another device on a bridge, then our SVA is
completely broken and we get data corruption.
this needs the background that PASID doesn't join PCI packet routing
thus the dma address (CPU VA) may hit a p2p range.
Sure.
This adds a flag in the iommu_group struct to indicate an immutable
singleton group, and uses standard PCI bus topology, isolation features,
and DMA alias quirks to set the flag. If the device came from DT, assume
it is static and then the singleton attribute can know from the device
count in the group.
where does the assumption come from?
Hotplug is the only factor that can dynamically affect the
characteristics of IOMMU group singleton as far as I can see. If a
device node was created from the DT, it could be treated as static,
hence we can judge the singleton in iommu probe phase during boot.
Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Suggested-by: Kevin Tian <kevin.tian@xxxxxxxxx>
Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
---
drivers/iommu/iommu.c | 67 ++++++++++++++++++++++++++++++++++++----
---
1 file changed, 57 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0c42ece25854..56ffbf5fdc18 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -48,6 +48,7 @@ struct iommu_group {
struct list_head entry;
unsigned int owner_cnt;
void *owner;
+ bool immutable_singleton;
Just call it 'singleton' with a comment to explain it must be immutable?
I was just trying to distinguish "singleton" and "immutable singleton".
A group is singleton if it only contains a single device in the device
list, while a group is immutable singleton only if the fabric doesn't
allow to change the state.
};
struct group_device {
@@ -74,6 +75,16 @@ static const char * const
iommu_group_resv_type_string[] = {
#define IOMMU_CMD_LINE_DMA_API BIT(0)
#define IOMMU_CMD_LINE_STRICT BIT(1)
+/*
+ * To consider a PCI device isolated, we require ACS to support Source
+ * Validation, Request Redirection, Completer Redirection, and Upstream
+ * Forwarding. This effectively means that devices cannot spoof their
+ * requester ID, requests and completions cannot be redirected, and all
+ * transactions are forwarded upstream, even as it passes through a
+ * bridge where the target device is downstream.
+ */
+#define REQ_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |
PCI_ACS_UF)
+
static int iommu_alloc_default_domain(struct iommu_group *group,
struct device *dev);
static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
@@ -89,6 +100,7 @@ static int
iommu_create_device_direct_mappings(struct iommu_group *group,
static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
static ssize_t iommu_group_store_type(struct iommu_group *group,
const char *buf, size_t count);
+static int iommu_group_device_count(struct iommu_group *group);
#define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)
\
struct iommu_group_attribute iommu_group_attr_##_name = \
@@ -844,6 +856,37 @@ static bool iommu_is_attach_deferred(struct device
*dev)
return false;
}
+static int has_pci_alias(struct pci_dev *pdev, u16 alias, void *opaque)
+{
+ return -EEXIST;
+}
+
+static bool pci_immutably_isolated(struct pci_dev *pdev)
+{
+ /* Skip the bridges. */
+ if (pci_is_bridge(pdev))
+ return false;
+
+ /*
+ * The device could be considered to be fully isolated if
+ * all devices on the path from the parent to the host-PCI
+ * bridge are protected from peer-to-peer DMA by ACS.
+ */
+ if (!pci_is_root_bus(pdev->bus) &&
+ !pci_acs_path_enabled(pdev->bus->self, NULL, REQ_ACS_FLAGS))
+ return false;
+
+ /* Multi-function devices should have ACS enabled. */
+ if (pdev->multifunction && !pci_acs_enabled(pdev, REQ_ACS_FLAGS))
+ return false;
Looks my earlier comment was lost, i.e. you can just use
pci_acs_path_enabled(pdev) to cover above two checks.
If a device is directly connected to the root bridge and it is not an
MFD, do we still need ACS on it? The Intel idxd device seems to be such
a device. I had a quick check with lspci, it has no ACS support.
I probably missed anything.
+
+ /* Filter out devices which has any alias device. */
+ if (pci_for_each_dma_alias(pdev, has_pci_alias, NULL))
+ return false;
+
+ return true;
+}
+
/**
* iommu_group_add_device - add a device to an iommu group
* @group: the group into which to add the device (reference should be held)
@@ -898,6 +941,20 @@ int iommu_group_add_device(struct iommu_group
*group, struct device *dev)
list_add_tail(&device->list, &group->devices);
if (group->domain && !iommu_is_attach_deferred(dev))
ret = __iommu_attach_device(group->domain, dev);
+
+ /*
+ * Use standard PCI bus topology, isolation features, and DMA
+ * alias quirks to set the immutable singleton attribute. If
+ * the device came from DT, assume it is static and then
+ * singleton can know from the device count in the group.
+ */
+ if (dev_is_pci(dev))
+ group->immutable_singleton =
+ pci_immutably_isolated(to_pci_dev(dev));
+ else if (is_of_node(dev_fwnode(dev)))
+ group->immutable_singleton =
+ (iommu_group_device_count(group) == 1);
+
mutex_unlock(&group->mutex);
if (ret)
goto err_put_group;
@@ -1290,16 +1347,6 @@ EXPORT_SYMBOL_GPL(iommu_group_id);
static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
unsigned long *devfns);
-/*
- * To consider a PCI device isolated, we require ACS to support Source
- * Validation, Request Redirection, Completer Redirection, and Upstream
- * Forwarding. This effectively means that devices cannot spoof their
- * requester ID, requests and completions cannot be redirected, and all
- * transactions are forwarded upstream, even as it passes through a
- * bridge where the target device is downstream.
- */
-#define REQ_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |
PCI_ACS_UF)
-
/*
* For multifunction devices which are not isolated from each other, find
* all the other non-isolated functions and look for existing groups. For
--
2.25.1
Best regards,
baolu