Re: [PATCH v7 1/4] iommu: Generalize PASID 0 for normal DMA w/o PASID
From: Jean-Philippe Brucker
Date: Thu Jun 01 2023 - 05:26:48 EST
Hi Jacob,
On Tue, May 23, 2023 at 10:34:48AM -0700, Jacob Pan wrote:
> PCIe Process address space ID (PASID) is used to tag DMA traffic, it
> provides finer grained isolation than requester ID (RID).
>
> For each device/RID, 0 is a special PASID for the normal DMA (no
> PASID). This is universal across all architectures that supports PASID,
> therefore warranted to be reserved globally and declared in the common
> header. Consequently, we can avoid the conflict between different PASID
> use cases in the generic code. e.g. SVA and DMA API with PASIDs.
>
> This paved away for device drivers to choose global PASID policy while
> continue doing normal DMA.
>
> Noting that VT-d could support none-zero RID/NO_PASID, but currently not
> used.
>
> Reviewed-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> ---
> v7:
> - renamed IOMMU_DEF_RID_PASID to be IOMMU_NO_PASID to be more generic
> v6:
> - let SMMU code use the common RID_PASID macro
> ---
> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +-
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 ++++----
> drivers/iommu/intel/iommu.c | 24 +++++++++----------
> drivers/iommu/intel/pasid.c | 2 +-
> drivers/iommu/intel/pasid.h | 1 -
> include/linux/iommu.h | 1 +
> 6 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index a5a63b1c947e..5e6b39881c04 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -80,7 +80,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
> * be some overlap between use of both ASIDs, until we invalidate the
> * TLB.
> */
> - arm_smmu_write_ctx_desc(smmu_domain, 0, cd);
> + arm_smmu_write_ctx_desc(smmu_domain, IOMMU_NO_PASID, cd);
>
> /* Invalidate TLB entries previously associated with that context */
> arm_smmu_tlb_inv_asid(smmu, asid);
> 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 3fd83fb75722..6d64c8fc923f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1053,7 +1053,7 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
> /*
> * This function handles the following cases:
> *
> - * (1) Install primary CD, for normal DMA traffic (SSID = 0).
> + * (1) Install primary CD, for normal DMA traffic (SSID = IOMMU_NO_PASID = 0).
> * (2) Install a secondary CD, for SID+SSID traffic.
> * (3) Update ASID of a CD. Atomically write the first 64 bits of the
> * CD, then invalidate the old entry and mappings.
> @@ -1869,7 +1869,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid;
> arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> }
> - arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
> + arm_smmu_atc_inv_domain(smmu_domain, IOMMU_NO_PASID, 0, 0);
> }
>
> static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> @@ -1957,7 +1957,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
> * Unfortunately, this can't be leaf-only since we may have
> * zapped an entire table.
> */
> - arm_smmu_atc_inv_domain(smmu_domain, 0, iova, size);
> + arm_smmu_atc_inv_domain(smmu_domain, IOMMU_NO_PASID, iova, size);
> }
>
> void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
> @@ -2131,7 +2131,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
> * the master has been added to the devices list for this domain.
> * This isn't an issue because the STE hasn't been installed yet.
> */
> - ret = arm_smmu_write_ctx_desc(smmu_domain, 0, &cfg->cd);
> + ret = arm_smmu_write_ctx_desc(smmu_domain, IOMMU_NO_PASID, &cfg->cd);
> if (ret)
> goto out_free_cd_tables;
>
> @@ -2317,7 +2317,7 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
> pdev = to_pci_dev(master->dev);
>
> atomic_inc(&smmu_domain->nr_ats_masters);
> - arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
> + arm_smmu_atc_inv_domain(smmu_domain, IOMMU_NO_PASID, 0, 0);
> if (pci_enable_ats(pdev, stu))
> dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
> }
Thanks, I think this makes the code clearer. For consistency you could
also add these:
---
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 6d64c8fc923fc..c977106357961 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1601,7 +1601,7 @@ static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64 *evt)
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;
+ ssid = ssv ? FIELD_GET(PRIQ_0_SSID, evt[0]) : IOMMU_NO_PASID;
last = FIELD_GET(PRIQ_0_PRG_LAST, evt[0]);
grpid = FIELD_GET(PRIQ_1_PRG_IDX, evt[1]);
@@ -1742,7 +1742,7 @@ arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
*/
*cmd = (struct arm_smmu_cmdq_ent) {
.opcode = CMDQ_OP_ATC_INV,
- .substream_valid = !!ssid,
+ .substream_valid = (ssid != IOMMU_NO_PASID),
.atc.ssid = ssid,
};
@@ -1789,7 +1789,7 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
struct arm_smmu_cmdq_ent cmd;
struct arm_smmu_cmdq_batch cmds;
- arm_smmu_atc_inv_to_cmd(0, 0, 0, &cmd);
+ arm_smmu_atc_inv_to_cmd(IOMMU_NO_PASID, 0, 0, &cmd);
cmds.num = 0;
for (i = 0; i < master->num_streams; i++) {
---
With that, for SMMU:
Reviewed-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>