Re: [PATCH 20/20] iommu: Convert remaining simple drivers to domain_alloc_paging()

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Mon, 2023-05-01 at 15:03 -0300, Jason Gunthorpe wrote:
> These drivers don't support IOMMU_DOMAIN_DMA, so this commit effectively
> allows them to support that mode.
> 
> The prior work to require default_domains makes this safe because every
> one of these drivers is either compilation incompatible with dma-iommu.c,
> or already establishing a default_domain. In both cases alloc_domain()
> will never be called with IOMMU_DOMAIN_DMA for these drivers so it is safe
> to drop the test.
> 
> Removing these tests clarifies that the domain allocation path is only
> about the functionality of a paging domain and has nothing to do with
> policy of how the paging domain is used for UNMANAGED/DMA/DMA_FQ.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/fsl_pamu_domain.c | 7 ++-----
>  drivers/iommu/msm_iommu.c       | 7 ++-----
>  drivers/iommu/mtk_iommu_v1.c    | 7 ++-----
>  drivers/iommu/omap-iommu.c      | 7 ++-----
>  drivers/iommu/s390-iommu.c      | 7 ++-----
>  drivers/iommu/tegra-gart.c      | 7 ++-----
>  6 files changed, 12 insertions(+), 30 deletions(-)
> 
> 
---8<---
> -static struct iommu_domain *s390_domain_alloc(unsigned domain_type)
> +static struct iommu_domain *s390_domain_alloc_paging(struct device *dev)
>  {
>  	struct s390_domain *s390_domain;
>  
> -	if (domain_type != IOMMU_DOMAIN_UNMANAGED)
> -		return NULL;
> -
>  	s390_domain = kzalloc(sizeof(*s390_domain), GFP_KERNEL);
>  	if (!s390_domain)
>  		return NULL;
> @@ -447,7 +444,7 @@ void zpci_destroy_iommu(struct zpci_dev *zdev)
>  static const struct iommu_ops s390_iommu_ops = {
>  	.default_domain = &s390_iommu_platform_domain,
>  	.capable = s390_iommu_capable,
> -	.domain_alloc = s390_domain_alloc,
> +	.domain_alloc_paging = s390_domain_alloc_paging,

Leaving .domain_alloc unset here leads to an OOPs with your GitHub
branch (iommu_mandatory_default) when I try to use vfio-pci for KVM
pass-through via the following call chain:

...
vfio_group_fops_unl_ioctl()
   vfio_group_ioctl_set_container()
      vfio_container_attach_group()
         iommu_group_claim_dma_owner()
            __iommu_take_dma_ownership()
               __iommu_group_alloc_blocking_domain()
               __iommu_domain_alloc(…, IOMMU_DOMAIN_BLOCKED)

The problem is that in __iommu_domain_alloc() a call to bus->iommu_ops-
>domain_alloc() is attempted for IOMMU_DOMAIN_BLCOKED even if the
function pointer is unset.

So I tried with the obvious fix:

@@ -1947,7 +1948,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
        if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
            bus->iommu_ops->domain_alloc_paging)
                domain = bus->iommu_ops->domain_alloc_paging(dev);
-       else
+       else if (bus->iommu_ops->domain_alloc)
                domain = bus->iommu_ops->domain_alloc(type);
        if (!domain)
                return NULL;

This then uses the fallback of an empty IOMMU_DOMAIN_UNMANAGED and I
get a working device in the guest. Also tried hot unplug where the
device is taken over by the host again. I think with my DMA API
conversion patches we can support blocking domains properly but for a
temporary solution the above may be acceptable.

>  	.probe_device = s390_iommu_probe_device,
>  	.release_device = s390_iommu_release_device,
>  	.device_group = generic_device_group,