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

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
                             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?

Thanks,
Jean

      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.

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/99ff96146e924055f38d97a
5897e4becfa378d15

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