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

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

> 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.

Hopefully someone with such a system (Huawei folk) are testing this
and can confirm nothing breaks.

> enabled on PCIe masters, so stall_enabled and pri_enabled never co-occur on
> a single master. 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.
> 
> Note that a CMD_PRI_RESP.Resp encodes 0b00 as ResponseFailure (a permanent
> non-paging error), 0b01 as InvalidRequest (page-in unsuccessful), and 0b10
> as Success. So IOMMU_PAGE_RESP_FAILURE maps to PRI_RESP_DENY (0b00) while
> IOMMU_PAGE_RESP_INVALID maps to PRI_RESP_FAIL (0b01), following the codes
> rather than the similarity of the enum names.
> 
> Extend arm_smmu_enable_iopf() to also proceed for a PRI-enabled master, so
> that attaching a fault-capable domain would set up IOPF for it. Note that
> a later change will set master->pri_enabled, once all PRI paths are ready.
> 
> Note: streams[0].id remains the RID because arm_smmu_enable_iopf() rejects
> num_streams != 1.
> 
> 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>

>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 352c916b2a57..64540cfb7324 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1028,32 +1028,69 @@ static int arm_smmu_drain_queue(struct arm_smmu_device *smmu,
>  	return -ETIMEDOUT;
>  }
>  
> -static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
> +static void arm_smmu_page_response(struct device *dev, struct iopf_fault *evt,
>  				   struct iommu_page_response *resp)
>  {
...
> +	} 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.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>