On Tue, Jun 02, 2020 at 12:12:30PM +0000, Shameerali Kolothum Thodi wrote: > > > > > > + if (ssid_valid) > > > > > > + flt->prm.flags |= > > > > IOMMU_FAULT_PAGE_REQUEST_PASID_VALID; > > > > > > > > > > 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? > > 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
