Re: [PATCH v3 04/19] iommufd: Add a flag to enforce dirty tracking on attach

Joao Martins
2023-10-13T16:20:30Z

On 13/10/2023 16:52, Jason Gunthorpe wrote:
> On Sat, Sep 23, 2023 at 02:24:56AM +0100, Joao Martins wrote:
>> Throughout IOMMU domain lifetime that wants to use dirty tracking, some
>> guarantees are needed such that any device attached to the iommu_domain
>> supports dirty tracking.
>>
>> The idea is to handle a case where IOMMU in the system are assymetric
>> feature-wise and thus the capability may not be supported for all devices.
>> The enforcement is done by adding a flag into HWPT_ALLOC namely:
>>
>> IOMMUFD_HWPT_ALLOC_ENFORCE_DIRTY
>>
>> .. Passed in HWPT_ALLOC ioctl() flags. The enforcement is done by creating
>> a iommu_domain via domain_alloc_user() and validating the requested flags
>> with what the device IOMMU supports (and failing accordingly) advertised).
>> Advertising the new IOMMU domain feature flag requires that the individual
>> iommu driver capability is supported when a future device attachment
>> happens.
>>
>> Link: https://lore.kernel.org/kvm/20220721142421.GB4609@nvidia.com/
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> drivers/iommu/iommufd/hw_pagetable.c | 8 ++++++--
>> include/uapi/linux/iommufd.h | 3 +++
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
>> index 26a8a818ffa3..32e259245314 100644
>> --- a/drivers/iommu/iommufd/hw_pagetable.c
>> +++ b/drivers/iommu/iommufd/hw_pagetable.c
>> @@ -83,7 +83,9 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>>
>> lockdep_assert_held(&ioas->mutex);
>>
>> - if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ops->domain_alloc_user)
>> + if ((flags & (IOMMU_HWPT_ALLOC_NEST_PARENT|
>> + IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) &&
>> + !ops->domain_alloc_user)
>> return ERR_PTR(-EOPNOTSUPP);
>
> This seems strange, why are we testing flags here? shouldn't this just
> be
>
> if (flags && !ops->domain_alloc_user)
> return ERR_PTR(-EOPNOTSUPP);
>
> ?

Yeah makes sense, let me switch to that.

It achieves the same without into the weeds of missing a flag update. And any
flag essentially requires alloc_user regardless.

>
>> hwpt = iommufd_object_alloc(ictx, hwpt, IOMMUFD_OBJ_HW_PAGETABLE);
>> @@ -157,7 +159,9 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
>> struct iommufd_ioas *ioas;
>> int rc;
>>
>> - if (cmd->flags & ~IOMMU_HWPT_ALLOC_NEST_PARENT || cmd->__reserved)
>> + if ((cmd->flags &
>> + ~(IOMMU_HWPT_ALLOC_NEST_PARENT|IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) ||
>> + cmd->__reserved)
>> return -EOPNOTSUPP;
>
> Please checkpatch your stuff,

I always do this, and there was no issues reported on this patch.

Sometimes there's one or another I ignore albeit very rarely (e.g. passing 1
character post 80 column gap and which fixing makes the code uglier).

> and even better feed the patches to
> clang-format and do most of what it says.
>

This one I don't (but I wasn't aware either that it's a thing)

> Otherwise seems like the right thing to do
>
> Jason