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

[Robin Murphy]

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.

> > > Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > Signed-off-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > > ---
> > >    drivers/iommu/s390-iommu.c | 9 +--------
> > >    1 file changed, 1 insertion(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> > > index 94c444b909bd..6bf23e7830a2 100644
> > > --- a/drivers/iommu/s390-iommu.c
> > > +++ b/drivers/iommu/s390-iommu.c
> > > @@ -12,13 +12,6 @@
> > >    #include <linux/sizes.h>
> > >    #include <asm/pci_dma.h>
> > >    -/*
> > > - * Physically contiguous memory regions can be mapped with 4 KiB alignment,
> > > - * we allow all page sizes that are an order of 4KiB (no special large page
> > > - * support so far).
> > > - */
> > > -#define S390_IOMMU_PGSIZES    (~0xFFFUL)
> > > -
> > >    static const struct iommu_ops s390_iommu_ops;
> > >      struct s390_domain {
> > > @@ -350,7 +343,7 @@ static const struct iommu_ops s390_iommu_ops = {
> > >        .probe_device = s390_iommu_probe_device,
> > >        .release_device = s390_iommu_release_device,
> > >        .device_group = generic_device_group,
> > > -    .pgsize_bitmap = S390_IOMMU_PGSIZES,
> > > +    .pgsize_bitmap = SZ_4K,
> > >        .get_resv_regions = s390_iommu_get_resv_regions,
> > >        .default_domain_ops = &(const struct iommu_domain_ops) {
> > >            .attach_dev    = s390_iommu_attach_device,