Re: [PATCH v2 1/2] iommu/amd: Enable swiotlb in all cases

[Robin Murphy]

On 2022-04-04 21:47, Mario Limonciello wrote:
> Previously the AMD IOMMU would only enable SWIOTLB in certain
> circumstances:
>   * IOMMU in passthrough mode
>   * SME enabled
>
> This logic however doesn't work when an untrusted device is plugged in
> that doesn't do page aligned DMA transactions.  The expectation is
> that a bounce buffer is used for those transactions.
>
> This fails like this:
>
> swiotlb buffer is full (sz: 4096 bytes), total 0 (slots), used 0 (slots)
>
> That happens because the bounce buffers have been allocated, followed by
> freed during startup but the bounce buffering code expects that all IOMMUs
> have left it enabled.
>
> Remove the criteria to set up bounce buffers on AMD systems to ensure
> they're always available for supporting untrusted devices.

FWIW it's also broken for another niche case where 
iommu_default_passthrough() == false at init, but the user later changes 
a 32-bit device's default domain type to passthrough via sysfs, such 
that it starts needing regular dma-direct bouncing.

Reviewed-by: Robin Murphy <robin.murphy@xxxxxxx>

> Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers")
> Suggested-by: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> v1->v2:
>   * Enable swiotlb for AMD instead of ignoring it for inactive
>
>   drivers/iommu/amd/iommu.c | 7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index a1ada7bff44e..079694f894b8 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1838,17 +1838,10 @@ void amd_iommu_domain_update(struct protection_domain *domain)
>   	amd_iommu_domain_flush_complete(domain);
>   }
>   
> -static void __init amd_iommu_init_dma_ops(void)
> -{
> -	swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0;
> -}
> -
>   int __init amd_iommu_init_api(void)
>   {
>   	int err;
>   
> -	amd_iommu_init_dma_ops();
> -
>   	err = bus_set_iommu(&pci_bus_type, &amd_iommu_ops);
>   	if (err)
>   		return err;