Re: "Convert the AMD iommu driver to the dma-iommu api" is buggy

[Robin Murphy]

On 16/10/2019 17:11, Qian Cai wrote:
> On Wed, 2019-10-16 at 18:03 +0200, Joerg Roedel wrote:
> > On Wed, Oct 16, 2019 at 11:53:33AM -0400, Qian Cai wrote:
> > > On Wed, 2019-10-16 at 17:31 +0200, Joerg Roedel wrote:
> > > The x86 one might just be a mistake.
> > >
> > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > > index ad05484d0c80..63c4b894751d 100644
> > > --- a/drivers/iommu/amd_iommu.c
> > > +++ b/drivers/iommu/amd_iommu.c
> > > @@ -2542,7 +2542,7 @@ static int amd_iommu_map(struct iommu_domain *dom,
> > > unsigned long iova,
> > >  ÂÂÂÂÂÂÂÂif (iommu_prot & IOMMU_WRITE)
> > >  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂprot |= IOMMU_PROT_IW;
> > >   
> > > -ÂÂÂÂÂÂÂret = iommu_map_page(domain, iova, paddr, page_size, prot, GFP_KERNEL);
> > > +ÂÂÂÂÂÂÂret = iommu_map_page(domain, iova, paddr, page_size, prot, gfp);
> >
> > Yeah, that is a bug, I spotted that too.
> >
> > > @@ -1185,7 +1185,7 @@ static struct iommu_dma_msi_page
> > > *iommu_dma_get_msi_page(struct device *dev,
> > >  ÂÂÂÂÂÂÂÂif (!iova)
> > >  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto out_free_page;
> > >   
> > > -ÂÂÂÂÂÂÂif (iommu_map(domain, iova, msi_addr, size, prot))
> > > +ÂÂÂÂÂÂÂif (iommu_map_atomic(domain, iova, msi_addr, size, prot))
> > >  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto out_free_iova;
> >
> > Not so sure this is a bug, this code is only about setting up MSIs on
> > ARM. It probably doesn't need to be atomic.
>
> The patch "iommu: Add gfp parameter to iommu_ops::map" does this. It could be
> called from an atomic context as showed in the arm64 call traces,
>
> +int iommu_map(struct iommu_domain *domain, unsigned long iova,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂphys_addr_t paddr, size_t size, int prot)
> +{
> +ÂÂÂÂÂÂÂmight_sleep();
> +ÂÂÂÂÂÂÂreturn __iommu_map(domain, iova, paddr, size, prot, GFP_KERNEL);
> +}

Also note that it's *only* the might_sleep() at issue here - none of the 
arm64 IOMMU drivers have been converted to look at the new gfp argument 
yet, so anything they actually allocate while mapping will still be 
GFP_ATOMIC anyway.

(Carrying that flag down through the whole io-pgtable stack is on my 
to-do list...)

Robin.