Re: [PATCH v2 08/16] iommu: define and export iommu_access_remote_vm()

From: Fenghua Yu <fenghua.yu@intel.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Baolu Lu <baolu.lu@linux.intel.com>,
	Vinod Koul <vkoul@kernel.org>,
	"Dave Jiang" <dave.jiang@intel.com>, <dmaengine@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Alistair Popple <apopple@nvidia.com>,
	"Joerg Roedel" <joro@8bytes.org>, Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Lorenzo Stoakes <lstoakes@gmail.com>, <iommu@lists.linux.dev>
Subject: Re: [PATCH v2 08/16] iommu: define and export iommu_access_remote_vm()
Date: Thu, 30 Mar 2023 17:44:27 -0700	[thread overview]
Message-ID: <c4ff442d-706d-bb00-84af-c991454a37de@intel.com> (raw)
In-Reply-To: <ZBhhEK1V+u2oV2Ll@infradead.org>

Hi, Christoph,

On 3/20/23 06:35, Christoph Hellwig wrote:
> On Tue, Mar 07, 2023 at 09:55:28AM -0800, Fenghua Yu wrote:
>>>
>>> I don't quite follow here. Isn't I/O page fault already supported?
>>
>> The following patch 9 in this series explains in details why IDXD device
>> cannot use page fault to write to user memory: https://lore.kernel.org/dmaengine/20230306163138.587484-10-fenghua.yu@intel.com/
>>
>> "DSA supports page fault handling through PRS. However, the DMA engine
>> that's processing the descriptor is blocked until the PRS response is
>> received. Other workqueues sharing the engine are also blocked.
>> Page fault handing by the driver with PRS disabled can be used to
>> mitigate the stalling.
>>
>> With PRS disabled while ATS remain enabled, DSA handles page faults on
>> a completion record by reporting an event in the event log. In this
>> instance, the descriptor is completed and the event log contains the
>> completion record address and the contents of the completion record."
> 
> This seems like a completely broken I/O model, and I don't think Linux
> should support this model when it requires operations like
> access_remote_vm.

This patch set have two parts:
1. Basic event log support and PRS disabling knob.
2. Completion record page fault fixup part. The current patch is the 
major patch in this part. It tries to fix up completion record page 
fault from user space.

Since the fix up in part 2 is debatable and part 1 can be sent out 
independently, how about we send out the parts separately?

In the new part 1, simply warn on completion record page fault and don't 
try to fix it up. Usually a process executes ENQCMD instruction and then 
keeps polling completion record periodically. So the completion record 
will be likely backed by page and won't generate page fault. If page 
fault is really an issue, sysadmin can enable PRS (which is enabled by 
default anyway) and let PRS to handle page fault.

Then in the next step, we will send out a new part 2 to eliminate 
completion record page fault. One proposal is to mmap() kernel allocated 
completion record area. So there won't be completion record page fault 
and fix up(no access_remote_vm() of course).

Is this OK for you?

Thank you very much for your comment!

-Fenghua