Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <372ec042d8fe6eed055c354c561316f7670dfb40.camel@linux.ibm.com>
Subject: Re: [PATCH v2 3/7] s390/pci: Use dma-iommu layer
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Robin Murphy <robin.murphy@arm.com>,
        Matthew Rosato <mjrosato@linux.ibm.com>,
        Gerd Bayer <gbayer@linux.ibm.com>, iommu@lists.linux.dev,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>
Cc:     Pierre Morel <pmorel@linux.ibm.com>, linux-s390@vger.kernel.org,
        borntraeger@linux.ibm.com, hca@linux.ibm.com, gor@linux.ibm.com,
        gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
        svens@linux.ibm.com, linux-kernel@vger.kernel.org,
        Julian Ruess <julianr@linux.ibm.com>
Date:   Mon, 19 Dec 2022 16:17:18 +0100
In-Reply-To: <bc1e4fd5-bf96-f579-8190-fda9b41d1547@arm.com>
References: <20221116171656.4128212-1-schnelle@linux.ibm.com>
         <20221116171656.4128212-4-schnelle@linux.ibm.com>
         <bc1e4fd5-bf96-f579-8190-fda9b41d1547@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.2 (3.46.2-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-11-28 at 18:03 +0000, Robin Murphy wrote:
> On 2022-11-16 17:16, Niklas Schnelle wrote:
> [...]
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index dc5f7a156ff5..804fb8f42d61 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -93,7 +93,7 @@ config IOMMU_DEBUGFS
> >   choice
> >   	prompt "IOMMU default domain type"
> >   	depends on IOMMU_API
> > -	default IOMMU_DEFAULT_DMA_LAZY if X86 || IA64
> > +	default IOMMU_DEFAULT_DMA_LAZY if X86 || IA64 || S390
> >   	default IOMMU_DEFAULT_DMA_STRICT
> >   	help
> >   	  Choose the type of IOMMU domain used to manage DMA API usage by
> > @@ -412,6 +412,7 @@ config ARM_SMMU_V3_SVA
> >   config S390_IOMMU
> >   	def_bool y if S390 && PCI
> >   	depends on S390 && PCI
> > +	select IOMMU_DMA
>=20
> Please add S390 to the condition under config IOMMU_DMA instead.

Done, will be in v3

>=20
> >   	select IOMMU_API
> >   	help
> >   	  Support for the IOMMU API for s390 PCI devices.
> [...]
> > @@ -45,9 +329,14 @@ static struct iommu_domain *s390_domain_alloc(unsig=
ned domain_type)
> >   {
> >   	struct s390_domain *s390_domain;
> >  =20
> > -	if (domain_type !=3D IOMMU_DOMAIN_UNMANAGED)
> > +	switch (domain_type) {
> > +	case IOMMU_DOMAIN_DMA:
> > +	case IOMMU_DOMAIN_DMA_FQ:
>=20
> I was about to question adding this without any visible treatment of=20
> iommu_iotlb_gather_queued() or domain->type =3D=3D IOMMU_DOMAIN_DMA_FQ as=
 a=20
> whole, but I guess if you never actually free any pagetables it does=20
> work out OK. Bit of a timebomb if there's a chance of that ever changing=
=20
> in future, though.

Yes, as we're not freeing pagetables they will be reused simply by
reusing same IOVA. Restricting the range of IOVAs is then what keeps
the translation table from growing unbounded. This also makes the
pagetable handling easier=20

It will definitely need handling if we ever add freeing pagestables
yes. I'd hope though that if we do add this functionality it will be
through common code reuse such as io-pgtable.h that will deal with this
naturally. I'm still pondering to add a comment here but I also fear
that this part is going to be refactored soon and such a comment will
get lost before it could matter.

>=20
> > +	case IOMMU_DOMAIN_UNMANAGED:
> > +		break;
> > +	default:
> >   		return NULL;
> > -
> > +	}
> >   	s390_domain =3D kzalloc(sizeof(*s390_domain), GFP_KERNEL);
> >   	if (!s390_domain)
> >   		return NULL;
> [...]
> > @@ -397,12 +678,29 @@ static size_t s390_iommu_unmap_pages(struct iommu=
_domain *domain,
> >   		return 0;
> >  =20
> >   	iommu_iotlb_gather_add_range(gather, iova, size);
> > +	atomic64_add(pgcount, &s390_domain->ctrs.unmapped_pages);
> >  =20
> >   	return size;
> >   }
> >  =20
> > +static void s390_iommu_probe_finalize(struct device *dev)
> > +{
> > +	struct iommu_domain *domain =3D iommu_get_domain_for_dev(dev);
> > +
> > +	iommu_dma_forcedac =3D true;
> > +	iommu_setup_dma_ops(dev, domain->geometry.aperture_start, domain->geo=
metry.aperture_end);
>=20
> For consistency with all but one other caller now, just pass 0 and=20
> U64_MAX for base and size to make it clear that they're meaningless=20
> (they will eventually go away as part of a bigger refactoring).
>=20
> Thanks,
> Robin.

Done will be in v3.

Now I'm still trying to figure out how to prefer the proposed
IOMMU_DOMAIN_DMA_SQ when running with a hypervisor with expensive IOTLB
flush while IOMMU_DOMAIN_DMA and IOMMU_DOMAIN_DMA_FQ both work but are
slower. Jason's patch idea of adding a DMA_API_POLICY sounds like a
good start but is also a rather big change and doesn't yet allow for a
preference by the IOMMU driver. Will see, if I can come up with
something minimal for that.