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:   Thu, 10 Aug 2023 16:18:54 -0300
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Lu Baolu <baolu.lu@linux.intel.com>
Cc:     Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Kevin Tian <kevin.tian@intel.com>,
        Jean-Philippe Brucker <jean-philippe@linaro.org>,
        Nicolin Chen <nicolinc@nvidia.com>,
        Yi Liu <yi.l.liu@intel.com>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        iommu@lists.linux.dev, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 12/12] iommu: Add helper to set iopf handler for domain
Message-ID: <ZNU4Hio8oAHH8RLn@ziepe.ca>
References: <20230727054837.147050-1-baolu.lu@linux.intel.com>
 <20230727054837.147050-13-baolu.lu@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230727054837.147050-13-baolu.lu@linux.intel.com>
Precedence: bulk

On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote:
> To avoid open code everywhere.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h | 11 ++++++++++-
>  drivers/iommu/iommu.c | 20 ++++++++++++++++++--
>  2 files changed, 28 insertions(+), 3 deletions(-)

Seems like overkill at this point..

Also, I think this is probably upside down.

We want to create the domains as fault enabled in the first place.

A fault enabled domain should never be attached to something that
cannot support faults. It should also not support changing the fault
handler while it exists.

Thus at the creation point would be the time to supply the fault handler
as part of requesting faulting.

Taking an existing domain and making it faulting enabled is going to
be really messy in all the corner cases.

My advice (and Robin will probably hate me), is to define a new op:

struct domain_alloc_paging_args {
       struct fault_handler *fault_handler;
       void *fault_data
};

struct iommu_domain *domain_alloc_paging2(struct device *dev, struct
       domain_alloc_paging_args *args)

The point would be to leave the majority of drivers using the
simplified, core assisted, domain_alloc_paging() interface and they
just don't have to touch any of this stuff at all.

Obviously if handler is given then the domain will be initialized as
faulting.

Jason