Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Thu, 7 Dec 2023 09:36:30 -0400
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Robin Murphy <robin.murphy@arm.com>
Cc:     Lu Baolu <baolu.lu@linux.intel.com>,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Kevin Tian <kevin.tian@intel.com>, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] iommu: Set owner token to sva and nested domains
Message-ID: <20231207133630.GS1489931@ziepe.ca>
References: <20231207021938.306738-1-baolu.lu@linux.intel.com>
 <f1c473f7-ba40-454e-bd3e-a47f6d58513e@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f1c473f7-ba40-454e-bd3e-a47f6d58513e@arm.com>
Precedence: bulk

On Thu, Dec 07, 2023 at 09:56:10AM +0000, Robin Murphy wrote:
> On 2023-12-07 2:19 am, Lu Baolu wrote:
> > Commit a9c362db3920 ("iommu: Validate that devices match domains") added
> > an owner token to an iommu_domain. This token is checked during domain
> > attachment to RID or PASID through the generic iommu interfaces.
> > 
> > The sva and nested domains are attached to device or PASID through the
> > generic iommu interfaces. Therefore, they require the owner token to be
> > set during allocation. Otherwise, they fail to attach.
> 
> Oops, I missed that iommu_sva_domain_alloc() is a thing - when did we get
> such a confusing proliferation of domain allocation paths? Sigh...

We have alot of different kinds of domains now, APIs that are giant
multiplexers are not good.

What I've been wanting to do for a while is to have the drivers call a
helper to allocate their domain struct and the helper would initialize
the common iommu_domain instead of doing this after the op
returns. This is more typical kernel pattern and avoids some of the
confusion about when struct members are valid or not (notice some of
driver code needs iommu_domain stuff set earlier and we confusingly
initialize things twice :()

> I think we should set the owner generically there, since presumably it's
> being missed for SMMUv3/AMD/etc. SVA domains as well. Nested domains are
> supposed to be OK since both ->domain_alloc_user callsites are covered, or
> is there some other sneaky path I've also missed?

Indeed, I also think the first hunk is not needed, the second hunk was
missed.

Jason