Re: [PATCH RFCv2 24/24] iommu/arm-smmu-v3: Advertise IOMMU_DOMAIN_F_ENFORCE_DIRTY

[Joao Martins <joao.m.martins@oracle.com>]

On 30/05/2023 15:10, Shameerali Kolothum Thodi wrote:
>> -----Original Message-----
>> From: Joao Martins [mailto:joao.m.martins@oracle.com]
>> Sent: 18 May 2023 21:47
>> To: iommu@lists.linux.dev
>> Cc: Jason Gunthorpe <jgg@nvidia.com>; Kevin Tian <kevin.tian@intel.com>;
>> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; Lu
>> Baolu <baolu.lu@linux.intel.com>; Yi Liu <yi.l.liu@intel.com>; Yi Y Sun
>> <yi.y.sun@intel.com>; Eric Auger <eric.auger@redhat.com>; Nicolin Chen
>> <nicolinc@nvidia.com>; Joerg Roedel <joro@8bytes.org>; Jean-Philippe
>> Brucker <jean-philippe@linaro.org>; Suravee Suthikulpanit
>> <suravee.suthikulpanit@amd.com>; Will Deacon <will@kernel.org>; Robin
>> Murphy <robin.murphy@arm.com>; Alex Williamson
>> <alex.williamson@redhat.com>; kvm@vger.kernel.org; Joao Martins
>> <joao.m.martins@oracle.com>
>> Subject: [PATCH RFCv2 24/24] iommu/arm-smmu-v3: Advertise
>> IOMMU_DOMAIN_F_ENFORCE_DIRTY
>>
>> Now that we probe, and handle the DBM bit modifier, unblock
>> the kAPI usage by exposing the IOMMU_DOMAIN_F_ENFORCE_DIRTY
>> and implement it's requirement of revoking device attachment
>> in the iommu_capable. Finally expose the IOMMU_CAP_DIRTY to
>> users (IOMMUFD_DEVICE_GET_CAPS).
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index bf0aac333725..71dd95a687fd 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -2014,6 +2014,8 @@ static bool arm_smmu_capable(struct device *dev,
>> enum iommu_cap cap)
>>  		return master->smmu->features &
>> ARM_SMMU_FEAT_COHERENCY;
>>  	case IOMMU_CAP_NOEXEC:
>>  		return true;
>> +	case IOMMU_CAP_DIRTY:
>> +		return arm_smmu_dbm_capable(master->smmu);
>>  	default:
>>  		return false;
>>  	}
>> @@ -2430,6 +2432,11 @@ static int arm_smmu_attach_dev(struct
>> iommu_domain *domain, struct device *dev)
>>  	master = dev_iommu_priv_get(dev);
>>  	smmu = master->smmu;
>>
>> +	if (domain->flags & IOMMU_DOMAIN_F_ENFORCE_DIRTY &&
>> +	    !arm_smmu_dbm_capable(smmu))
>> +		return -EINVAL;
>> +
>> +
> 
> Since we have the supported_flags always set to " IOMMU_DOMAIN_F_ENFORCE_DIRTY"
> below, platforms that doesn't have DBM capability will fail here, right? 
> Or the idea is to set
> domain flag only if the capability is reported true? But the iommu_domain_set_flags() doesn't
> seems to check the capability though. 
> 
As posted the checking was only take place at device_attach (and you would set
the enforcement flag if iommufd reports the capability for the device via
IOMMU_DEVICE_GET_CAPS).

But the workflow will change a bit: while the enforcement also takes place on
device attach, but when we create a HWPT domain with flags (in
domain_alloc_user[0]), the dirty tracking is also going to be checked there
against the device passed in domain_alloc_user() in the driver implementation.
And otherwise fail if doesn't support when dirty-tracking support enforcement as
passed by flags. When we don't request dirty tracking the iommu ops that perform
the dirty tracking will also be kept cleared.

[0] https://lore.kernel.org/linux-iommu/20230511143844.22693-2-yi.l.liu@intel.com/

> (This seems to be causing problem with a rebased Qemu branch for ARM I have while sanity
> testing on a platform that doesn't have DBM. I need to double check though).
> 

Perhaps due to the broken check I had that I need validate the two bits
together, when it didn't had DBM set? Or I suspect because the qemu last patch I
was always end up setting IOMMU_DOMAIN_F_ENFORCE_DIRTY [*], and because the
checking is always enabled you can never attach devices.

[*] That last patch isn't quite there yet as it is meant to be using
device-get-caps prior to setting the enforcement, like the selftests