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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

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