| 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: <aIDlsUvF2Xbdelvx@google.com>
Date: Wed, 23 Jul 2025 13:37:53 +0000
From: Pranjal Shrivastava <praan@...gle.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: jgg@...dia.com, will@...nel.org, joro@...tes.org, robin.murphy@....com,
linux-arm-kernel@...ts.infradead.org, iommu@...ts.linux.dev,
linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Replace vsmmu_size/type with
get_viommu_size
On Mon, Jul 21, 2025 at 01:04:44PM -0700, Nicolin Chen wrote:
> It's more flexible to have a get_viommu_size op. Replace static vsmmu_size
> and vsmmu_type with that.
>
> Suggested-by: Will Deacon <will@...nel.org>
> Acked-by: Will Deacon <will@...nel.org>
> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> ---
> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 8 ++------
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++--
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +--
> drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 14 ++++++++++++--
> 4 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> index c034d6c5468f..8cd8929bbfdf 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -423,10 +423,9 @@ size_t arm_smmu_get_viommu_size(struct device *dev,
> if (viommu_type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
> return VIOMMU_STRUCT_SIZE(struct arm_vsmmu, core);
>
> - if (!smmu->impl_ops || !smmu->impl_ops->vsmmu_size ||
> - viommu_type != smmu->impl_ops->vsmmu_type)
> + if (!smmu->impl_ops || !smmu->impl_ops->get_viommu_size)
> return 0;
> - return smmu->impl_ops->vsmmu_size;
> + return smmu->impl_ops->get_viommu_size(viommu_type);
> }
>
> int arm_vsmmu_init(struct iommufd_viommu *viommu,
> @@ -451,9 +450,6 @@ int arm_vsmmu_init(struct iommufd_viommu *viommu,
> return 0;
> }
>
> - /* Unsupported type was rejected in arm_smmu_get_viommu_size() */
> - if (WARN_ON(viommu->type != smmu->impl_ops->vsmmu_type))
> - return -EOPNOTSUPP;
> return smmu->impl_ops->vsmmu_init(vsmmu, user_data);
> }
>
> 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 9f4ad3705801..f56113107c8a 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -4716,8 +4716,8 @@ static struct arm_smmu_device *arm_smmu_impl_probe(struct arm_smmu_device *smmu)
>
> ops = new_smmu->impl_ops;
> if (ops) {
> - /* vsmmu_size and vsmmu_init ops must be paired */
> - if (WARN_ON(!ops->vsmmu_size != !ops->vsmmu_init)) {
> + /* get_viommu_size and vsmmu_init ops must be paired */
> + if (WARN_ON(!ops->get_viommu_size != !ops->vsmmu_init)) {
> ret = -EINVAL;
> goto err_remove;
> }
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 3fa02c51df9f..e332f5ba2f8a 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -728,8 +728,7 @@ struct arm_smmu_impl_ops {
> */
> void *(*hw_info)(struct arm_smmu_device *smmu, u32 *length,
> enum iommu_hw_info_type *type);
> - const size_t vsmmu_size;
> - const enum iommu_viommu_type vsmmu_type;
> + size_t (*get_viommu_size)(enum iommu_viommu_type viommu_type);
> int (*vsmmu_init)(struct arm_vsmmu *vsmmu,
> const struct iommu_user_data *user_data);
> };
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index 4c86eacd36b1..46005ed52bc2 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -832,6 +832,13 @@ static void *tegra241_cmdqv_hw_info(struct arm_smmu_device *smmu, u32 *length,
> return info;
> }
>
> +static size_t tegra241_cmdqv_get_vintf_size(enum iommu_viommu_type viommu_type)
> +{
> + if (viommu_type != IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV)
> + return 0;
> + return VIOMMU_STRUCT_SIZE(struct tegra241_vintf, vsmmu.core);
> +}
> +
> static struct arm_smmu_impl_ops tegra241_cmdqv_impl_ops = {
> /* For in-kernel use */
> .get_secondary_cmdq = tegra241_cmdqv_get_cmdq,
> @@ -839,8 +846,7 @@ static struct arm_smmu_impl_ops tegra241_cmdqv_impl_ops = {
> .device_remove = tegra241_cmdqv_remove,
> /* For user-space use */
> .hw_info = tegra241_cmdqv_hw_info,
> - .vsmmu_size = VIOMMU_STRUCT_SIZE(struct tegra241_vintf, vsmmu.core),
> - .vsmmu_type = IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV,
> + .get_viommu_size = tegra241_cmdqv_get_vintf_size,
> .vsmmu_init = tegra241_cmdqv_init_vintf_user,
> };
>
> @@ -1273,6 +1279,10 @@ tegra241_cmdqv_init_vintf_user(struct arm_vsmmu *vsmmu,
> phys_addr_t page0_base;
> int ret;
>
> + /* Unsupported type was rejected in tegra241_cmdqv_get_vintf_size() */
> + if (WARN_ON(vsmmu->core.type != IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV))
> + return -EOPNOTSUPP;
> +
Nit: I don't think we'd expect a call to this if the vintf_size returned
0? I see that in iommufd_viommu_alloc_ioctl, we already have a check:
viommu_size = ops->get_viommu_size(idev->dev, cmd->type);
if (!viommu_size) {
rc = -EOPNOTSUPP;
goto out_put_idev;
}
And call ops->viommu_init only when the above isn't met. Thus,
if we still end up calling ops->viommu_init, shouldn't we BUG_ON() it?
I'd rather have the core code handle such things (since the driver is
simply implementing the ops) and BUG_ON() something that's terribly
wrong..
I can't see any ops->viommu_init being called elsewhere atm, let me
know if there's a different path that I missed..
Apart from the above nit,
Reviewed-by: Pranjal Shrivastava <praan@...gle.com>
> if (!user_data)
> return -EINVAL;
>
> --
> 2.43.0
>
>
Powered by blists - more mailing lists