On 19/05/2023 14:29, Jason Gunthorpe wrote:
> On Fri, May 19, 2023 at 12:56:19PM +0100, Joao Martins wrote:
>> On 19/05/2023 12:51, Jason Gunthorpe wrote:
>>> On Fri, May 19, 2023 at 12:47:24PM +0100, Joao Martins wrote:
>>>> In practice it is done as soon after the domain is created but I understand what
>>>> you mean that both should be together; I have this implemented like that as my
>>>> first take as a domain_alloc passed flags, but I was a little undecided because
>>>> we are adding another domain_alloc() op for the user-managed pagetable and after
>>>> having another one we would end up with 3 ways of creating iommu domain -- but
>>>> maybe that's not an issue
>>>
>>> It should ride on the same user domain alloc op as some generic flags,
>>
>> OK, I suppose that makes sense specially with this being tied in HWPT_ALLOC
>> where all this new user domain alloc does.
>
> Yes, it should be easy.
>
> Then do what Robin said and make the domain ops NULL if the user
> didn't ask for dirty tracking and then attach can fail if there are
> domain incompatibility's.
>
> Since alloc_user (or whatever it settles into) will have the struct
> device * argument this should be easy enough with out getting mixed
> with the struct bus cleanup.
Taking a step back, the iommu domain ops are a shared global pointer in all
iommu domains AFAIU at least in all three iommu implementations I was targetting
with this -- init-ed from iommu_ops::domain_default_ops. Not something we can
"just" clear part of it as that's the same global pointer shared with every
other domain. We would have to duplicate for every vendor two domain ops: one
with dirty and another without dirty tracking; though the general sentiment
behind clearing makes sense
But this is for IOMMUFD API driven only, perhaps we can just enforce at HWPT
allocation time as we are given a device ID there, or via device_attach too
inside iommufd core when we attach a device to an already existent hwpt.
This is a bit simpler and as a bonus it avoids getting dependent on the
domain_alloc_user() nesting infra and no core iommu domain changes;
Unless we also need to worry about non-IOMMUFD device-attach, which I don't
think it is the case here
e.g.
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 4c37eeea2bcd..4966775f5b00 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -339,6 +339,12 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable
*hwpt,
goto err_unlock;
}
+ if (hwpt->enforce_dirty &&
+ !device_iommu_capable(idev->dev, IOMMU_CAP_DIRTY)) {
+ rc = -EINVAL;
+ goto out_abort;
+ }
+
/* Try to upgrade the domain we have */
if (idev->enforce_cache_coherency) {
rc = iommufd_hw_pagetable_enforce_cc(hwpt);
@@ -542,7 +548,7 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev,
}
hwpt = iommufd_hw_pagetable_alloc(idev->ictx, ioas, idev,
- immediate_attach);
+ immediate_attach, false);
if (IS_ERR(hwpt)) {
destroy_hwpt = ERR_CAST(hwpt);
goto out_unlock;
diff --git a/drivers/iommu/iommufd/hw_pagetable.c
b/drivers/iommu/iommufd/hw_pagetable.c
index 838530460d9b..da831b4404fd 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -62,6 +62,8 @@ int iommufd_hw_pagetable_enforce_cc(struct
iommufd_hw_pagetable *hwpt)
* @ioas: IOAS to associate the domain with
* @idev: Device to get an iommu_domain for
* @immediate_attach: True if idev should be attached to the hwpt
+ * @enforce_dirty: True if dirty tracking support should be enforced
+ * on device attach
*
* Allocate a new iommu_domain and return it as a hw_pagetable. The HWPT
* will be linked to the given ioas and upon return the underlying iommu_domain
@@ -73,7 +75,8 @@ int iommufd_hw_pagetable_enforce_cc(struct
iommufd_hw_pagetable *hwpt)
*/
struct iommufd_hw_pagetable *
iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
- struct iommufd_device *idev, bool immediate_attach)
+ struct iommufd_device *idev, bool immediate_attach,
+ bool enforce_dirty)
{
const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
struct iommufd_hw_pagetable *hwpt;
@@ -90,8 +93,17 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct
iommufd_ioas *ioas,
refcount_inc(&ioas->obj.users);
hwpt->ioas = ioas;
+ if (enforce_dirty &&
+ !device_iommu_capable(idev->dev, IOMMU_CAP_DIRTY)) {
+ rc = -EINVAL;
+ goto out_abort;
+ }
+
@@ -99,6 +111,8 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct
iommufd_ioas *ioas,
hwpt->domain = NULL;
goto out_abort;
}
+
+ hwpt->enforce_dirty = enforce_dirty;
} else {
hwpt->domain = iommu_domain_alloc(idev->dev->bus);
if (!hwpt->domain) {
@@ -154,7 +168,8 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
struct iommufd_ioas *ioas;
int rc;
- if (cmd->flags || cmd->__reserved)
+ if ((cmd->flags & ~(IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) ||
+ cmd->__reserved)
return -EOPNOTSUPP;
idev = iommufd_get_device(ucmd, cmd->dev_id);
@@ -168,7 +183,8 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
}
mutex_lock(&ioas->mutex);
- hwpt = iommufd_hw_pagetable_alloc(ucmd->ictx, ioas, idev, false);
+ hwpt = iommufd_hw_pagetable_alloc(ucmd->ictx, ioas, idev, false,
+ cmd->flags & IOMMU_HWPT_ALLOC_ENFORCE_DIRTY);
if (IS_ERR(hwpt)) {
rc = PTR_ERR(hwpt);
goto out_unlock;
diff --git a/drivers/iommu/iommufd/iommufd_private.h
b/drivers/iommu/iommufd/iommufd_private.h
index 8ba786bc95ff..7f0173e54c9c 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -247,6 +247,7 @@ struct iommufd_hw_pagetable {
struct iommu_domain *domain;
bool auto_domain : 1;
bool enforce_cache_coherency : 1;
+ bool enforce_dirty : 1;
bool msi_cookie : 1;
/* Head at iommufd_ioas::hwpt_list */
struct list_head hwpt_item;
