Re: [PATCH v3 13/13] iommu/arm-smmu-v3: Enable PRI for PCI device in arm_smmu_probe_device()

[Jonathan Cameron]

> Now PRI requests can be correctly handled. Enable the PCI cap when probing
> a PCI device. Also flush the priq in arm_smmu_attach_release().
> 
> Drain the priq for any PRI-enabled master following the same rationale as
> the eventq drain: a stale page request must be answered while it can still
> resolve to the old attach handle, even when the old attachment did not set
> up IOPF, or else the threaded handler could route it to the next domain.
> 
> Note that PRI is enabled at the probe time, while ATS gets toggled by the
> attach/detach routines, so a master could have PRI enabled when its ATS is
> disabled. PCIe (Base 6.3, Table 10-14) sets no ATS precondition on the PRI
> Enable bit. Its only ordering rule is that the interface must have gotten
> successfully Stopped prior to an enabling, which pci_enable_pri() already
> checks using PCI_PRI_STATUS_STOPPED. Also, a PRI-enabled device would not
> issue a page request until it starts to use ATS.
> 
> Set the per-device outstanding request budget to the full priq depth, same
> as intel-iommu's per-device PRQ_DEPTH choice. A fixed per-device cap won't
> prevent multiple PRI-capable devices from potentially exceeding the priq's
> capacity; priq overflow is recoverable per the SMMUv3 spec, and it is rare
> in practice.
> 
> Select PCI_PRI in Kconfig like other IOMMUs, gated on PCI so the build can
> stay clean for non-PCI ARM SMMUv3 configurations.
> 
> SMMUv3 forbids the Stall model on PCIe streams. Refuse to enable PRI on a
> PCIe master that came with stall_enabled, so page_response() can dispatch
> on master state unambiguously.

As earlier, we have exceptions in tree for Stall mode on PCIe streams
(lets not reopen that fun arguement) so I'd focus this on PRI not
making any sense if stall mode is in use as that has alternative
handling for page faults.


> 
> Refuse to enable PRI as well on any master reporting num_streams != 1, as
> arm_smmu_enable_iopf() rejects multi-stream masters, so IOPF cannot be set
> up for them; keeping PRI enabled would let a PRI request arrive on an alias
> StreamID and get a PRI_RESP_DENY issued against streams[0] by the driver's
> error-response path.
> 
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
A few minor things in here.

Thanks,
Jonathan

>
> diff --git a/drivers/iommu/arm/Kconfig b/drivers/iommu/arm/Kconfig
> index b848a4253677..a31d04f5b031 100644
> --- a/drivers/iommu/arm/Kconfig
> +++ b/drivers/iommu/arm/Kconfig
> @@ -80,6 +80,7 @@ config ARM_SMMU_V3
>  	select IOMMU_IO_PGTABLE_LPAE
>  	select IOMMU_IOPF
>  	select GENERIC_MSI_IRQ
> +	select PCI_PRI if PCI
>  	select IOMMUFD_DRIVER if IOMMUFD
>  	help
>  	  Support for implementations of the ARM System MMU architecture
> 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 47c95b691503..d35d814900cb 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3509,6 +3509,17 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
>  			synchronize_irq(smmu->combined_irq);
>  	}
>  
> +	/* Same as the eventq drain above, for the hardware priq */
> +	if (master->pri_enabled) {
> +		ret |= arm_smmu_drain_queue(smmu, &smmu->priq.q, false);
> +		/* Ensure pending requests have reached the IOPF queue */
> +		if (!ret && smmu->priq.q.irq)

Similar to before, I'd factor out the if (!ret)
Using |= on return values is never particularly nice though safe
as used here.  Still I'd use another variable to store that things
timed out already so we are skipping these.

> +			synchronize_irq(smmu->priq.q.irq);
> +		/* Pending requests might be in the combined_irq handler */
> +		if (!ret && smmu->combined_irq)
> +			synchronize_irq(smmu->combined_irq);
> +	}
> +
>  	/*
>  	 * Only IOPF-enabled attachments queue fault work, and such work
>  	 * references the old domain via its attach handle. Flush it, as
> @@ -4446,6 +4457,40 @@ static int arm_smmu_master_prepare_ats(struct arm_smmu_master *master)
>  	return arm_smmu_alloc_cd_tables(master);
>  }
>  
> +static void arm_smmu_master_enable_pri(struct arm_smmu_master *master)
> +{
> +	struct arm_smmu_device *smmu = master->smmu;
> +	struct pci_dev *pdev;
> +	unsigned int reqs;
> +
> +	if (!(smmu->features & ARM_SMMU_FEAT_PRI) || !smmu->evtq.iopf)
> +		return;
> +	if (!dev_is_pci(master->dev))
> +		return;
> +	pdev = to_pci_dev(master->dev);
> +
> +	if (!pci_pri_supported(pdev))
> +		return;
> +
> +	/* SMMUv3 forbids the Stall model on PCIe streams */

Again, I'd tweak the wording given we have quite a few examples
in tree that do stall mode on PCIe smelling streams.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>