diff mbox series

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

Message ID 20230518204650.14541-25-joao.m.martins@oracle.com (mailing list archive)
State New, archived
Headers show
Series IOMMUFD Dirty Tracking | expand

Commit Message

Joao Martins May 18, 2023, 8:46 p.m. UTC 
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(+)

Comments

Shameerali Kolothum Thodi May 30, 2023, 2:10 p.m. UTC | #1 
> -----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. 

(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).

Thanks,
Shameer

   
>  	/*
>  	 * Checking that SVA is disabled ensures that this device isn't bound to
>  	 * any mm, and can be safely detached from its old domain. Bonds
> cannot
> @@ -2913,6 +2920,7 @@ static void arm_smmu_remove_dev_pasid(struct
> device *dev, ioasid_t pasid)
>  }
> 
>  static struct iommu_ops arm_smmu_ops = {
> +	.supported_flags	= IOMMU_DOMAIN_F_ENFORCE_DIRTY,
>  	.capable		= arm_smmu_capable,
>  	.domain_alloc		= arm_smmu_domain_alloc,
>  	.probe_device		= arm_smmu_probe_device,
> --
> 2.17.2
Joao Martins May 30, 2023, 7:19 p.m. UTC | #2 
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
Shameerali Kolothum Thodi May 31, 2023, 9:21 a.m. UTC | #3 
> -----Original Message-----
> From: Joao Martins [mailto:joao.m.martins@oracle.com]
> Sent: 30 May 2023 20:20
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Jason Gunthorpe <jgg@nvidia.com>; Kevin Tian <kevin.tian@intel.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;
> iommu@lists.linux.dev
> Subject: Re: [PATCH RFCv2 24/24] iommu/arm-smmu-v3: Advertise
> IOMMU_DOMAIN_F_ENFORCE_DIRTY
> 
> 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).

Ok. So CAPS is retrieved before we set the enforcement flag.

> 
> 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.

Ok.

> 
> [0]
> https://lore.kernel.org/linux-iommu/20230511143844.22693-2-yi.l.liu@inte
> l.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?

I have that fixed in my branch now.

 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.

Ah.. this is it. 

> [*] 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

Got it.

Thanks,
Shameer
diff mbox series

Patch

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;
+
+
 	/*
 	 * Checking that SVA is disabled ensures that this device isn't bound to
 	 * any mm, and can be safely detached from its old domain. Bonds cannot
@@ -2913,6 +2920,7 @@  static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 }
 
 static struct iommu_ops arm_smmu_ops = {
+	.supported_flags	= IOMMU_DOMAIN_F_ENFORCE_DIRTY,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,