RE: [PATCH v2 39/40] iommu/arm-smmu-v3: Add support for PRI

Bharat Kumar Gogada
2018-05-25T14:08:11Z

>
> For PCI devices that support it, enable the PRI capability and handle PRI Page
> Requests with the generic fault handler. It is enabled on demand by
> iommu_sva_device_init().
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker-5wv7dgnIgG8@public.gmane.org>
>
> ---
> v1->v2:
> * Terminate the page request and disable PRI if no handler is registered
> * Enable and disable PRI in sva_device_init/shutdown, instead of
> add/remove_device
> ---
> drivers/iommu/arm-smmu-v3.c | 192 +++++++++++++++++++++++++++-------
> --
> 1 file changed, 145 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 6cb69ace371b..0edbb8d19579 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -248,6 +248,7 @@
> #define STRTAB_STE_1_S1COR GENMASK_ULL(5, 4)
> #define STRTAB_STE_1_S1CSH GENMASK_ULL(7, 6)
>
> +#define STRTAB_STE_1_PPAR (1UL << 18)
> #define STRTAB_STE_1_S1STALLD (1UL << 27)
>
> #define STRTAB_STE_1_EATS GENMASK_ULL(29, 28)
> @@ -309,6 +310,9 @@
> #define CMDQ_PRI_0_SID GENMASK_ULL(63, 32)
> #define CMDQ_PRI_1_GRPID GENMASK_ULL(8, 0)
> #define CMDQ_PRI_1_RESP GENMASK_ULL(13, 12)
> +#define CMDQ_PRI_1_RESP_FAILURE
> FIELD_PREP(CMDQ_PRI_1_RESP, 0UL)
> +#define CMDQ_PRI_1_RESP_INVALID
> FIELD_PREP(CMDQ_PRI_1_RESP, 1UL)
> +#define CMDQ_PRI_1_RESP_SUCCESS
> FIELD_PREP(CMDQ_PRI_1_RESP, 2UL)
>
> #define CMDQ_RESUME_0_SID GENMASK_ULL(63, 32)
> #define CMDQ_RESUME_0_ACTION_RETRY (1UL << 12)
> @@ -383,12 +387,6 @@ module_param_named(disable_ats_check,
> disable_ats_check, bool, S_IRUGO);
> MODULE_PARM_DESC(disable_ats_check,
> "By default, the SMMU checks whether each incoming transaction
> marked as translated is allowed by the stream configuration. This option
> disables the check.");
>
> -enum pri_resp {
> - PRI_RESP_DENY = 0,
> - PRI_RESP_FAIL = 1,
> - PRI_RESP_SUCC = 2,
> -};
> -
> enum arm_smmu_msi_index {
> EVTQ_MSI_INDEX,
> GERROR_MSI_INDEX,
> @@ -471,7 +469,7 @@ struct arm_smmu_cmdq_ent {
> u32 sid;
> u32 ssid;
> u16 grpid;
> - enum pri_resp resp;
> + enum page_response_code resp;
> } pri;
>
> #define CMDQ_OP_RESUME 0x44
> @@ -556,6 +554,7 @@ struct arm_smmu_strtab_ent {
> struct arm_smmu_s2_cfg *s2_cfg;
>
> bool can_stall;
> + bool prg_resp_needs_ssid;
> };
>
> struct arm_smmu_strtab_cfg {
> @@ -907,14 +906,18 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd,
> struct arm_smmu_cmdq_ent *ent)
> cmd[0] |= FIELD_PREP(CMDQ_PRI_0_SID, ent->pri.sid);
> cmd[1] |= FIELD_PREP(CMDQ_PRI_1_GRPID, ent->pri.grpid);
> switch (ent->pri.resp) {
> - case PRI_RESP_DENY:
> - case PRI_RESP_FAIL:
> - case PRI_RESP_SUCC:
> + case IOMMU_PAGE_RESP_FAILURE:
> + cmd[1] |= CMDQ_PRI_1_RESP_FAILURE;
> + break;
> + case IOMMU_PAGE_RESP_INVALID:
> + cmd[1] |= CMDQ_PRI_1_RESP_INVALID;
> + break;
> + case IOMMU_PAGE_RESP_SUCCESS:
> + cmd[1] |= CMDQ_PRI_1_RESP_SUCCESS;
> break;
> default:
> return -EINVAL;
> }
> - cmd[1] |= FIELD_PREP(CMDQ_PRI_1_RESP, ent->pri.resp);
> break;
> case CMDQ_OP_RESUME:
> cmd[0] |= FIELD_PREP(CMDQ_RESUME_0_SID, ent-
> >resume.sid); @@ -1114,8 +1117,15 @@ static int
> arm_smmu_page_response(struct device *dev,
> cmd.resume.sid = sid;
> cmd.resume.stag = resp->page_req_group_id;
> cmd.resume.resp = resp->resp_code;
> + } else if (master->can_fault) {
> + cmd.opcode = CMDQ_OP_PRI_RESP;
> + cmd.substream_valid = resp->pasid_present &&
> + master->ste.prg_resp_needs_ssid;
> + cmd.pri.sid = sid;
> + cmd.pri.ssid = resp->pasid;
> + cmd.pri.grpid = resp->page_req_group_id;
> + cmd.pri.resp = resp->resp_code;
> } else {
> - /* TODO: put PRI response here */
> return -ENODEV;
> }
>
> @@ -1236,6 +1246,9 @@ static void arm_smmu_write_strtab_ent(struct
> arm_smmu_device *smmu, u32 sid,
> FIELD_PREP(STRTAB_STE_1_S1CSH,
> ARM_SMMU_SH_ISH) |
> FIELD_PREP(STRTAB_STE_1_STRW, strw));
>
> + if (ste->prg_resp_needs_ssid)
> + dst[1] |= STRTAB_STE_1_PPAR;
> +
> if (smmu->features & ARM_SMMU_FEAT_STALLS &&
> !(smmu->features & ARM_SMMU_FEAT_STALL_FORCE) &&
> !ste->can_stall)
> @@ -1471,39 +1484,54 @@ static irqreturn_t arm_smmu_evtq_thread(int
> irq, void *dev)
>
> static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64
> *evt) {
> - u32 sid, ssid;
> - u16 grpid;
> - bool ssv, last;
> -
> - sid = FIELD_GET(PRIQ_0_SID, evt[0]);
> - ssv = FIELD_GET(PRIQ_0_SSID_V, evt[0]);
> - ssid = ssv ? FIELD_GET(PRIQ_0_SSID, evt[0]) : 0;
> - last = FIELD_GET(PRIQ_0_PRG_LAST, evt[0]);
> - grpid = FIELD_GET(PRIQ_1_PRG_IDX, evt[1]);
> -
> - dev_info(smmu->dev, "unexpected PRI request received:\n");
> - dev_info(smmu->dev,
> - "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access
> at iova 0x%016llx\n",
> - sid, ssid, grpid, last ? "L" : "",
> - evt[0] & PRIQ_0_PERM_PRIV ? "" : "un",
> - evt[0] & PRIQ_0_PERM_READ ? "R" : "",
> - evt[0] & PRIQ_0_PERM_WRITE ? "W" : "",
> - evt[0] & PRIQ_0_PERM_EXEC ? "X" : "",
> - evt[1] & PRIQ_1_ADDR_MASK);
> -
> - if (last) {
> - struct arm_smmu_cmdq_ent cmd = {
> - .opcode =
> CMDQ_OP_PRI_RESP,
> - .substream_valid = ssv,
> - .pri = {
> - .sid = sid,
> - .ssid = ssid,
> - .grpid = grpid,
> - .resp = PRI_RESP_DENY,
> - },
> + u32 sid = FIELD_PREP(PRIQ_0_SID, evt[0]);
> +
> + struct arm_smmu_master_data *master;
> + struct iommu_fault_event fault = {
> + .type = IOMMU_FAULT_PAGE_REQ,
> + .last_req = FIELD_GET(PRIQ_0_PRG_LAST,
> evt[0]),
> + .pasid_valid = FIELD_GET(PRIQ_0_SSID_V, evt[0]),
> + .pasid = FIELD_GET(PRIQ_0_SSID, evt[0]),
> + .page_req_group_id = FIELD_GET(PRIQ_1_PRG_IDX,
> evt[1]),
> + .addr = evt[1] & PRIQ_1_ADDR_MASK,
> + };
> +
> + if (evt[0] & PRIQ_0_PERM_READ)
> + fault.prot |= IOMMU_FAULT_READ;
> + if (evt[0] & PRIQ_0_PERM_WRITE)
> + fault.prot |= IOMMU_FAULT_WRITE;
> + if (evt[0] & PRIQ_0_PERM_EXEC)
> + fault.prot |= IOMMU_FAULT_EXEC;
> + if (evt[0] & PRIQ_0_PERM_PRIV)
> + fault.prot |= IOMMU_FAULT_PRIV;
> +
> + /* Discard Stop PASID marker, it isn't used */
> + if (!(fault.prot & (IOMMU_FAULT_READ|IOMMU_FAULT_WRITE)) &&
> + fault.last_req)
> + return;
> +
> + master = arm_smmu_find_master(smmu, sid);
> + if (WARN_ON(!master))
> + return;
> +
> + if (iommu_report_device_fault(master->dev, &fault)) {
> + /*
> + * No handler registered, so subsequent faults won't produce
> + * better results. Try to disable PRI.
> + */
> + struct page_response_msg page_response = {
> + .addr = fault.addr,
> + .pasid = fault.pasid,
> + .pasid_present = fault.pasid_valid,
> + .page_req_group_id = fault.page_req_group_id,
> + .resp_code =
> IOMMU_PAGE_RESP_FAILURE,
> };
>
> - arm_smmu_cmdq_issue_cmd(smmu, &cmd);
> + dev_warn(master->dev,
> + "PPR 0x%x:0x%llx 0x%x: nobody cared, disabling
> PRI\n",
> + fault.pasid_valid ? fault.pasid : 0, fault.addr,
> + fault.prot);
> + arm_smmu_page_response(master->dev, &page_response);
> }
> }
>
> @@ -1529,6 +1557,11 @@ static irqreturn_t arm_smmu_priq_thread(int irq,
> void *dev)
> }
>
> if (queue_sync_prod(q) == -EOVERFLOW)
> + /*
> + * TODO: flush pending faults, since the SMMU might
> have
> + * auto-responded to the Last request of a pending
> + * group
> + */
> dev_err(smmu->dev, "PRIQ overflow detected --
> requests lost\n");
> } while (!queue_empty(q));
>
> @@ -1577,7 +1610,8 @@ static int arm_smmu_flush_queues(void *cookie,
> struct device *dev)
> master = dev->iommu_fwspec->iommu_priv;
> if (master->ste.can_stall)
> arm_smmu_flush_queue(smmu, &smmu->evtq.q,
> "evtq");
> - /* TODO: add support for PRI */
> + else if (master->can_fault)
> + arm_smmu_flush_queue(smmu, &smmu->priq.q,
> "priq");
> return 0;
> }
>
> @@ -2301,6 +2335,59 @@ arm_smmu_iova_to_phys(struct iommu_domain
> *domain, dma_addr_t iova)
> return ops->iova_to_phys(ops, iova);
> }
>
> +static int arm_smmu_enable_pri(struct arm_smmu_master_data *master) {
> + int ret, pos;
> + struct pci_dev *pdev;
> + /*
> + * TODO: find a good inflight PPR number. We should divide the PRI
> queue
> + * by the number of PRI-capable devices, but it's impossible to know
> + * about current and future (hotplugged) devices. So we're at risk of
> + * dropping PPRs (and leaking pending requests in the FQ).
> + */
> + size_t max_inflight_pprs = 16;
> + struct arm_smmu_device *smmu = master->smmu;
> +
> + if (!(smmu->features & ARM_SMMU_FEAT_PRI) ||
> !dev_is_pci(master->dev))
> + return -ENOSYS;
> +
> + pdev = to_pci_dev(master->dev);
> +
> + ret = pci_reset_pri(pdev);
> + if (ret)
> + return ret;
> +
> + ret = pci_enable_pri(pdev, max_inflight_pprs);
> + if (ret) {
> + dev_err(master->dev, "cannot enable PRI: %d\n", ret);
> + return ret;
> + }
> +
> + master->can_fault = true;
> + master->ste.prg_resp_needs_ssid =
> pci_prg_resp_requires_prefix(pdev);

Any reason why this is not cleared in arm_smmu_disable_pri ?

> +
> + dev_dbg(master->dev, "enabled PRI\n");
> +
> + return 0;
> +}
> +
> +static void arm_smmu_disable_pri(struct arm_smmu_master_data
> *master) {
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(master->dev))
> + return;
> +
> + pdev = to_pci_dev(master->dev);
> +
> + if (!pdev->pri_enabled)
> + return;
> +
> + pci_disable_pri(pdev);
> + dev_dbg(master->dev, "disabled PRI\n");
> + master->can_fault = false;
> +}
> +
> static int arm_smmu_sva_init(struct device *dev, struct iommu_sva_param
> *param) {
> int ret;
> @@ -2314,11 +2401,15 @@ static int arm_smmu_sva_init(struct device
> *dev, struct iommu_sva_param *param)
> return -EINVAL;
>
> if (param->features & IOMMU_SVA_FEAT_IOPF) {
> - if (!master->can_fault)
> - return -EINVAL;
> + arm_smmu_enable_pri(master);
> + if (!master->can_fault) {
> + ret = -ENODEV;
> + goto err_disable_pri;
> + }
> +
> ret = iopf_queue_add_device(master->smmu->iopf_queue,
> dev);
> if (ret)
> - return ret;
> + goto err_disable_pri;
> }
>
> if (!param->max_pasid)
> @@ -2329,11 +2420,17 @@ static int arm_smmu_sva_init(struct device
> *dev, struct iommu_sva_param *param)
> param->max_pasid = min(param->max_pasid, (1U << master-
> >ssid_bits) - 1);
>
> return 0;
> +
> +err_disable_pri:
> + arm_smmu_disable_pri(master);
> +
> + return ret;
> }
>
> static void arm_smmu_sva_shutdown(struct device *dev,
> struct iommu_sva_param *param)
> {
> + arm_smmu_disable_pri(dev->iommu_fwspec->iommu_priv);
> iopf_queue_remove_device(dev);
> }
>
> @@ -2671,6 +2768,7 @@ static void arm_smmu_remove_device(struct
> device *dev)
> iommu_group_remove_device(dev);
> arm_smmu_remove_master(smmu, master);
> iommu_device_unlink(&smmu->iommu, dev);
> + arm_smmu_disable_pri(master);
> arm_smmu_disable_ats(master);
> kfree(master);
> iommu_fwspec_free(dev);
> --
> 2.17.0