On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote:
> From: Sai Praneeth Prakhya <[email protected]>
>
> Presently, the default domain of an iommu group is allocated during boot
> time and it cannot be changed later. So, the device would typically be
> either in identity (also known as pass_through) mode or the device would be
> in DMA mode as long as the machine is up and running. There is no way to
> change the default domain type dynamically i.e. after booting, a device
> cannot switch between identity mode and DMA mode.
>
> But, assume a use case wherein the user trusts the device and believes that
> the OS is secure enough and hence wants *only* this device to bypass IOMMU
> (so that it could be high performing) whereas all the other devices to go
> through IOMMU (so that the system is protected). Presently, this use case
> is not supported. It will be helpful if there is some way to change the
> default domain of an iommu group dynamically. Hence, add such support.
>
> A privileged user could request the kernel to change the default domain
> type of a iommu group by writing to
> "/sys/kernel/iommu_groups/<grp_id>/type" file. Presently, only three values
> are supported
> 1. identity: all the DMA transactions from the device in this group are
> *not* translated by the iommu
> 2. DMA: all the DMA transactions from the device in this group are
> translated by the iommu
> 3. auto: change to the type the device was booted with
>
> Note:
> 1. Default domain of an iommu group with two or more devices cannot be
> changed.
> 2. The device in the iommu group shouldn't be bound to any driver.
> 3. The device shouldn't be assigned to user for direct access.
> 4. The vendor iommu driver is required to add def_domain_type() callback.
> The change request will fail if the request type conflicts with that
> returned from the callback.
>
> Please see "Documentation/ABI/testing/sysfs-kernel-iommu_groups" for more
> information.
>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Ashok Raj <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Lu Baolu <[email protected]>
> Cc: Sohil Mehta <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Jacob Pan <[email protected]>
> Reviewed-by: Lu Baolu <[email protected]>
> Signed-off-by: Sai Praneeth Prakhya <[email protected]>
> ---
> drivers/iommu/iommu.c | 225
> +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 224 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6c14c88cd525..2e93c48ce248 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -93,6 +93,8 @@ static void __iommu_detach_group(struct iommu_domain
> *domain,
> static int iommu_create_device_direct_mappings(struct iommu_group *group,
> struct device *dev);
> 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);
>
> #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store) \
> struct iommu_group_attribute iommu_group_attr_##_name = \
> @@ -525,7 +527,8 @@ static IOMMU_GROUP_ATTR(name, S_IRUGO,
> iommu_group_show_name, NULL);
> static IOMMU_GROUP_ATTR(reserved_regions, 0444,
> iommu_group_show_resv_regions, NULL);
>
> -static IOMMU_GROUP_ATTR(type, 0444, iommu_group_show_type, NULL);
> +static IOMMU_GROUP_ATTR(type, 0644, iommu_group_show_type,
> + iommu_group_store_type);
>
> static void iommu_group_release(struct kobject *kobj)
> {
> @@ -2849,3 +2852,223 @@ int iommu_sva_get_pasid(struct iommu_sva *handle)
> return ops->sva_get_pasid(handle);
> }
> EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
> +
> +/*
> + * Changes the default domain of an iommu group that has *only* one device
> + *
> + * @group: The group for which the default domain should be changed
> + * @prev_dev: The device in the group (this is used to make sure that the
> device
> + * hasn't changed after the caller has called this function)
> + * @type: The type of the new default domain that gets associated with the
> group
> + *
> + * Returns 0 on success and error code on failure
> + *
> + * Note:
> + * 1. Presently, this function is called only when user requests to change
> the
> + * group's default domain type through
> /sys/kernel/iommu_groups/<grp_id>/type
> + * Please take a closer look if intended to use for other purposes.
> + */
> +static int iommu_change_dev_def_domain(struct iommu_group *group,
> + struct device *prev_dev, int type)
> +{
> + struct iommu_domain *prev_dom;
> + struct group_device *grp_dev;
> + const struct iommu_ops *ops;
> + int ret, dev_def_dom;
> + struct device *dev;
> +
> + if (!group)
> + return -EINVAL;
> +
> + mutex_lock(&group->mutex);
> +
> + if (group->default_domain != group->domain) {
> + pr_err_ratelimited("Group not assigned to default domain\n");
This error is lacking any context. Can we use dev_err_ratelimited to make
it a bit more useful? Same for the other errors you're adding in this
function.
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + /*
> + * iommu group wasn't locked while acquiring device lock in
> + * iommu_group_store_type(). So, make sure that the device count hasn't
> + * changed while acquiring device lock.
> + *
> + * Changing default domain of an iommu group with two or more devices
> + * isn't supported because there could be a potential deadlock. Consider
> + * the following scenario. T1 is trying to acquire device locks of all
> + * the devices in the group and before it could acquire all of them,
> + * there could be another thread T2 (from different sub-system and use
> + * case) that has already acquired some of the device locks and might be
> + * waiting for T1 to release other device locks.
> + */
> + if (iommu_group_device_count(group) != 1) {
> + pr_err_ratelimited("Cannot change default domain: Group has
> more than one device\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* Since group has only one device */
> + grp_dev = list_first_entry(&group->devices, struct group_device, list);
> + dev = grp_dev->dev;
> +
> + if (prev_dev != dev) {
> + pr_err_ratelimited("Cannot change default domain: Device has
> been changed\n");
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + prev_dom = group->default_domain;
> + if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ops = prev_dom->ops;
> + dev_def_dom = ops->def_domain_type(dev);
> +
> + /* Check if user requested domain is supported by the device or not */
> + if (!type) {
> + /*
> + * If the user hasn't requested any specific type of domain and
> + * if the device supports both the domains, then default to the
> + * domain the device was booted with
> + */
> + type = dev_def_dom ?: iommu_def_domain_type;
> + } else if (dev_def_dom && type != dev_def_dom) {
> + pr_err_ratelimited("Device cannot be in %s domain\n",
> + iommu_domain_type_str(type));
> + ret = -EINVAL;
> + goto out;
> + }
Can we replace this with iommu_get_def_domain_type() ?
> +
> + /*
> + * Switch to a new domain only if the requested domain type is different
> + * from the existing default domain type
> + */
> + if (prev_dom->type == type) {
> + ret = 0;
> + goto out;
> + }
> +
> + /* Sets group->default_domain to the newly allocated domain */
> + ret = iommu_group_alloc_default_domain(dev->bus, group, type);
> + if (ret)
> + goto out;
> +
> + ret = iommu_create_device_direct_mappings(group, dev);
> + if (ret)
> + goto free_new_domain;
> +
> + ret = __iommu_attach_device(group->default_domain, dev);
> + if (ret)
> + goto free_new_domain;
> +
> + group->domain = group->default_domain;
> +
> + /*
> + * Release the mutex here because ops->probe_finalize() call-back of
> + * some vendor IOMMU drivers calls arm_iommu_attach_device() which
> + * in-turn might call back into IOMMU core code, where it tries to take
> + * group->mutex, resulting in a deadlock.
> + */
> + mutex_unlock(&group->mutex);
> +
> + /* Make sure dma_ops is appropriatley set */
> + iommu_group_do_probe_finalize(dev, group->default_domain);
> + iommu_domain_free(prev_dom);
> + return 0;
> +
> +free_new_domain:
> + iommu_domain_free(group->default_domain);
> + group->default_domain = prev_dom;
> + group->domain = prev_dom;i
Hmm. This seems to rely on all users of group->default_domain holding the
group->mutex. Have you confirmed that this is the case? There's a funny
use of iommu_group_get() in the exynos IOMMU driver at least.
Will
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
