Re: [PATCH v3 06/13] iommu/arm-smmu-v3: Submit CMDQ_OP_PRI_RESP for IOPF event

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Sep 03, 2026 at 12:18:33PM -0700, Jonathan Cameron wrote:
> > To handle IOMMU_FAULT_PAGE_REQ from the PRI queue, arm_smmu_page_response()
> > must issue a CMDQ_OP_PRI_RESP back to the SMMU.
> > 
> > A stall event in the EVTQ and a PRI request in the PRIQ both surface to the
> > IOPF infrastructure with fault.type == IOMMU_FAULT_PAGE_REQ. SMMUv3 forbids
> > the Stall model on PCIe streams (PCIe must use Terminate), and PRI is only
> 
> There are those systems that annoy some because they smell like PCIe
> (present PCIe software interfaces) but aren't and use stall mode. However, it
> is nonsense to use PRI with stall mode. So, instead I'd just argue that for
> stall the fault handling is done synchronously from a device point of
> view so a PRI request makes no sense rather htan associating this with
> PCIe as such.

I changed the paragraph to:

A stall event in the EVTQ and a PRI request in the PRIQ both surface to the
IOPF infrastructure with fault.type == IOMMU_FAULT_PAGE_REQ. Yet, both two
models are alternatives: a stalling master resolves a fault synchronously,
holding the transaction until a CMDQ_OP_RESUME, so it has no reason to also
issue a page request. A later change refuses PRI on a stall_enabled master,
so the two will never co-occur. arm_smmu_page_response() can therefore key
on the master state: CMDQ_OP_RESUME for stall_enabled, CMDQ_OP_PRI_RESP for
pri_enabled, mapping IOMMU_PAGE_RESP_* to the PRI response codes.

> > Co-developed-by: Barak Biber <bbiber@nvidia.com>
> > Signed-off-by: Barak Biber <bbiber@nvidia.com>
> > Co-developed-by: Stefan Kaestle <skaestle@nvidia.com>
> > Signed-off-by: Stefan Kaestle <skaestle@nvidia.com>
> > Signed-off-by: Malak Marrid <mmarrid@nvidia.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

> One trivial comment inline.  Given I've mostly forgotten how all this
> works, this tag might not worth that much!
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Your review is helpful!

> > +	} else if (master->pri_enabled) {
> > +		enum pri_resp pri_resp;
> > +		bool ssv;
> > +
> > +		/* PCIe allows only one PRG Response per group */
> > +		if (!(evt->fault.prm.flags &
> > +		      IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
> 
> Go long on that line.  It is worth it for readability and it's only 81
> chars.

I added a prm and cleaned two:

@@ -1061,12 +1061,12 @@ static void arm_smmu_page_response(struct device *dev, struct iopf_fault *evt,
 		}
 		cmd = arm_smmu_make_cmd_resume(sid, resp->grpid, resume_resp);
 	} else if (master->pri_enabled) {
+		struct iommu_fault_page_request *prm = &evt->fault.prm;
 		enum pri_resp pri_resp;
 		bool ssv;
 
 		/* PCIe allows only one PRG Response per group */
-		if (!(evt->fault.prm.flags &
-		      IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
+		if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
 			return;
 		switch (resp->code) {
 		case IOMMU_PAGE_RESP_SUCCESS:
@@ -1084,8 +1084,7 @@ static void arm_smmu_page_response(struct device *dev, struct iopf_fault *evt,
 			WARN_ON(true);
 			return;
 		}
-		ssv = !!(evt->fault.prm.flags &
-			 IOMMU_FAULT_PAGE_REQUEST_PASID_VALID);
+		ssv = !!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID);
 		cmd = arm_smmu_make_cmd_pri_resp(sid, resp->pasid, ssv,
 						 resp->grpid, pri_resp);
 	} else {

Thanks
Nicolin