Re: [PATCH v4 10/15] iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()

[Jonathan Cameron]

> For a PRI-enabled master, convert every PRIQ entry into an iopf_fault and
> report it through iommu_report_device_fault().
> 
> For an unknown StreamID or a master without PRI enabled, keep the existing
> LAST-page DENY path. Merge its two messages into one ratelimited line so a
> faulty device cannot flood the kernel log.
> 
> Discard any PASID Stop Marker (LRW = 0b100) prior to the fault report and
> the DENY fallback both, because a Stop Marker does not expect a response.
> The IOPF infrastructure requires the driver to discard it, as per the doc
> at iommu_report_device_fault(). This also matches the intel-iommu code.
> 
> Note that master->pri_enabled will only be set by a later change, once all
> the PRI paths are ready.
> 
> Co-developed-by: Barak Biber <bbiber@xxxxxxxxxx>
> Signed-off-by: Barak Biber <bbiber@xxxxxxxxxx>
> Co-developed-by: Stefan Kaestle <skaestle@xxxxxxxxxx>
> Signed-off-by: Stefan Kaestle <skaestle@xxxxxxxxxx>
> Signed-off-by: Malak Marrid <mmarrid@xxxxxxxxxx>
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>

One question inline. I made the error of looking at the spec
and got confused.

Jonathan


>
> 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 43a6029cef43..c631c6344685 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2370,6 +2370,7 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>  
>  static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64 *evt)
>  {
> +	struct arm_smmu_master *master;
>  	u32 sid, ssid;
>  	u16 grpid;
>  	bool ssv, last;
> @@ -2380,9 +2381,47 @@ static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64 *evt)
>  	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",
> +	/*
> +	 * A PASID Stop Marker (LRW = 0b100) does not expect a response and
> +	 * must be discarded before fault reporting: see the documentation
> +	 * at iommu_report_device_fault().
> +	 */
> +	if (last && !(evt[0] & (PRIQ_0_PERM_READ | PRIQ_0_PERM_WRITE)))

There is a bit of spec language about case where there is no PASID
LWR=0xb100 not being a stop marker, SSV == 0 and should be treated as
a PRI request. I'm not sure what that means in practice but maybe
a comment on why that case doesn't matter. Feels like breadcrumbs we
might need in future.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>