| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b1625daf-b77a-4838-8fed-f5ee2233fd2a@linux.intel.com>
Date: Wed, 12 Mar 2025 11:10:34 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Will Deacon <will@...nel.org>
Cc: Joerg Roedel <joro@...tes.org>, Robin Murphy <robin.murphy@....com>,
Jason Gunthorpe <jgg@...pe.ca>, Kevin Tian <kevin.tian@...el.com>,
Dave Jiang <dave.jiang@...el.com>, Vinod Koul <vkoul@...nel.org>,
Fenghua Yu <fenghuay@...dia.com>, Zhangfei Gao <zhangfei.gao@...aro.org>,
Zhou Wang <wangzhou1@...ilicon.com>, iommu@...ts.linux.dev,
linux-kernel@...r.kernel.org, Jason Gunthorpe <jgg@...dia.com>
Subject: Re: [PATCH v3 01/12] iommu/arm-smmu-v3: Put iopf enablement in the
domain attach path
On 3/12/25 00:13, Will Deacon wrote:
> On Fri, Feb 28, 2025 at 05:26:20PM +0800, Lu Baolu wrote:
>> From: Jason Gunthorpe <jgg@...dia.com>
>>
>> SMMUv3 co-mingles FEAT_IOPF and FEAT_SVA behaviors so that fault reporting
>> doesn't work unless both are enabled. This is not correct and causes
>> problems for iommufd which does not enable FEAT_SVA for it's fault capable
>> domains.
>>
>> These APIs are both obsolete, update SMMUv3 to use the new method like AMD
>> implements.
>>
>> A driver should enable iopf support when a domain with an iopf_handler is
>> attached, and disable iopf support when the domain is removed.
>>
>> Move the fault support logic to sva domain allocation and to domain
>> attach, refusing to create or attach fault capable domains if the HW
>> doesn't support it.
>>
>> Move all the logic for controlling the iopf queue under
>> arm_smmu_attach_prepare(). Keep track of the number of domains on the
>> master (over all the SSIDs) that require iopf. When the first domain
>> requiring iopf is attached create the iopf queue, when the last domain is
>> detached destroy it.
>>
>> Turn FEAT_IOPF and FEAT_SVA into no ops.
>>
>> Remove the sva_lock, this is all protected by the group mutex.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> Tested-by: Zhangfei Gao <zhangfei.gao@...aro.org>
>> ---
>> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 86 +-------------
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 105 +++++++++++++-----
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 39 ++-----
>> 3 files changed, 91 insertions(+), 139 deletions(-)
>
> [...]
>
>> @@ -2748,6 +2750,54 @@ to_smmu_domain_devices(struct iommu_domain *domain)
>> return NULL;
>> }
>>
>> +static int arm_smmu_enable_iopf(struct arm_smmu_master *master,
>> + struct arm_smmu_master_domain *master_domain)
>> +{
>> + int ret;
>> +
>> + iommu_group_mutex_assert(master->dev);
>> +
>> + if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
>> + return -EOPNOTSUPP;
>> +
>> + /*
>> + * Drivers for devices supporting PRI or stall require iopf others have
>> + * device-specific fault handlers and don't need IOPF, so this is not a
>> + * failure.
>> + */
>> + if (!master->stall_enabled)
>> + return 0;
>> +
>> + /* We're not keeping track of SIDs in fault events */
>> + if (master->num_streams != 1)
>> + return -EOPNOTSUPP;
>> +
>> + if (master->iopf_refcount) {
>> + master->iopf_refcount++;
>> + master_domain->using_iopf = true;
>> + return 0;
>> + }
>> +
>> + ret = iopf_queue_add_device(master->smmu->evtq.iopf, master->dev);
>> + if (ret)
>> + return ret;
>> + master->iopf_refcount = 1;
>> + master_domain->using_iopf = true;
>> + return 0;
>> +}
>> +
>> +static void arm_smmu_disable_iopf(struct arm_smmu_master *master)
>> +{
>> + iommu_group_mutex_assert(master->dev);
>> +
>> + if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
>> + return;
>
> I think it would be a little cleaner to push the '->using_iopf' check
> in here rather than have the callers check it. Then the SVA check above
> makes more sense and I think the enable/disable paths are a bit more
> symmetric.
Yes, sure. I will address it like this,
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 4aaf9b7bf862..a519854b3798 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2786,13 +2786,17 @@ static int arm_smmu_enable_iopf(struct
arm_smmu_master *master,
return 0;
}
-static void arm_smmu_disable_iopf(struct arm_smmu_master *master)
+static void arm_smmu_disable_iopf(struct arm_smmu_master *master,
+ struct arm_smmu_master_domain
*master_domain)
{
iommu_group_mutex_assert(master->dev);
if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
return;
+ if (!master_domain || !master_domain->using_iopf)
+ return;
+
master->iopf_refcount--;
if (master->iopf_refcount == 0)
iopf_queue_remove_device(master->smmu->evtq.iopf,
master->dev);
@@ -2823,11 +2827,8 @@ static void arm_smmu_remove_master_domain(struct
arm_smmu_master *master,
}
spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
- if (master_domain) {
- if (master_domain->using_iopf)
- arm_smmu_disable_iopf(master);
- kfree(master_domain);
- }
+ arm_smmu_disable_iopf(master, master_domain);
+ kfree(master_domain);
}
/*
@@ -2945,8 +2946,7 @@ int arm_smmu_attach_prepare(struct
arm_smmu_attach_state *state,
return 0;
err_iopf:
- if (master_domain && master_domain->using_iopf)
- arm_smmu_disable_iopf(master);
+ arm_smmu_disable_iopf(master);
err_free_master_domain:
kfree(master_domain);
return ret;
>
> With that:
>
> Acked-by: Will Deacon <will@...nel.org>
>
> Will
Thanks,
baolu
Powered by blists - more mailing lists