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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250128173002.GE1524382@ziepe.ca>
Date: Tue, 28 Jan 2025 13:30:02 -0400
From: Jason Gunthorpe <jgg@...pe.ca>
To: Matthew Rosato <mjrosato@...ux.ibm.com>
Cc: joro@...tes.org, will@...nel.org, robin.murphy@....com,
	gerald.schaefer@...ux.ibm.com, schnelle@...ux.ibm.com,
	hca@...ux.ibm.com, gor@...ux.ibm.com, agordeev@...ux.ibm.com,
	svens@...ux.ibm.com, borntraeger@...ux.ibm.com,
	farman@...ux.ibm.com, clegoate@...hat.com, iommu@...ts.linux.dev,
	linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org
Subject: Re: [PATCH v3 3/3] iommu/s390: implement iommu passthrough via
 identity domain

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@...ux.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@...dia.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