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

Jean-Philippe Brucker
2020-06-02T09:38:54Z

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?

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.

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/99ff96146e924055f38d97a5897e4becfa378d15