RE: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support for platform devices

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----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 <shameerali.kolothum.thodi@huawei.com>
> 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:
> > Hi Jean,
> >
> > > -----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 <shameerali.kolothum.thodi@huawei.com>
> > > 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:
> > > > >  /* 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]),
> > > > > +		};
> > > > > +
> > > >
> > > > > +		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 :)

Thanks,
Shameer

> Thanks,
> Jean
> 
> >       arm_smmu_page_response()
> >
> > Hope the above is clear.
> >
> > > 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
> >
> > > Thanks,
> > > Jean
> > >
> > > > 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.
> > > >
> > > > 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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu