Update iopf enablement in the iommufd mock device driver to use the new
method, similar to the arm-smmu-v3 driver. Enable iopf support when any
domain with an iopf_handler is attached, and disable it when the domain
is removed.
Add a refcount in the mock device state structure to keep track of the
number of domains set to the device and PASIDs that require iopf.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/iommufd/selftest.c | 52 +++++++++++++++++++++++++-------
1 file changed, 41 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index d40deb0a4f06..a6b12cee7b00 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -58,6 +58,9 @@ enum {
MOCK_PFN_HUGE_IOVA = _MOCK_PFN_START << 2,
};
+static int mock_dev_enable_iopf(struct device *dev, struct iommu_domain *domain);
+static void mock_dev_disable_iopf(struct device *dev, struct iommu_domain *domain);
+
/*
* Syzkaller has trouble randomizing the correct iova to use since it is linked
* to the map ioctl's output, and it has no ide about that. So, simplify things.
@@ -164,6 +167,7 @@ struct mock_dev {
unsigned long flags;
int id;
u32 cache[MOCK_DEV_CACHE_NUM];
+ unsigned int iopf_refcount;
};
static inline struct mock_dev *to_mock_dev(struct device *dev)
@@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
return -EINVAL;
+ return mock_dev_enable_iopf(dev, domain);
+}
+
+static int mock_domain_blocking_attach(struct iommu_domain *domain,
+ struct device *dev)
+{
+ mock_dev_disable_iopf(dev, domain);
+
return 0;
}
static const struct iommu_domain_ops mock_blocking_ops = {
- .attach_dev = mock_domain_nop_attach,
+ .attach_dev = mock_domain_blocking_attach,
};
static struct iommu_domain mock_blocking_domain = {
@@ -549,22 +561,42 @@ static void mock_domain_page_response(struct device *dev, struct iopf_fault *evt
{
}
-static int mock_dev_enable_feat(struct device *dev, enum iommu_dev_features feat)
+static int mock_dev_enable_iopf(struct device *dev, struct iommu_domain *domain)
{
- if (feat != IOMMU_DEV_FEAT_IOPF || !mock_iommu_iopf_queue)
+ struct mock_dev *mdev = to_mock_dev(dev);
+ int ret;
+
+ if (!domain->iopf_handler)
+ return 0;
+
+ if (!mock_iommu_iopf_queue)
return -ENODEV;
- return iopf_queue_add_device(mock_iommu_iopf_queue, dev);
+ if (mdev->iopf_refcount) {
+ mdev->iopf_refcount++;
+ return 0;
+ }
+
+ ret = iopf_queue_add_device(mock_iommu_iopf_queue, dev);
+ if (ret)
+ return ret;
+
+ mdev->iopf_refcount = 1;
+
+ return 0;
}
-static int mock_dev_disable_feat(struct device *dev, enum iommu_dev_features feat)
+static void mock_dev_disable_iopf(struct device *dev, struct iommu_domain *domain)
{
- if (feat != IOMMU_DEV_FEAT_IOPF || !mock_iommu_iopf_queue)
- return -ENODEV;
+ struct mock_dev *mdev = to_mock_dev(dev);
+
+ if (!domain->iopf_handler)
+ return;
+
+ if (--mdev->iopf_refcount)
+ return;
iopf_queue_remove_device(mock_iommu_iopf_queue, dev);
-
- return 0;
}
static void mock_viommu_destroy(struct iommufd_viommu *viommu)
@@ -709,8 +741,6 @@ static const struct iommu_ops mock_ops = {
.device_group = generic_device_group,
.probe_device = mock_probe_device,
.page_response = mock_domain_page_response,
- .dev_enable_feat = mock_dev_enable_feat,
- .dev_disable_feat = mock_dev_disable_feat,
.user_pasid_table = true,
.viommu_alloc = mock_viommu_alloc,
.default_domain_ops =
--
2.43.0On Fri, Feb 14, 2025 at 02:11:00PM +0800, Lu Baolu wrote:
> @@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
> if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
> return -EINVAL;
>
> + return mock_dev_enable_iopf(dev, domain);
> +}
This isn't going to work for a replace type operation? Maybe like:
if (old_domain->iopf_handler && !domain->iopf_handler)
return mock_dev_disable_iopf(dev, domain);
if (old_domain->iopf_handler && domain->iopf_handler)
return 0;
return mock_dev_enable_iopf(dev, domain);
?
Jason
On 2/20/25 09:02, Jason Gunthorpe wrote:
> On Fri, Feb 14, 2025 at 02:11:00PM +0800, Lu Baolu wrote:
>> @@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
>> if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
>> return -EINVAL;
>>
>> + return mock_dev_enable_iopf(dev, domain);
>> +}
>
> This isn't going to work for a replace type operation? Maybe like:
>
> if (old_domain->iopf_handler && !domain->iopf_handler)
> return mock_dev_disable_iopf(dev, domain);
> if (old_domain->iopf_handler && domain->iopf_handler)
> return 0;
> return mock_dev_enable_iopf(dev, domain);
>
> ?
The iommufd mock device driver appears not to support replacement. The
replacement operation on this driver is likely handled as follows:
- attach domain_a
- attach blocking_domain
- attach domain_b
The mock_dev_disable_iopf() is called in attach_dev of the blocking
domain.
There seems to be a bug in this patch. The existing domain should be
passed to mock_dev_disable_iopf(). It requires something similar to the
following:
diff --git a/drivers/iommu/iommufd/selftest.c
b/drivers/iommu/iommufd/selftest.c
index a6b12cee7b00..54a6f0851758 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -168,6 +168,7 @@ struct mock_dev {
int id;
u32 cache[MOCK_DEV_CACHE_NUM];
unsigned int iopf_refcount;
+ struct iommu_domain *domain;
};
static inline struct mock_dev *to_mock_dev(struct device *dev)
@@ -197,17 +198,24 @@ static int mock_domain_nop_attach(struct
iommu_domain *domain,
struct device *dev)
{
struct mock_dev *mdev = to_mock_dev(dev);
+ int ret;
if (domain->dirty_ops && (mdev->flags &
MOCK_FLAGS_DEVICE_NO_DIRTY))
return -EINVAL;
- return mock_dev_enable_iopf(dev, domain);
+ ret = mock_dev_enable_iopf(dev, domain);
+ if (ret)
+ return ret;
+
+ mdev->domain = domain;
}
static int mock_domain_blocking_attach(struct iommu_domain *domain,
struct device *dev)
{
- mock_dev_disable_iopf(dev, domain);
+ struct mock_dev *mdev = to_mock_dev(dev);
+
+ mock_dev_disable_iopf(dev, mdev->domain);
return 0;
}
Thanks,
baolu
On Thu, Feb 20, 2025 at 03:03:21PM +0800, Baolu Lu wrote: > On 2/20/25 09:02, Jason Gunthorpe wrote: > > On Fri, Feb 14, 2025 at 02:11:00PM +0800, Lu Baolu wrote: > > > @@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain, > > > if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY)) > > > return -EINVAL; > > > + return mock_dev_enable_iopf(dev, domain); > > > +} > > > > This isn't going to work for a replace type operation? Maybe like: > > > > if (old_domain->iopf_handler && !domain->iopf_handler) > > return mock_dev_disable_iopf(dev, domain); > > if (old_domain->iopf_handler && domain->iopf_handler) > > return 0; > > return mock_dev_enable_iopf(dev, domain); > > > > ? > > The iommufd mock device driver appears not to support replacement. That's not technically a choice the driver gets to have .. > The > replacement operation on this driver is likely handled as follows: > > - attach domain_a attach blocking_domain attach domain_b Nothing actually does that though? Jason
On 2/21/25 02:00, Jason Gunthorpe wrote:
> On Thu, Feb 20, 2025 at 03:03:21PM +0800, Baolu Lu wrote:
>> On 2/20/25 09:02, Jason Gunthorpe wrote:
>>> On Fri, Feb 14, 2025 at 02:11:00PM +0800, Lu Baolu wrote:
>>>> @@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
>>>> if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
>>>> return -EINVAL;
>>>> + return mock_dev_enable_iopf(dev, domain);
>>>> +}
>>> This isn't going to work for a replace type operation? Maybe like:
>>>
>>> if (old_domain->iopf_handler && !domain->iopf_handler)
>>> return mock_dev_disable_iopf(dev, domain);
>>> if (old_domain->iopf_handler && domain->iopf_handler)
>>> return 0;
>>> return mock_dev_enable_iopf(dev, domain);
>>>
>>> ?
>> The iommufd mock device driver appears not to support replacement.
> That's not technically a choice the driver gets to have ..
Yes.
>
>> The
>> replacement operation on this driver is likely handled as follows:
>>
>> - attach domain_a attach blocking_domain attach domain_b
> Nothing actually does that though?
Ah! you are right.
This driver allows a new domain to be set even if a domain already
exists. This is different from what vt-d driver does, which removes the
old domain first. So perhaps we need to enhance it in two ways:
- Store the existing domain (a.k.a. old domain).
- Handle iopf enablement, taking the existing domain into account.
How about below change?
diff --git a/drivers/iommu/iommufd/selftest.c
b/drivers/iommu/iommufd/selftest.c
index a6b12cee7b00..5ffbd4e3f372 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -168,6 +168,7 @@ struct mock_dev {
int id;
u32 cache[MOCK_DEV_CACHE_NUM];
unsigned int iopf_refcount;
+ struct iommu_domain *domain;
};
static inline struct mock_dev *to_mock_dev(struct device *dev)
@@ -197,17 +198,28 @@ static int mock_domain_nop_attach(struct
iommu_domain *domain,
struct device *dev)
{
struct mock_dev *mdev = to_mock_dev(dev);
+ int ret;
if (domain->dirty_ops && (mdev->flags &
MOCK_FLAGS_DEVICE_NO_DIRTY))
return -EINVAL;
- return mock_dev_enable_iopf(dev, domain);
+ if (mdev->domain)
+ mock_dev_disable_iopf(dev, mdev->domain);
+
+ ret = mock_dev_enable_iopf(dev, domain);
+ if (ret)
+ return ret;
+
+ mdev->domain = domain;
+ return 0;
}
Thanks,
baolu
On Fri, Feb 21, 2025 at 09:31:48AM +0800, Baolu Lu wrote: > How about below change? Sure > - return mock_dev_enable_iopf(dev, domain); > + if (mdev->domain) > + mock_dev_disable_iopf(dev, mdev->domain); > + > + ret = mock_dev_enable_iopf(dev, domain); > + if (ret) Though here the domain is disabled but not removed from mdev->domain, is it OK? Jason
On 2/21/25 23:04, Jason Gunthorpe wrote:
>> - return mock_dev_enable_iopf(dev, domain);
>> + if (mdev->domain)
>> + mock_dev_disable_iopf(dev, mdev->domain);
>> +
>> + ret = mock_dev_enable_iopf(dev, domain);
>> + if (ret)
> Though here the domain is disabled but not removed from mdev->domain,
> is it OK?
That's not okay. I can make it like below:
static int mock_domain_nop_attach(struct iommu_domain *domain,
struct device *dev)
{
struct mock_dev *mdev = to_mock_dev(dev);
int ret;
if (domain->dirty_ops && (mdev->flags &
MOCK_FLAGS_DEVICE_NO_DIRTY))
return -EINVAL;
ret = mock_dev_enable_iopf(dev, domain);
if (ret)
return ret;
mock_dev_disable_iopf(dev, mdev->domain);
mdev->domain = domain;
return 0;
}
Both mock_dev_enable/disable_iopf() will be a no-op if domain or
domain's iopf handler is empty:
if (!domain || !domain->iopf_handler)
return;
Does it work for you?
Thanks,
baolu
On Sat, Feb 22, 2025 at 03:25:43PM +0800, Baolu Lu wrote:
> On 2/21/25 23:04, Jason Gunthorpe wrote:
> > > - return mock_dev_enable_iopf(dev, domain);
> > > + if (mdev->domain)
> > > + mock_dev_disable_iopf(dev, mdev->domain);
> > > +
> > > + ret = mock_dev_enable_iopf(dev, domain);
> > > + if (ret)
> > Though here the domain is disabled but not removed from mdev->domain,
> > is it OK?
>
> That's not okay. I can make it like below:
>
> static int mock_domain_nop_attach(struct iommu_domain *domain,
> struct device *dev)
> {
> struct mock_dev *mdev = to_mock_dev(dev);
> int ret;
>
> if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
> return -EINVAL;
>
> ret = mock_dev_enable_iopf(dev, domain);
> if (ret)
> return ret;
>
> mock_dev_disable_iopf(dev, mdev->domain);
> mdev->domain = domain;
>
> return 0;
> }
>
> Both mock_dev_enable/disable_iopf() will be a no-op if domain or
> domain's iopf handler is empty:
>
> if (!domain || !domain->iopf_handler)
> return;
>
> Does it work for you?
Yeah that looks Ok
Jason
© 2016 - 2025 Red Hat, Inc.