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;
Date:   Sun, 30 Apr 2023 20:06:30 -0300
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        linux-kernel@vger.kernel.org, iommu@lists.linux.dev
Subject: Re: [git pull] IOMMU Updates for Linux v6.4
Message-ID: <ZE70doFi8X3KgfrV@ziepe.ca>
References: <ZE5NR5Ml8I2/Ze0f@8bytes.org>
 <CAHk-=wiriLmq6OgLLF9seANqCJqjCrgUC384zcJUFtv3xJgVkQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHk-=wiriLmq6OgLLF9seANqCJqjCrgUC384zcJUFtv3xJgVkQ@mail.gmail.com>
Precedence: bulk

On Sun, Apr 30, 2023 at 01:07:45PM -0700, Linus Torvalds wrote:
> On Sun, Apr 30, 2023 at 4:13 AM Joerg Roedel <joro@8bytes.org> wrote:
> >
> > this pull-request is somewhat messier than usual because it has a lot of
> > conflicts with your tree. I resolved them in a test-merge and sorted it out
> > for you to compare your solution to mine (mine is also mostly similar to
> > the one in linux-next).
> 
> Your resolution is different from mine.
> 
> Some of it is just white-space differences etc, but some of it is meaningful.
> 
> For example, you have
> 
>                 if (mm->pasid < min || mm->pasid >= max)
> 
> in your iommu_sva_alloc_pasid(), which seems to have undone the change
> in commit 4e14176ab13f ("iommu/sva: Stop using ioasid_set for SVA"),
> which changed it to check for
> 
>            .. mm->pasid > max)
> 
> instead (which seems also consistent with what ida_alloc_range() does:
> 'max' is inclusive).

Yes, that is what the new function comment says it should do, and the
only caller is:

drivers/iommu/iommu-sva.c:      ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1);

Which looks inclusive also

> You also seem to have kept the deleted <linux/ioasid.h> header file.

Should be deleted
 
> I'm also a bit unsure about what the intent with mm_valid_pasid() is.
> In commit cd3891158a77 ("iommu/sva: Move PASID helpers to sva code")
> that helper (under the previous name) got moved to a different header
> file, but in the process it also got done unconditionally as

I think the whole thing was originally a micro-optimization to remove
this if statement from some mm paths..

> But in your merge, you ended up splitting it into two versions again.
> 
> I don't think that's technically the "right" merge (it basically
> changes things wrt the two branches), but I do think it's nicer.

It is closer to the intent, I think

> Finally, I'm not happy with the Kconfig situation here. Commit
> 99b5726b4423 ("iommu: Remove ioasid infrastructure") removed
> CONFIG_IOASID, but left the
> 
>         select IOASID
 
> in the 'config INTEL_IOMMU' Kconfig case. I removed that as dead, but
> now we have that
> 
>         select IOMMU_SVA

We've had this longstanding confusion in the iommu layer that SVA and
PASID are one and the same thing, we are slowly reorganizing it.. For
now it is fine for IOMMU_SVA to cover the PASID allocator as the only
drivers that support PASID also support SVA.

Arguably the design is backwards and IOMMU_SVA should be user
selectable and it should turn off the SVA code entirely including the
driver code.

> Somebody should double-check my result, in other words.

I didn't notice anything wrong, I'm sure Lu and Yi will test it!

Thanks,
Jason