Re: [PATCH v4 5/5] iommu/s390: Fix incorrect pgsize_bitmap

[Niklas Schnelle]

On Wed, 2022-10-05 at 10:53 +0100, Robin Murphy wrote:
> On 2022-10-04 17:13, Niklas Schnelle wrote:
> > On Tue, 2022-10-04 at 16:31 +0100, Robin Murphy wrote:
> > > On 2022-10-04 16:12, Matthew Rosato wrote:
> > > > On 10/4/22 11:02 AM, Robin Murphy wrote:
> > > > > On 2022-10-04 13:07, Niklas Schnelle wrote:
> > > > > > The .pgsize_bitmap property of struct iommu_ops is not a page mask but
> > > > > > rather has a bit set for each size of pages the IOMMU supports. As the
> > > > > > comment correctly pointed out at this moment the code only support 4K
> > > > > > pages so simply use SZ_4K here.
> > > > > 
> > > > > Unless it's already been done somewhere else, you'll want to switch over to the {map,unmap}_pages() interfaces as well to avoid taking a hit on efficiency here. The "page mask" thing was an old hack to trick the core API into making fewer map/unmap calls where the driver could map arbitrary numbers of pages at once anyway. The multi-page interfaces now do that more honestly and generally better (since they work for non-power-of-two sizes as well).
> > > > 
> > > > Thanks for the heads up -- Niklas has some additional series coming soon as described here:
> > > > 
> > > > https://lore.kernel.org/linux-iommu/a10424adbe01a0fd40372cbd0736d11e517951a1.camel@xxxxxxxxxxxxx/
> > > > 
> > > > So implementing the _pages() interfaces is soon up on the roadmap.  But given what you say I wonder if this patch should just wait until the series that implements {map,unmap}_pages().
> > > 
> > > Perhaps, although the full change should be trivial enough that there's
> > > probably just as much argument for doing the whole thing in its own
> > > right for the sake of this cleanup. The main point is that
> > > S390_IOMMU_PGSIZES is not incorrect as such, it's just not spelling out
> > > the deliberate trick that it's achieving - everyone copied it from
> > > intel-iommu, but since that got converted to the new interfaces the
> > > original explanation is now gone. The only effect of "fixing" it in
> > > isolation right now will be to make large VFIO mappings slower.
> > > 
> > > Robin.
> > 
> > The patch changing to map_pages()/unmap_pages() is currently part of a
> > larger series of improvements, some of which are less trivial. So I'm
> > planning to send those as RFC first. Those include changing the
> > spin_lock protected list to RCU so the map/unmap can paralellize
> > better. Another one is atomic updates to the IOMMU tables to do away
> > with locks in map/unmap. So I think pulling that whole
> > series into this one isn't ideal. I could pull just the
> > map_pages()/unmap_pages() change though.
> 
> Yeah, literally just updating the s390_iommu_{map,unmap} function 
> prototypes and replacing "size" with "pgsize * count" within is all 
> that's needed to clean up this hack properly. That can (and probably 
> should) be completely independent of other improvements deeper down.
> 
> Thanks,
> Robin.
> 

Pretty much, it's a bit cleaner to slightly change
s390_iommu_update_trans() to take pgcount as argument since that
currently calculates the pgcount from the size anyway which is
redundant if we have a pgcount already but that's redudant if we have
the pgcount already. But yes it's all pretty simple and I reordered
things for v5 already.

Speaking of v5, if that were the final form, what do you think would be
the best tree to take it? Except for patch 1 they depend on the removal
of the bus_next field in struct zpci_dev. That commit is not yet in
Linus' tree but already in the s390 feature branch on git.kernel.org so
if these changes were to go via the s390 tree that would be taken care
of. Otherwise one would have to merge that tree first. Or as an
alternative I also have a kernel.org account and can provide this
series as a GPG signed branch based on the s390 tree.