Received-SPF: pass (google.com: domain of linux-kernel+bounces-30522-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 18 Jan 2024 15:36:27 -0400
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>, Kevin Tian <kevin.tian@intel.com>,
	Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
	iommu@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] iommufd/selftest: Use right iommu_ops for mock device
Message-ID: <20240118193627.GJ50608@ziepe.ca>
References: <20240111073213.180020-1-baolu.lu@linux.intel.com>
 <20240111144840.GW50608@ziepe.ca>
 <016e052e-8a99-4d60-b864-87c9859953f2@arm.com>
 <20240111155648.GX50608@ziepe.ca>
 <e08cd3b3-7ce0-47c9-b2f3-28095987f46b@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e08cd3b3-7ce0-47c9-b2f3-28095987f46b@arm.com>

On Tue, Jan 16, 2024 at 06:19:00PM +0000, Robin Murphy wrote:

> > > As I've tried to explain before, this is in fact the correct use of fwspec
> > > as originally designed, i.e. being set up by *bus code* before device_add()
> > > (remember this is not the "IOMMU driver" part of selftest.c).
> > 
> > I understand it was the intention, but it doesn't relaly match how the
> > code works today..
> 
> The fact that some things aren't following the pattern, and are broken and
> problematic in several ways as a result, does not mean that other things
> that *can* follow the pattern correctly shouldn't.

What pattern? fwspec was never setup "by bus code" before
device_add(). I'm not even sure I see how that will be possible since
fwspec relies on the iommu driver being present to parse the FW tables
to create the fwspec in the first place.

The main tension is that the information the bus code needs to supply
to parse the FW has to go into the driver to accomplish the parse and
then be discarded once the parsing is done. Why would we attach
temporary data to the struct device prior to device_add and waste that
memory when we can generate it on the fly with a bus op callback?

> > > I'm working to get things back to that model (wherein the dev_iommu and
> > > fwspec lifecycles become trivial), just with the slight tweak that these
> > > days it's going to make more sense to have the initialisation factored into
> > > device_add() itself (via iommu_probe_device()), rather than beforehand.
> > 
> > I would prefer to simply remove fwspec as I've already shown patches
> > for. You should give some comment on them.
> 
> You mean the 1600 lines of churn which did nothing to address any real
> problem (but did at least acknowledge so in the cover letter)?

Sometimes it takes cleanup before you can solve the real problem. Just
constantly hacking around the edges often creates an architectural
mess.

Indeed is not quite nothing - it did solve alot of the wonky lifetime
issues throughout and replaces an incomplete abstraction with a
complete one.

Look, put aside your aesthetic distaste and point to something that is
actually fundamentally wrong and can't ever work with what I've
done. I showed everything, so if there is some issue it should be
visible.

Otherwise you should admit it is technically sound, even if you don't
like how it looks.

> I thought I had responded to that, but it must have been one of the
> many drafts which end up getting deleted out of utter
> exasperation. Needless to say, the response was a NAK.

Nope, no reply!

I think you should take a careful and thoughtful look. Given it is
right there ready to go we can be done with this probe mis-ordering
saga in a couple of months.

My plan is to break up the three parsing ways into smaller series and
go ahead with them in stages.

> For the last time, any fwspec lifetime issues are a *symptom* of a
> well-understood problem which exists, and not a problem in
> themselves.

There is more wrong than just the lifetime issues.

> some stuff being carried around in iommu_fwspec that really
> shouldn't need to be, but once probing is properly fixed it will get
> stripped back down to the useful shared abstraction of stored
> firmware data that has always been its true spirit. 

Which is what exactly? What can you put in the fwspec that is actually
shared by every, or even most, drivers? From my complete analysis the
answer is pretty much nothing.

> In the meantime, adding a load more complexity to unabstract it and
> support 2 or 3 different ways of drivers all individually
> open-coding storage of the same data is not helpful now, and even
> less helpful in future.

I think you should look at the series much more closely, because I
don't think that impression can really be justified.

Most drivers had a net LOC reduction and many had a significant
reduction in their probe-time complexity, like apple-dart. Many little
bugs and missing checks went away because the shared common code was
now actually doing what the drivers need.

Like it or not we actually have 4-5 different ways the drivers do
things! Explicitly supporting those ways and factoring common logic
into common code is *good design*. fwspec doesn't do that, it
 supports *one way* and everything else gets no good support.

We already had several different open coded ways for ID storage. That
isn't going away as far as I can see. The one common method, the u32
array of ids, is shorter, faster, and uses less memory. At no
significant complexity cost either.

The cleaner layering between FW parsing and IOMMU driver parsing, I
maintain, makes the whole thing easier to understand as the FW code
layer has a clear API boundary now instead of being messily co-mingled
with iommu code.

Jason