Re: [PATCH v3 3/3] iommu/s390: implement iommu passthrough via identity domain

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Jan 24, 2025 at 03:17:17PM -0500, Matthew Rosato wrote:
> Enabled via the kernel command-line 'iommu.passthrough=1' option.
> 
> Introduce the concept of identity domains to s390-iommu, which relies on
> the bus_dma_region to offset identity mappings to the start of the DMA
> aperture advertized by CLP.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>  arch/s390/pci/pci.c        |  6 ++-
>  drivers/iommu/s390-iommu.c | 95 +++++++++++++++++++++++++++++---------
>  2 files changed, 76 insertions(+), 25 deletions(-)

Seems Ok

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

> -static const struct iommu_ops s390_iommu_ops = {
> -	.blocked_domain		= &blocking_domain,
> -	.release_domain		= &blocking_domain,
> -	.capable = s390_iommu_capable,
> -	.domain_alloc_paging = s390_domain_alloc_paging,
> -	.probe_device = s390_iommu_probe_device,
> -	.device_group = generic_device_group,
> -	.pgsize_bitmap = SZ_4K,
> -	.get_resv_regions = s390_iommu_get_resv_regions,
> -	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= s390_iommu_attach_device,
> -		.map_pages	= s390_iommu_map_pages,
> -		.unmap_pages	= s390_iommu_unmap_pages,
> -		.flush_iotlb_all = s390_iommu_flush_iotlb_all,
> -		.iotlb_sync      = s390_iommu_iotlb_sync,
> -		.iotlb_sync_map  = s390_iommu_iotlb_sync_map,
> -		.iova_to_phys	= s390_iommu_iova_to_phys,
> -		.free		= s390_domain_free,
> +#define S390_IOMMU_COMMON_OPS() \
> +	.blocked_domain		= &blocking_domain, \
> +	.release_domain		= &blocking_domain, \
> +	.capable = s390_iommu_capable, \
> +	.domain_alloc_paging = s390_domain_alloc_paging, \
> +	.probe_device = s390_iommu_probe_device, \
> +	.device_group = generic_device_group, \
> +	.pgsize_bitmap = SZ_4K, \
> +	.get_resv_regions = s390_iommu_get_resv_regions, \
> +	.default_domain_ops = &(const struct iommu_domain_ops) { \
> +		.attach_dev	= s390_iommu_attach_device, \
> +		.map_pages	= s390_iommu_map_pages, \
> +		.unmap_pages	= s390_iommu_unmap_pages, \
> +		.flush_iotlb_all = s390_iommu_flush_iotlb_all, \
> +		.iotlb_sync      = s390_iommu_iotlb_sync, \
> +		.iotlb_sync_map  = s390_iommu_iotlb_sync_map, \
> +		.iova_to_phys	= s390_iommu_iova_to_phys, \
> +		.free		= s390_domain_free, \
>  	}
> +
> +static const struct iommu_ops s390_iommu_ops = {
> +	S390_IOMMU_COMMON_OPS()
> +};
> +
> +static const struct iommu_ops s390_iommu_rtr_ops = {
> +	.identity_domain	= &s390_identity_domain,
> +	S390_IOMMU_COMMON_OPS()
>  };

Though it is a pattern in iommu drivers to use a non-cost ops and
mutate them during probe. For instance you could NULL
s390_iommu_rtr_ops->identity_domain if the platform does not support
it.

On the other hand your version with a const ops is security nicer.

Alternatively, I have a patch series that adds a
domain_alloc_identity() function to get virtio-iommu moved to the new
APIs, that could work well here too and keep the const ops.

Jason