On Tue, Jun 02, 2020 at 12:12:30PM +0000, Shameerali Kolothum Thodi wrote:

quoted

quoted

quoted

quoted

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 :)

No you're right, we shouldn't send back malformed responses to the SMMU. I
suppose we can store a flag "PASID required" in the fault and check that
against the response. If we have to discard the guest's response, then we
can either fake a response (abort the stall) right away, or wait for the
response timeout to kick, which will do the same.

Thanks,
Jean