Hi Jean-Philippe,
On Thu, 1 Jun 2023 10:26:34 +0100, Jean-Philippe Brucker
<jean-philippe@xxxxxxxxxx> wrote:
> 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>
Will do!
Thanks,
Jacob
