2023-07-12 16:58:44

by Jacob Pan

[permalink] [raw]
Subject: [PATCH v10 6/7] iommu/vt-d: Add set_dev_pasid callback for dma domain

From: Lu Baolu <[email protected]>

This allows the upper layers to set a domain to a PASID of a device
if the PASID feature is supported by the IOMMU hardware. The typical
use cases are, for example, kernel DMA with PASID and hardware
assisted mediated device drivers.

The attaching device and pasid information is tracked in a per-domain
list and is used for IOTLB and devTLB invalidation.

Signed-off-by: Lu Baolu <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
---
drivers/iommu/intel/iommu.c | 103 ++++++++++++++++++++++++++++++++++--
drivers/iommu/intel/iommu.h | 7 +++
2 files changed, 105 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 4b54a56831b4..f710e724c51a 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1359,6 +1359,7 @@ domain_lookup_dev_info(struct dmar_domain *domain,

static void domain_update_iotlb(struct dmar_domain *domain)
{
+ struct dev_pasid_info *dev_pasid;
struct device_domain_info *info;
bool has_iotlb_device = false;
unsigned long flags;
@@ -1370,6 +1371,14 @@ static void domain_update_iotlb(struct dmar_domain *domain)
break;
}
}
+
+ list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) {
+ info = dev_iommu_priv_get(dev_pasid->dev);
+ if (info->ats_enabled) {
+ has_iotlb_device = true;
+ break;
+ }
+ }
domain->has_iotlb_device = has_iotlb_device;
spin_unlock_irqrestore(&domain->lock, flags);
}
@@ -1455,6 +1464,7 @@ static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
u64 addr, unsigned mask)
{
+ struct dev_pasid_info *dev_pasid;
struct device_domain_info *info;
unsigned long flags;

@@ -1464,6 +1474,19 @@ static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
spin_lock_irqsave(&domain->lock, flags);
list_for_each_entry(info, &domain->devices, link)
__iommu_flush_dev_iotlb(info, addr, mask);
+
+ list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) {
+ info = dev_iommu_priv_get(dev_pasid->dev);
+
+ if (!info->ats_enabled)
+ continue;
+
+ qi_flush_dev_iotlb_pasid(info->iommu,
+ PCI_DEVID(info->bus, info->devfn),
+ info->pfsid, dev_pasid->pasid,
+ info->ats_qdep, addr,
+ mask);
+ }
spin_unlock_irqrestore(&domain->lock, flags);
}

@@ -1477,9 +1500,13 @@ static void domain_flush_pasid_iotlb(struct intel_iommu *iommu,
unsigned long npages, bool ih)
{
u16 did = domain_id_iommu(domain, iommu);
+ struct dev_pasid_info *dev_pasid;
unsigned long flags;

spin_lock_irqsave(&domain->lock, flags);
+ list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain)
+ qi_flush_piotlb(iommu, did, dev_pasid->pasid, addr, npages, ih);
+
if (!list_empty(&domain->devices))
qi_flush_piotlb(iommu, did, IOMMU_NO_PASID, addr, npages, ih);
spin_unlock_irqrestore(&domain->lock, flags);
@@ -1744,6 +1771,7 @@ static struct dmar_domain *alloc_domain(unsigned int type)
domain->use_first_level = true;
domain->has_iotlb_device = false;
INIT_LIST_HEAD(&domain->devices);
+ INIT_LIST_HEAD(&domain->dev_pasids);
spin_lock_init(&domain->lock);
xa_init(&domain->iommu_array);

@@ -4731,7 +4759,10 @@ static void intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
{
struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
+ struct dev_pasid_info *curr, *dev_pasid = NULL;
+ struct dmar_domain *dmar_domain;
struct iommu_domain *domain;
+ unsigned long flags;

domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
if (!domain)
@@ -4747,17 +4778,78 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
goto out_tear_down;
}

- /*
- * Should never reach here until we add support for attaching
- * non-SVA domain to a pasid.
- */
- WARN_ON(1);
+ dmar_domain = to_dmar_domain(domain);
+ spin_lock_irqsave(&dmar_domain->lock, flags);
+ list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
+ if (curr->dev == dev && curr->pasid == pasid) {
+ list_del(&curr->link_domain);
+ dev_pasid = curr;
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&dmar_domain->lock, flags);

+ domain_detach_iommu(dmar_domain, iommu);
+ kfree(dev_pasid);
out_tear_down:
intel_pasid_tear_down_entry(iommu, dev, pasid, false);
intel_drain_pasid_prq(dev, pasid);
}

+static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+ struct intel_iommu *iommu = info->iommu;
+ struct dev_pasid_info *dev_pasid;
+ unsigned long flags;
+ int ret;
+
+ if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
+ return -EOPNOTSUPP;
+
+ if (context_copied(iommu, info->bus, info->devfn))
+ return -EBUSY;
+
+ ret = prepare_domain_attach_device(domain, dev);
+ if (ret)
+ return ret;
+
+ dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
+ if (!dev_pasid)
+ return -ENOMEM;
+
+ ret = domain_attach_iommu(dmar_domain, iommu);
+ if (ret)
+ goto out_free;
+
+ if (domain_type_is_si(dmar_domain))
+ ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
+ dev, pasid);
+ else if (dmar_domain->use_first_level)
+ ret = domain_setup_first_level(iommu, dmar_domain,
+ dev, pasid);
+ else
+ ret = intel_pasid_setup_second_level(iommu, dmar_domain,
+ dev, pasid);
+ if (ret)
+ goto out_detach_iommu;
+
+ dev_pasid->dev = dev;
+ dev_pasid->pasid = pasid;
+ spin_lock_irqsave(&dmar_domain->lock, flags);
+ list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
+ spin_unlock_irqrestore(&dmar_domain->lock, flags);
+
+ return 0;
+out_detach_iommu:
+ domain_detach_iommu(dmar_domain, iommu);
+out_free:
+ kfree(dev_pasid);
+ return ret;
+}
+
const struct iommu_ops intel_iommu_ops = {
.capable = intel_iommu_capable,
.domain_alloc = intel_iommu_domain_alloc,
@@ -4777,6 +4869,7 @@ const struct iommu_ops intel_iommu_ops = {
#endif
.default_domain_ops = &(const struct iommu_domain_ops) {
.attach_dev = intel_iommu_attach_device,
+ .set_dev_pasid = intel_iommu_set_dev_pasid,
.map_pages = intel_iommu_map_pages,
.unmap_pages = intel_iommu_unmap_pages,
.iotlb_sync_map = intel_iommu_iotlb_sync_map,
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 6d94a29f5d52..68bb7cdf5543 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -595,6 +595,7 @@ struct dmar_domain {

spinlock_t lock; /* Protect device tracking lists */
struct list_head devices; /* all devices' list */
+ struct list_head dev_pasids; /* all attached pasids */

struct dma_pte *pgd; /* virtual address */
int gaw; /* max guest address width */
@@ -717,6 +718,12 @@ struct device_domain_info {
struct pasid_table *pasid_table; /* pasid table */
};

+struct dev_pasid_info {
+ struct list_head link_domain; /* link to domain siblings */
+ struct device *dev; /* the physical device */
+ ioasid_t pasid; /* PASID of the physical device */
+};
+
static inline void __iommu_flush_cache(
struct intel_iommu *iommu, void *addr, int size)
{
--
2.25.1



2023-07-13 08:14:53

by Tian, Kevin

[permalink] [raw]
Subject: RE: [PATCH v10 6/7] iommu/vt-d: Add set_dev_pasid callback for dma domain

> From: Jacob Pan <[email protected]>
> Sent: Thursday, July 13, 2023 12:34 AM
>
>
> - /*
> - * Should never reach here until we add support for attaching
> - * non-SVA domain to a pasid.
> - */
> - WARN_ON(1);
> + dmar_domain = to_dmar_domain(domain);
> + spin_lock_irqsave(&dmar_domain->lock, flags);
> + list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain)
> {
> + if (curr->dev == dev && curr->pasid == pasid) {
> + list_del(&curr->link_domain);
> + dev_pasid = curr;
> + break;
> + }
> + }
> + spin_unlock_irqrestore(&dmar_domain->lock, flags);
>

what about no matching dev_pasid is find?

> + domain_detach_iommu(dmar_domain, iommu);
> + kfree(dev_pasid);
> out_tear_down:
> intel_pasid_tear_down_entry(iommu, dev, pasid, false);
> intel_drain_pasid_prq(dev, pasid);
> }
>
> +static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + struct device_domain_info *info = dev_iommu_priv_get(dev);
> + struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> + struct intel_iommu *iommu = info->iommu;
> + struct dev_pasid_info *dev_pasid;
> + unsigned long flags;
> + int ret;
> +
> + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
> + return -EOPNOTSUPP;
> +
> + if (context_copied(iommu, info->bus, info->devfn))
> + return -EBUSY;
> +
> + ret = prepare_domain_attach_device(domain, dev);
> + if (ret)
> + return ret;
> +
> + dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
> + if (!dev_pasid)
> + return -ENOMEM;
> +
> + ret = domain_attach_iommu(dmar_domain, iommu);
> + if (ret)
> + goto out_free;
> +
> + if (domain_type_is_si(dmar_domain))
> + ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
> + dev, pasid);
> + else if (dmar_domain->use_first_level)
> + ret = domain_setup_first_level(iommu, dmar_domain,
> + dev, pasid);
> + else
> + ret = intel_pasid_setup_second_level(iommu, dmar_domain,
> + dev, pasid);
> + if (ret)
> + goto out_detach_iommu;
> +
> + dev_pasid->dev = dev;
> + dev_pasid->pasid = pasid;
> + spin_lock_irqsave(&dmar_domain->lock, flags);
> + list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
> + spin_unlock_irqrestore(&dmar_domain->lock, flags);
> +
> + return 0;
> +out_detach_iommu:
> + domain_detach_iommu(dmar_domain, iommu);
> +out_free:
> + kfree(dev_pasid);
> + return ret;
> +}
> +
> const struct iommu_ops intel_iommu_ops = {
> .capable = intel_iommu_capable,
> .domain_alloc = intel_iommu_domain_alloc,
> @@ -4777,6 +4869,7 @@ const struct iommu_ops intel_iommu_ops = {
> #endif
> .default_domain_ops = &(const struct iommu_domain_ops) {
> .attach_dev = intel_iommu_attach_device,
> + .set_dev_pasid = intel_iommu_set_dev_pasid,
> .map_pages = intel_iommu_map_pages,
> .unmap_pages = intel_iommu_unmap_pages,
> .iotlb_sync_map =
> intel_iommu_iotlb_sync_map,
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 6d94a29f5d52..68bb7cdf5543 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -595,6 +595,7 @@ struct dmar_domain {
>
> spinlock_t lock; /* Protect device tracking lists */
> struct list_head devices; /* all devices' list */
> + struct list_head dev_pasids; /* all attached pasids */
>
> struct dma_pte *pgd; /* virtual address */
> int gaw; /* max guest address width */
> @@ -717,6 +718,12 @@ struct device_domain_info {
> struct pasid_table *pasid_table; /* pasid table */
> };
>
> +struct dev_pasid_info {
> + struct list_head link_domain; /* link to domain siblings */
> + struct device *dev; /* the physical device */
> + ioasid_t pasid; /* PASID of the physical device */
> +};

the comment for dev/pasid is meaningless.

2023-07-14 03:50:04

by Lu Baolu

[permalink] [raw]
Subject: Re: [PATCH v10 6/7] iommu/vt-d: Add set_dev_pasid callback for dma domain

On 2023/7/13 15:56, Tian, Kevin wrote:
>> From: Jacob Pan <[email protected]>
>> Sent: Thursday, July 13, 2023 12:34 AM
>>
>>
>> - /*
>> - * Should never reach here until we add support for attaching
>> - * non-SVA domain to a pasid.
>> - */
>> - WARN_ON(1);
>> + dmar_domain = to_dmar_domain(domain);
>> + spin_lock_irqsave(&dmar_domain->lock, flags);
>> + list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain)
>> {
>> + if (curr->dev == dev && curr->pasid == pasid) {
>> + list_del(&curr->link_domain);
>> + dev_pasid = curr;
>> + break;
>> + }
>> + }
>> + spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>
>
> what about no matching dev_pasid is find?

kfree() can handle this gracefully.

>
>> + domain_detach_iommu(dmar_domain, iommu);
>> + kfree(dev_pasid);
>> out_tear_down:
>> intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>> intel_drain_pasid_prq(dev, pasid);
>> }
>>
>> +static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t pasid)
>> +{
>> + struct device_domain_info *info = dev_iommu_priv_get(dev);
>> + struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> + struct intel_iommu *iommu = info->iommu;
>> + struct dev_pasid_info *dev_pasid;
>> + unsigned long flags;
>> + int ret;
>> +
>> + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
>> + return -EOPNOTSUPP;
>> +
>> + if (context_copied(iommu, info->bus, info->devfn))
>> + return -EBUSY;
>> +
>> + ret = prepare_domain_attach_device(domain, dev);
>> + if (ret)
>> + return ret;
>> +
>> + dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
>> + if (!dev_pasid)
>> + return -ENOMEM;
>> +
>> + ret = domain_attach_iommu(dmar_domain, iommu);
>> + if (ret)
>> + goto out_free;
>> +
>> + if (domain_type_is_si(dmar_domain))
>> + ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
>> + dev, pasid);
>> + else if (dmar_domain->use_first_level)
>> + ret = domain_setup_first_level(iommu, dmar_domain,
>> + dev, pasid);
>> + else
>> + ret = intel_pasid_setup_second_level(iommu, dmar_domain,
>> + dev, pasid);
>> + if (ret)
>> + goto out_detach_iommu;
>> +
>> + dev_pasid->dev = dev;
>> + dev_pasid->pasid = pasid;
>> + spin_lock_irqsave(&dmar_domain->lock, flags);
>> + list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
>> + spin_unlock_irqrestore(&dmar_domain->lock, flags);
>> +
>> + return 0;
>> +out_detach_iommu:
>> + domain_detach_iommu(dmar_domain, iommu);
>> +out_free:
>> + kfree(dev_pasid);
>> + return ret;
>> +}
>> +
>> const struct iommu_ops intel_iommu_ops = {
>> .capable = intel_iommu_capable,
>> .domain_alloc = intel_iommu_domain_alloc,
>> @@ -4777,6 +4869,7 @@ const struct iommu_ops intel_iommu_ops = {
>> #endif
>> .default_domain_ops = &(const struct iommu_domain_ops) {
>> .attach_dev = intel_iommu_attach_device,
>> + .set_dev_pasid = intel_iommu_set_dev_pasid,
>> .map_pages = intel_iommu_map_pages,
>> .unmap_pages = intel_iommu_unmap_pages,
>> .iotlb_sync_map =
>> intel_iommu_iotlb_sync_map,
>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>> index 6d94a29f5d52..68bb7cdf5543 100644
>> --- a/drivers/iommu/intel/iommu.h
>> +++ b/drivers/iommu/intel/iommu.h
>> @@ -595,6 +595,7 @@ struct dmar_domain {
>>
>> spinlock_t lock; /* Protect device tracking lists */
>> struct list_head devices; /* all devices' list */
>> + struct list_head dev_pasids; /* all attached pasids */
>>
>> struct dma_pte *pgd; /* virtual address */
>> int gaw; /* max guest address width */
>> @@ -717,6 +718,12 @@ struct device_domain_info {
>> struct pasid_table *pasid_table; /* pasid table */
>> };
>>
>> +struct dev_pasid_info {
>> + struct list_head link_domain; /* link to domain siblings */
>> + struct device *dev; /* the physical device */
>> + ioasid_t pasid; /* PASID of the physical device */
>> +};
>
> the comment for dev/pasid is meaningless.

Then we can remove them.

Best regards,
baolu

2023-07-14 04:33:50

by Tian, Kevin

[permalink] [raw]
Subject: RE: [PATCH v10 6/7] iommu/vt-d: Add set_dev_pasid callback for dma domain

> From: Baolu Lu <[email protected]>
> Sent: Friday, July 14, 2023 11:34 AM
>
> On 2023/7/13 15:56, Tian, Kevin wrote:
> >> From: Jacob Pan <[email protected]>
> >> Sent: Thursday, July 13, 2023 12:34 AM
> >>
> >>
> >> - /*
> >> - * Should never reach here until we add support for attaching
> >> - * non-SVA domain to a pasid.
> >> - */
> >> - WARN_ON(1);
> >> + dmar_domain = to_dmar_domain(domain);
> >> + spin_lock_irqsave(&dmar_domain->lock, flags);
> >> + list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain)
> >> {
> >> + if (curr->dev == dev && curr->pasid == pasid) {
> >> + list_del(&curr->link_domain);
> >> + dev_pasid = curr;
> >> + break;
> >> + }
> >> + }
> >> + spin_unlock_irqrestore(&dmar_domain->lock, flags);
> >>
> >
> > what about no matching dev_pasid is find?
>
> kfree() can handle this gracefully.

but what about domain_detach_iommu()? Is it correct to adjust
the refcnting when there is no matching dev_pasid?

2023-07-14 06:07:43

by Lu Baolu

[permalink] [raw]
Subject: Re: [PATCH v10 6/7] iommu/vt-d: Add set_dev_pasid callback for dma domain

On 2023/7/14 11:50, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Friday, July 14, 2023 11:34 AM
>>
>> On 2023/7/13 15:56, Tian, Kevin wrote:
>>>> From: Jacob Pan <[email protected]>
>>>> Sent: Thursday, July 13, 2023 12:34 AM
>>>>
>>>>
>>>> - /*
>>>> - * Should never reach here until we add support for attaching
>>>> - * non-SVA domain to a pasid.
>>>> - */
>>>> - WARN_ON(1);
>>>> + dmar_domain = to_dmar_domain(domain);
>>>> + spin_lock_irqsave(&dmar_domain->lock, flags);
>>>> + list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain)
>>>> {
>>>> + if (curr->dev == dev && curr->pasid == pasid) {
>>>> + list_del(&curr->link_domain);
>>>> + dev_pasid = curr;
>>>> + break;
>>>> + }
>>>> + }
>>>> + spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>>>
>>>
>>> what about no matching dev_pasid is find?
>>
>> kfree() can handle this gracefully.
>
> but what about domain_detach_iommu()? Is it correct to adjust
> the refcnting when there is no matching dev_pasid?

You are right.

Logically, if we get a valid domain for a pasid, we should have a
dev_pasid allocated for it. Perhaps, adding a check in the code will
make the code more readable?

Best regards,
baolu

2023-07-19 06:38:47

by Tian, Kevin

[permalink] [raw]
Subject: RE: [PATCH v10 6/7] iommu/vt-d: Add set_dev_pasid callback for dma domain

> From: Baolu Lu <[email protected]>
> Sent: Friday, July 14, 2023 2:00 PM
>
> On 2023/7/14 11:50, Tian, Kevin wrote:
> >> From: Baolu Lu <[email protected]>
> >> Sent: Friday, July 14, 2023 11:34 AM
> >>
> >> On 2023/7/13 15:56, Tian, Kevin wrote:
> >>>> From: Jacob Pan <[email protected]>
> >>>> Sent: Thursday, July 13, 2023 12:34 AM
> >>>>
> >>>>
> >>>> - /*
> >>>> - * Should never reach here until we add support for attaching
> >>>> - * non-SVA domain to a pasid.
> >>>> - */
> >>>> - WARN_ON(1);
> >>>> + dmar_domain = to_dmar_domain(domain);
> >>>> + spin_lock_irqsave(&dmar_domain->lock, flags);
> >>>> + list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain)
> >>>> {
> >>>> + if (curr->dev == dev && curr->pasid == pasid) {
> >>>> + list_del(&curr->link_domain);
> >>>> + dev_pasid = curr;
> >>>> + break;
> >>>> + }
> >>>> + }
> >>>> + spin_unlock_irqrestore(&dmar_domain->lock, flags);
> >>>>
> >>>
> >>> what about no matching dev_pasid is find?
> >>
> >> kfree() can handle this gracefully.
> >
> > but what about domain_detach_iommu()? Is it correct to adjust
> > the refcnting when there is no matching dev_pasid?
>
> You are right.
>
> Logically, if we get a valid domain for a pasid, we should have a
> dev_pasid allocated for it. Perhaps, adding a check in the code will
> make the code more readable?
>

yes