-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org]
On Behalf Of Jean-Philippe Brucker
Sent: 02 June 2020 12:46
To: Shameerali Kolothum Thodi <redacted>
Cc: devicetree@vger.kernel.org; kevin.tian@intel.com; fenghua.yu@intel.com;
linux-pci@vger.kernel.org; felix.kuehling@amd.com; robin.murphy@arm.com;
christian.koenig@amd.com; hch@infradead.org; jgg@ziepe.ca;
iommu@lists.linux-foundation.org; catalin.marinas@arm.com;
zhangfei.gao@linaro.org; will@kernel.org; linux-mm@kvack.org;
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support for
platform devices

On Tue, Jun 02, 2020 at 10:31:29AM +0000, Shameerali Kolothum Thodi wrote:

quoted

Hi Jean,

quoted

-----Original Message-----
From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces@lists.infradead.org]
On Behalf Of Jean-Philippe Brucker
Sent: 02 June 2020 10:39
To: Shameerali Kolothum Thodi <redacted>
Cc: devicetree@vger.kernel.org; kevin.tian@intel.com;
will@kernel.org; fenghua.yu@intel.com; jgg@ziepe.ca;
linux-pci@vger.kernel.org; felix.kuehling@amd.com;
hch@infradead.org; linux-mm@kvack.org;
iommu@lists.linux-foundation.org; catalin.marinas@arm.com;
zhangfei.gao@linaro.org; robin.murphy@arm.com;
christian.koenig@amd.com; linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support
for platform devices

Hi Shameer,

On Mon, Jun 01, 2020 at 12:42:15PM +0000, Shameerali Kolothum Thodi
wrote:

quoted

quoted

 /* IRQ and event handlers */
+static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
+u64
+*evt) {
+	int ret;
+	u32 perm = 0;
+	struct arm_smmu_master *master;
+	bool ssid_valid = evt[0] & EVTQ_0_SSV;
+	u8 type = FIELD_GET(EVTQ_0_ID, evt[0]);
+	u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
+	struct iommu_fault_event fault_evt = { };
+	struct iommu_fault *flt = &fault_evt.fault;
+
+	/* Stage-2 is always pinned at the moment */
+	if (evt[1] & EVTQ_1_S2)
+		return -EFAULT;
+
+	master = arm_smmu_find_master(smmu, sid);
+	if (!master)
+		return -EINVAL;
+
+	if (evt[1] & EVTQ_1_READ)
+		perm |= IOMMU_FAULT_PERM_READ;
+	else
+		perm |= IOMMU_FAULT_PERM_WRITE;
+
+	if (evt[1] & EVTQ_1_EXEC)
+		perm |= IOMMU_FAULT_PERM_EXEC;
+
+	if (evt[1] & EVTQ_1_PRIV)
+		perm |= IOMMU_FAULT_PERM_PRIV;
+
+	if (evt[1] & EVTQ_1_STALL) {
+		flt->type = IOMMU_FAULT_PAGE_REQ;
+		flt->prm = (struct iommu_fault_page_request) {
+			.flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
+			.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]),
+			.grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
+			.perm = perm,
+			.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
+		};
+

quoted

+		if (ssid_valid)
+			flt->prm.flags |=

IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;

quoted

Do we need to set this for STALL mode only support? I had an issue
with this being set on a vSVA POC based on our D06 zip
device(which is a "fake " pci dev that supports STALL mode but no
PRI). The issue is, CMDQ_OP_RESUME doesn't have any ssid or SSV
params and works on sid

and stag only.

I don't understand the problem, arm_smmu_page_response() doesn't set
SSID or SSV when sending a CMDQ_OP_RESUME. Could you detail the flow
of a stall event and RESUME command in your prototype?  Are you
getting issues with the host driver or the guest driver?

The issue is on the host side iommu_page_response(). The flow is
something like below.

Stall: Host:-

arm_smmu_handle_evt()
  iommu_report_device_fault()
    vfio_pci_iommu_dev_fault_handler()

Stall: Qemu:-

vfio_dma_fault_notifier_handler()
  inject_faults()
    smmuv3_inject_faults()

Stall: Guest:-

arm_smmu_handle_evt()
  iommu_report_device_fault()
    iommu_queue_iopf
  ...
  iopf_handle_group()
    iopf_handle_single()
      handle_mm_fault()
        iopf_complete()
           iommu_page_response()
             arm_smmu_page_response()
               arm_smmu_cmdq_issue_cmd(CMDQ_OP_RESUME)

Resume: Qemu:-

smmuv3_cmdq_consume(SMMU_CMD_RESUME)
  smmuv3_notify_page_resp()
    vfio:ioctl(page_response)  --> struct iommu_page_response is filled
                             with only version, grpid and code.

Resume: Host:-
  ioctl(page_response)
    iommu_page_response()  --> fails as the pending req has PASID_VALID

flag

quoted

                             set and it checks for a match.

I believe the fix needs to be here. It's also wrong for PRI since not all PCIe
endpoint require a PASID in the page response. Could you try the attached
patch?

Going through the patch, yes, that will definitely fix the issue. But isn't it better if
the request itself indicate whether it expects a response msg with a valid pasid or
not? The response msg can come from userspace as well(vSVA) and if for some reason
doesn't set it for a req that expects pasid then it should be an error, right? In the temp
fix I had, I introduced another flag to indicate the endpoint has PRI support or not and
used that to verify the pasid requirement. But for the PRI case you mentioned 
above, not sure it is easy to get that information or not. May be I am complicating things
here :)

Thanks,
Shameer

Thanks,
Jean

quoted

      arm_smmu_page_response()

Hope the above is clear.

quoted

We do need to forward the SSV flag all the way to the guest driver,
so the guest can find the faulting address space using the SSID.
Once the guest handled the fault, then we don't send the SSID back
to the host as part of the RESUME command.

True, the guest requires SSV flag to handle the page fault. But, as
shown in the flow above, the issue is on the host side
iommu_page_response() where it searches for a matching pending req
based on pasid. Not sure we can bypass that and call
arm_smmu_page_response() directly but then have to delete the pending req

from the list as well.

quoted

Please let me know if there is a better way to handle the host side
page response.

Thanks,
Shameer

quoted

Thanks,
Jean

quoted

Hence, it is difficult for
Qemu SMMUv3 to populate this fields while preparing a page
response. I can see that this flag is being checked in
iopf_handle_single() (patch
04/24) as well. For POC, I used a temp fix[1] to work around this.
Please let

me know your thoughts.

quoted

Thanks,
Shameer

1.
https://github.com/hisilicon/kernel-dev/commit/99ff96146e924055f38
d97a
5897e4becfa378d15

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel