Re: [PATCH RFC v3 02/12] iommu: Add a flag to indicate immutable singleton group

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi Kevin,

Thanks for your time.

On 2022/4/12 11:15, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> 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@nvidia.com>
>> Suggested-by: Kevin Tian <kevin.tian@intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu