Re: [PATCH v8 11/15] iommufd/viommu: Select vIOMMU operations before allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] iommu/amd: unreferenced amd_viommu_ops after amd_iommufd_get_viommu_ops()
--

commit 7d905a9d1ce6388259ae73f7d0cf19d414cd5da4
Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

iommufd/viommu: Select vIOMMU operations before allocation

This commit adds iommu_ops::get_viommu_ops() to select vIOMMU operations
before allocation and moves get_viommu_size() and viommu_init() into
struct iommufd_viommu_ops across the IOMMUFD core and backend drivers.

> diff --git a/drivers/iommu/amd/iommufd.c b/drivers/iommu/amd/iommufd.c
> index 52300b867c1f2..dcbe80fcb8c70 100644
> --- a/drivers/iommu/amd/iommufd.c
> +++ b/drivers/iommu/amd/iommufd.c
> @@ -32,13 +32,21 @@ void *amd_iommufd_hw_info(struct device *dev, u32 *length, enum iommu_hw_info_ty
>  	return hwinfo;
>  }
>  
> -size_t amd_iommufd_get_viommu_size(struct device *dev, enum iommu_viommu_type viommu_type)
> +static size_t amd_iommufd_get_viommu_size(struct device *dev,
> +		enum iommu_viommu_type viommu_type)
>  {
>  	return VIOMMU_STRUCT_SIZE(struct amd_iommu_viommu, core);
>  }
>  
> -int amd_iommufd_viommu_init(struct iommufd_viommu *viommu, struct iommu_domain *parent,
> -			    const struct iommu_user_data *user_data)
> +const struct iommufd_viommu_ops *
> +amd_iommufd_get_viommu_ops(struct device *dev, enum iommu_viommu_type viommu_type)
> +{
> +	return NULL;
> +}
> +
> +int amd_iommufd_viommu_init(struct iommufd_viommu *viommu,
> +		struct device *dev, struct iommu_domain *parent,
> +		const struct iommu_user_data *user_data)
>  {
>  	unsigned long flags;
>  	struct protection_domain *pdom = to_pdomain(parent);
> @@ -47,8 +55,6 @@ int amd_iommufd_viommu_init(struct iommufd_viommu *viommu, struct iommu_domain *
>  	xa_init_flags(&aviommu->gdomid_array, XA_FLAGS_ALLOC1);
>  	aviommu->parent = pdom;
>  
> -	viommu->ops = &amd_viommu_ops;
> -
>  	spin_lock_irqsave(&pdom->lock, flags);
>  	list_add(&aviommu->pdom_list, &pdom->viommu_list);
>  	spin_unlock_irqrestore(&pdom->lock, flags);
> @@ -73,5 +79,7 @@ static void amd_iommufd_viommu_destroy(struct iommufd_viommu *viommu)
>   * struct iommufd_viommu_ops - vIOMMU specific operations
>   */
>  static const struct iommufd_viommu_ops amd_viommu_ops = {
> +	.get_viommu_size = amd_iommufd_get_viommu_size,
> +	.viommu_init = amd_iommufd_viommu_init,
>  	.destroy = amd_iommufd_viommu_destroy,
>  };
> diff --git a/drivers/iommu/amd/iommufd.h b/drivers/iommu/amd/iommufd.h
> index 62e9e1bebfbe5..0d8e7c0902cff 100644
> --- a/drivers/iommu/amd/iommufd.h
> +++ b/drivers/iommu/amd/iommufd.h
> @@ -8,13 +8,16 @@
>  
>  #if IS_ENABLED(CONFIG_AMD_IOMMU_IOMMUFD)
>  void *amd_iommufd_hw_info(struct device *dev, u32 *length, enum iommu_hw_info_type *type);
> -size_t amd_iommufd_get_viommu_size(struct device *dev, enum iommu_viommu_type viommu_type);
> -int amd_iommufd_viommu_init(struct iommufd_viommu *viommu, struct iommu_domain *parent,
> -			    const struct iommu_user_data *user_data);
> +const struct iommufd_viommu_ops *
> +amd_iommufd_get_viommu_ops(struct device *dev,
> +		enum iommu_viommu_type viommu_type);
> +int amd_iommufd_viommu_init(struct iommufd_viommu *viommu,
> +		struct device *dev, struct iommu_domain *parent,
> +		const struct iommu_user_data *user_data);
>  #else
>  #define amd_iommufd_hw_info NULL
>  #define amd_iommufd_viommu_init NULL
> -#define amd_iommufd_get_viommu_size NULL
> +#define amd_iommufd_get_viommu_ops NULL
>  #endif /* CONFIG_AMD_IOMMU_IOMMUFD */
>  
>  #endif /* AMD_IOMMUFD_H */

[Severity: Low]
With viommu->ops = &amd_viommu_ops removed from amd_iommufd_viommu_init()
and amd_iommufd_get_viommu_ops() unconditionally returning NULL,
amd_viommu_ops in drivers/iommu/amd/iommufd.c has no remaining references.

When building with CONFIG_AMD_IOMMU_IOMMUFD=y, won't this trigger
-Wunused-const-variable warnings on amd_viommu_ops under W=1 (and Clang
-Wunused-function warnings on its static callbacks)?

Also, since amd_iommufd_viommu_init() is no longer referenced from
drivers/iommu/amd/iommu.c, should it be made static in
drivers/iommu/amd/iommufd.c and removed from drivers/iommu/amd/iommufd.h,
with amd_viommu_ops marked __maybe_unused while AMD vIOMMU allocation is
disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104161632.3547423-1-aneesh.kumar@kernel.org?part=11