Re: [PATCH v5 2/9] iommufd: Support a HWPT without an iommu driver for noiommu

[Baolu Lu]

On 5/12/26 02:41, Jacob Pan wrote:
> From: Jason Gunthorpe<jgg@xxxxxxxxxx>
>
> Create just a little part of a real iommu driver, enough to
> slot in under the dev_iommu_ops() and allow iommufd to call
> domain_alloc_paging_flags() and fail everything else.
>
> This allows explicitly creating a HWPT under an IOAS.
>
> A new Kconfig option IOMMUFD_NOIOMMU is introduced to differentiate
> from the VFIO group/container based noiommu mode.
>
> Signed-off-by: Jason Gunthorpe<jgg@xxxxxxxxxx>
> Signed-off-by: Jacob Pan<jacob.pan@xxxxxxxxxxxxxxxxxxx>
> ---
> v5:
>     - Use the new IOMMUFD_NOIOMMU Kconfig instead of VFIO_NOIOMMU
>     - Use consistent wording referring to VFIO noiommu mode (Kevin)
>     - Copyright date fix (Kevin)
> v4:
>     - Make iommufd_noiommu_ops const
> v3:
>     - Add comment to explain the design difference over the
>       legacy noiommu VFIO code.
> ---
>   drivers/iommu/iommufd/Kconfig           |  13 +++
>   drivers/iommu/iommufd/Makefile          |   1 +
>   drivers/iommu/iommufd/hw_pagetable.c    |  15 +++-
>   drivers/iommu/iommufd/hwpt_noiommu.c    | 102 ++++++++++++++++++++++++
>   drivers/iommu/iommufd/iommufd_private.h |   2 +
>   5 files changed, 131 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/iommu/iommufd/hwpt_noiommu.c
>
> diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
> index 455bac0351f2..74d6ea5b5b3b 100644
> --- a/drivers/iommu/iommufd/Kconfig
> +++ b/drivers/iommu/iommufd/Kconfig
> @@ -16,6 +16,19 @@ config IOMMUFD
>   	  If you don't know what to do here, say N.
>   
>   if IOMMUFD
> +config IOMMUFD_NOIOMMU
> +	bool
> +	depends on !GENERIC_ATOMIC64 # IOMMU_PT_AMDV1 requires cmpxchg64
> +	select GENERIC_PT
> +	select IOMMU_PT
> +	select IOMMU_PT_AMDV1
> +	help
> +	  Provides a SW-only IO page table for devices without hardware
> +	  IOMMU backing. This uses the AMDV1 page table format for
> +	  IOVA-to-PA lookups only, not for hardware DMA translation.
> +
> +	  Selected by VFIO_CDEV_NOIOMMU. Not intended to be enabled directly.
> +
>   config IOMMUFD_VFIO_CONTAINER
>   	bool "IOMMUFD provides the VFIO container /dev/vfio/vfio"
>   	depends on VFIO_GROUP && !VFIO_CONTAINER
> diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
> index 71d692c9a8f4..67207914bb6e 100644
> --- a/drivers/iommu/iommufd/Makefile
> +++ b/drivers/iommu/iommufd/Makefile
> @@ -10,6 +10,7 @@ iommufd-y := \
>   	vfio_compat.o \
>   	viommu.o
>   
> +iommufd-$(CONFIG_IOMMUFD_NOIOMMU) += hwpt_noiommu.o
>   iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o
>   
>   obj-$(CONFIG_IOMMUFD) += iommufd.o
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index fe789c2dc0c9..0ae14cd3fc72 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -8,6 +8,15 @@
>   #include "../iommu-priv.h"
>   #include "iommufd_private.h"
>   
> +static const struct iommu_ops *get_iommu_ops(struct iommufd_device *idev)
> +{
> +	if (IS_ENABLED(CONFIG_IOMMUFD_NOIOMMU) && !idev->igroup->group)
> +		return &iommufd_noiommu_ops;
> +	if (WARN_ON_ONCE(!idev->dev->iommu))
> +		return NULL;
> +	return dev_iommu_ops(idev->dev);
> +}
> +
>   static void __iommufd_hwpt_destroy(struct iommufd_hw_pagetable *hwpt)
>   {
>   	if (hwpt->domain)
> @@ -114,11 +123,13 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>   				IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
>   				IOMMU_HWPT_FAULT_ID_VALID |
>   				IOMMU_HWPT_ALLOC_PASID;
> -	const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
> +	const struct iommu_ops *ops = get_iommu_ops(idev);
>   	struct iommufd_hwpt_paging *hwpt_paging;
>   	struct iommufd_hw_pagetable *hwpt;
>   	int rc;
>   
> +	if (!ops)
> +		return ERR_PTR(-ENODEV);

Nit: it's unnecessary to add this check. get_iommu_ops() will WARN if
ops is NULL. The previous code assumes that ops is always valid and does
not return here. If you want to change this behavior, it’s better to
explain the reasoning in the commit message.

Otherwise it looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>

Thanks,
baolu