[PATCH v2 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc
Joao Martins
joao.m.martins at oracle.com
Thu Feb 22 05:23:26 PST 2024
On 22/02/2024 12:24, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins at oracle.com>
>> Sent: Thursday, February 22, 2024 11:38 AM
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>
>> Cc: joro at 8bytes.org; jgg at nvidia.com; kevin.tian at intel.com;
>> nicolinc at nvidia.com; iommu at lists.linux.dev; mshavit at google.com;
>> robin.murphy at arm.com; will at kernel.org; jiangkunkun
>> <jiangkunkun at huawei.com>; zhukeqian <zhukeqian1 at huawei.com>; Linuxarm
>> <linuxarm at huawei.com>; linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH v2 3/4] iommu/arm-smmu-v3: Add support for dirty tracking
>> in domain alloc
>>
>> On 22/02/2024 11:31, Shameerali Kolothum Thodi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Joao Martins <joao.m.martins at oracle.com>
>>>> Sent: Thursday, February 22, 2024 11:04 AM
>>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>
>>>> Cc: joro at 8bytes.org; jgg at nvidia.com; kevin.tian at intel.com;
>>>> nicolinc at nvidia.com; iommu at lists.linux.dev; mshavit at google.com;
>>>> robin.murphy at arm.com; will at kernel.org; jiangkunkun
>>>> <jiangkunkun at huawei.com>; zhukeqian <zhukeqian1 at huawei.com>;
>>>> Linuxarm <linuxarm at huawei.com>; linux-arm-kernel at lists.infradead.org
>>>> Subject: Re: [PATCH v2 3/4] iommu/arm-smmu-v3: Add support for dirty
>>>> tracking in domain alloc
>>>>
>>>> On 22/02/2024 09:49, Shameer Kolothum wrote:
>>>>> From: Joao Martins <joao.m.martins at oracle.com>
>>>>>
>>>>> This provides all the infrastructure to enable dirty tracking if the
>>>>> hardware has the capability and domain alloc request for it.
>>>>>
>>>>> Please note, we still report no support for IOMMU_CAP_DIRTY_TRACKING
>>>>> as it will finally be enabled in a subsequent patch.
>>>>>
>>>>> Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
>>>>> Signed-off-by: Shameer Kolothum
>>>> <shameerali.kolothum.thodi at huawei.com>
>>>>> ---
>>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 95
>>>> ++++++++++++++++-----
>>>>> include/linux/io-pgtable.h | 4 +
>>>>> 2 files changed, 77 insertions(+), 22 deletions(-)
>>>>>
>>>>> 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 bd30739e3588..058bbb0dbe2e 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> @@ -43,6 +43,7 @@ MODULE_PARM_DESC(disable_msipolling,
>>>>> "Disable MSI-based polling for CMD_SYNC completion.");
>>>>>
>>>>> static struct iommu_ops arm_smmu_ops;
>>>>> +static struct iommu_dirty_ops arm_smmu_dirty_ops;
>>>>>
>>>>> enum arm_smmu_msi_index {
>>>>> EVTQ_MSI_INDEX,
>>>>> @@ -86,7 +87,8 @@ static struct arm_smmu_option_prop
>>>>> arm_smmu_options[] = {
>>>>>
>>>>> static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device
>>>>> *smmu); static int arm_smmu_domain_finalise(struct arm_smmu_domain
>>>> *smmu_domain,
>>>>> - struct arm_smmu_device *smmu);
>>>>> + struct arm_smmu_device *smmu,
>>>>> + bool enable_dirty);
>>>>> static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
>>>>> static void arm_smmu_tlb_inv_all_s2(struct arm_smmu_domain
>>>>> *smmu_domain);
>>>>>
>>>>> @@ -2378,7 +2380,7 @@ static struct iommu_domain
>>>> *arm_smmu_domain_alloc_paging(struct device *dev)
>>>>> struct arm_smmu_master *master =
>>>> dev_iommu_priv_get(dev);
>>>>> int ret;
>>>>>
>>>>> - ret = arm_smmu_domain_finalise(smmu_domain, master-
>>>>> smmu);
>>>>> + ret = arm_smmu_domain_finalise(smmu_domain, master-
>>>>> smmu, false);
>>>>> if (ret) {
>>>>> kfree(smmu_domain);
>>>>> return ERR_PTR(ret);
>>>>> @@ -2445,10 +2447,11 @@ static void arm_smmu_domain_free(struct
>>>>> iommu_domain *domain) }
>>>>>
>>>>> static int arm_smmu_domain_finalise(struct arm_smmu_domain
>>>> *smmu_domain,
>>>>> - struct arm_smmu_device *smmu)
>>>>> + struct arm_smmu_device *smmu,
>>>>> + bool enable_dirty)
>>>>> {
>>>>> int ret;
>>>>> - unsigned long ias, oas;
>>>>> + unsigned long ias;
>>>>> enum io_pgtable_fmt fmt;
>>>>> struct io_pgtable_cfg pgtbl_cfg;
>>>>> struct io_pgtable_ops *pgtbl_ops;
>>>>> @@ -2459,31 +2462,31 @@ static int arm_smmu_domain_finalise(struct
>>>> arm_smmu_domain *smmu_domain,
>>>>> if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>>>>> smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>>>>>
>>>>> + pgtbl_cfg = (struct io_pgtable_cfg) {
>>>>> + .pgsize_bitmap = smmu->pgsize_bitmap,
>>>>> + .coherent_walk = smmu->features &
>>>> ARM_SMMU_FEAT_COHERENCY,
>>>>> + .tlb = &arm_smmu_flush_ops,
>>>>> + .iommu_dev = smmu->dev,
>>>>> + };
>>>>> +
>>>>> switch (smmu_domain->stage) {
>>>>> case ARM_SMMU_DOMAIN_S1:
>>>>> ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
>>>>> - ias = min_t(unsigned long, ias, VA_BITS);
>>>>> - oas = smmu->ias;
>>>>> + pgtbl_cfg.ias = min_t(unsigned long, ias, VA_BITS);
>>>>> + pgtbl_cfg.oas = smmu->ias;
>>>>> + if (enable_dirty)
>>>>> + pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
>>>>> fmt = ARM_64_LPAE_S1;
>>>>> break;
>>>>> case ARM_SMMU_DOMAIN_S2:
>>>>> - ias = smmu->ias;
>>>>> - oas = smmu->oas;
>>>>> + pgtbl_cfg.ias = smmu->ias;
>>>>> + pgtbl_cfg.oas = smmu->oas;
>>>>> fmt = ARM_64_LPAE_S2;
>>>>> break;
>>>>> default:
>>>>> return -EINVAL;
>>>>> }
>>>>>
>>>>> - pgtbl_cfg = (struct io_pgtable_cfg) {
>>>>> - .pgsize_bitmap = smmu->pgsize_bitmap,
>>>>> - .ias = ias,
>>>>> - .oas = oas,
>>>>> - .coherent_walk = smmu->features &
>>>> ARM_SMMU_FEAT_COHERENCY,
>>>>> - .tlb = &arm_smmu_flush_ops,
>>>>> - .iommu_dev = smmu->dev,
>>>>> - };
>>>>> -
>>>>> pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
>>>>> if (!pgtbl_ops)
>>>>> return -ENOMEM;
>>>>> @@ -2491,7 +2494,8 @@ static int arm_smmu_domain_finalise(struct
>>>> arm_smmu_domain *smmu_domain,
>>>>> smmu_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
>>>>> smmu_domain->domain.geometry.aperture_end = (1UL <<
>>>> pgtbl_cfg.ias) - 1;
>>>>> smmu_domain->domain.geometry.force_aperture = true;
>>>>> -
>>>>> + if (enable_dirty && smmu_domain->stage ==
>>>> ARM_SMMU_DOMAIN_S1)
>>>>> + smmu_domain->domain.dirty_ops =
>>>> &arm_smmu_dirty_ops;
>>>>> ret = arm_smmu_domain_alloc_id(smmu, smmu_domain);
>>>>> if (ret < 0) {
>>>>> free_io_pgtable_ops(pgtbl_ops);
>>>>> @@ -2811,7 +2815,7 @@ static int arm_smmu_attach_dev(struct
>>>> iommu_domain *domain, struct device *dev)
>>>>> mutex_lock(&smmu_domain->init_mutex);
>>>>>
>>>>> if (!smmu_domain->smmu) {
>>>>> - ret = arm_smmu_domain_finalise(smmu_domain, smmu);
>>>>> + ret = arm_smmu_domain_finalise(smmu_domain, smmu,
>>>> false);
>>>>> } else if (smmu_domain->smmu != smmu)
>>>>> ret = -EINVAL;
>>>>>
>>>>
>>>>
>>>> I think we are missing the domain attach_dev check for dirty tracking.
>>>>
>>>> Something like:
>>>>
>>>> if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu))
>>>> return -EINVAL;
>>>>
>>>> But that helper is only introduced in the last patch, so maybe:
>>>>
>>>> if (domain->dirty_ops &&
>>>> !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING))
>>>> return -EINVAL;
>>>
>>> Ok. But do we really need to check this in attach()? As dirty_ops are added only
>>> if it is requested in alloc_user() and there we return err when hardware doesn't
>>> have the capability. So not sure how this matters in attach() path. May be I am
>>> missing something.
>>
>> That's when you create the domain with dev A. Afterwards that dev A is
>> attached,
>> but later on you can attach another device B to the domain. So this check is
>> there such that a domain with dirty tracking ops set will only have devices in
>> there that support dirty tracking.
>
> But that only matters if dev B is on different smmu without dbm capability, right?
> In that case we already fail the attach with,
>
>>>> } else if (smmu_domain->smmu != smmu)
>>>>> ret = -EINVAL;
>>>>>
>
> Is that right?
>
Oh yes, I missed that. That looks indeed to do the equivalent.
Perhaps a comment there at least to remind to reader in the event that such
check being lifted.
More information about the linux-arm-kernel
mailing list