iommu/arm-smmu-v3: Share domain across SMMU/vSMMU instances
Subsystems (5)
v2 cover iommu/arm-smmu-v3: Share domain across SMMU/vSMMU instances
In a system with multiple physical SMMU instances, multiple devices can be passed through to a VM. Currently, a VM would allocate one domain per SMMU instance that might be shared across devices that sit behind the same SMMU instance. However, the gPA->PA mappings (either an S1 unmanaged domain or an S2 nesting parent domain) can be shared across all the devices that sit behind different SMMU instances as well, provided that the shared I/O page table is compatible with all the SMMU instances.
The major difficulty in sharing the domain has been invalidation, since a change to the shared I/O page table results in an invalidation on all SMMU instances. A traditional approach involves building a linked list of SMMUs within the domain, which is very inefficient for the invalidation path as the linked list has to be locked.
To address this, the SMMUv3 driver now uses an RCU-protected invalidation array. Any new device (and its SMMU) is preloaded into the array during a device attachment. This array maintains all necessary information, such as ASID/VMID and which SMMU instance (CMDQ) to issue the command to.
The second issue concerns the lifecycle of the iotlb tag. Currently, ASID or VMID is allocated per domain and kept in the domain structure (cd->asid or s2_cfg->vmid). This does not work ideally when the domain (e.g. S2) is shared, as the VMID will have to be global across all SMMU instances, even if a VM is not using all of them. This results in wasted VMID resources in the bitmaps of unused SMMU instances.
Instead, an iotlb tag should be allocated per SMMU instance. Consequently, these tags must be allocated and maintained separately. Since ASID or VMID is only used when a CD or STE is installed to the HW (which happens during device attachment), and the invalidation array is built right before that, it is ideal to allocate a new iotlb tag before arm_smmu_invs_merge():
- when a device attaches, the driver first searches for an existing iotlb tag for the SMMU the device sits behind
- If a match is found, the "users" counter is incremented
- otherwise, a new tag is allocated.
A nested domain case is slightly unique as certain HW requires the VMID at the vSMMU init stage v.s. a device attachment (to the nested domain). Thus
- allocate/free a vmid in vsmmu_init/vsmmu_destroy and store in vSMMU
- introduce an INV_TYPE_S2_VMID_VSMMU to separate it from a naked S2 case
- retrieve the vmid from the vSMMU during attachment instead of allocation
With this, deprecate the cd->asid and s2_cfg->vmid from struct smmu_domain, and replace them with the iotlb tag stored in the smmu_domain->invs array.
Finally, allow sharing a domain across the SMMU instances, so long as they passes a compatibility test.
This is on Github:
https://github.com/nicolinc/iommufd/commits/smmuv3_share_domain-v2
This is based on the series "Introduce an RCU-protected invalidation array"
https://lore.kernel.org/all/[email protected]/
So the whole implementation follows the path Jason envisioned initially.
A earlier effort to share S2 domain can be found:
https://lore.kernel.org/all/[email protected]/
Changelog
v2
* Add arm_smmu_domain_get_iotlb_tag()
* Drop asid array and vmid from master structure, and get the iotlb tag in the smmu_domain->invs array
* Introduce INV_TYPE_S2_VMID_VSMMU for vSMMU type, and separate the nested attach case from a naked S2 attach case
v1
https://lore.kernel.org/all/[email protected]/
Thanks
Nicolin
Nicolin Chen (10):
iommu/arm-smmu-v3: Store IOTLB cache tags in struct
arm_smmu_attach_state
iommu/arm-smmu-v3: Pass in IOTLB cache tag to CD and STE iommu/arm-smmu-v3: Look for existing iotlb tag in smmu_domain->invs iommu/arm-smmu-v3: Allocate IOTLB cache tag if no id to reuse iommu/arm-smmu-v3: Flush iotlb in arm_smmu_iotlb_tag_free() iommu/arm-smmu-v3: Allocate vmid in arm_vsmmu_init iommu/arm-smmu-v3: Pass in vsmmu to arm_smmu_domain_get_iotlb_tag() iommu/arm-smmu-v3: Introduce INV_TYPE_S2_VMID_VSMMU iommu/arm-smmu-v3: Remove ASID/VMID from arm_smmu_domain iommu/arm-smmu-v3: Allow sharing domain across SMMUs
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 53 +++- .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 50 ++- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 42 ++- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c | 21 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 286 ++++++++++-------- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 1 + 6 files changed, 266 insertions(+), 187 deletions(-)
Signature · 1 line
2.43.0
PATCH v2 1/10 iommu/arm-smmu-v3: Store IOTLB cache tags in struct arm_smmu_attach_state
So far, an IOTLB tag (ASID or VMID) has been stored in the arm_smmu_domain structure. Its lifecycle is aligned with the smmu_domain.
However, an IOTLB tag (ASID/VMID) will not be used:
1) Before being installed to CD/STE during a device attachment
2) After being removed from CD/STE during a device detachment
Both (1) and (2) exactly align with the lifecycle of smmu_domain->invs.
The bigger problem is that storing the IOTLB tag in struct arm_smmu_domain makes it difficult to share across SMMU instances, a common use case for a nesting parent domain.
Store the IOTLB tags (old domain's and new domain's) in the state. This'll be forwarded to CD and STE entries, to replace the references of cd->asid and s2_cfg->vmid from the smmu_domain.
Add a new arm_smmu_domain_get_iotlb_tag() helper provisionally copying the existing IOTLB tags from smmu_domain to fill arm_smmu_master_build_invs(). Later it will retrieve an IOTLB tag from the smmu_domain->invs or allocate a new one for the lifecycle issue.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 6 ++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 78 ++++++++++++++------- 2 files changed, 60 insertions(+), 24 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +6 -0
@@ -1111,11 +1111,13 @@ static inline bool arm_smmu_master_canwbs(struct arm_smmu_master *master)* @new_invs: for new domain, this is the new invs array to update domain->invs; * for old domain, this is the master->build_invs to pass in as the * to_unref argument to an arm_smmu_invs_unref() call +* @tag: IOTLB cache tag (INV_TYPE_S1_ASID or INV_TYPE_S2_VMID) */ struct arm_smmu_inv_state {struct arm_smmu_invs __rcu **invs_ptr;struct arm_smmu_invs *old_invs;struct arm_smmu_invs *new_invs; +struct arm_smmu_inv tag; };struct arm_smmu_attach_state {
@@ -1132,6 +1134,10 @@ struct arm_smmu_attach_state {bool ats_enabled; }; +int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain, +struct arm_smmu_device *smmu, +struct arm_smmu_inv *tag); +int arm_smmu_attach_prepare (struct arm_smmu_attach_state *state,struct iommu_domain *new_domain);void arm_smmu_attach_commit (struct arm_smmu_attach_state *state);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +54 -24
121 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v2 01/10] iommu/arm-smmu-v3: Store IOTLB cache tags in struct arm_smmu_attach_state
Quoted 13 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +13
-0+int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain, +struct arm_smmu_device *smmu, +struct arm_smmu_inv *tag) +{ +/* Decide the type of the iotlb cache tag */ +switch (smmu_domain->stage) { +case ARM_SMMU_DOMAIN_SVA : +case ARM_SMMU_DOMAIN_S1 : + tag->type =INV_TYPE_S1_ASID ; +break ; +case ARM_SMMU_DOMAIN_S2 : + tag->type =INV_TYPE_S2_VMID ; +break ;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_disable_iopf(struct arm_smmu_master *master, · line 3142
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10
-0+default : +return -EINVAL ; + } + + tag->smmu = smmu; + +if (tag->type ==INV_TYPE_S1_ASID ) + tag->id = smmu_domain->cd.asid; +else + tag->id = smmu_domain->s2_cfg.vmid;
Would be tidier to move these up into the case
Otherwise looks OK
Jason
Re: [PATCH v2 01/10] iommu/arm-smmu-v3: Store IOTLB cache tags in struct arm_smmu_attach_state
Quoted 18 lines · Jason Gunthorpe
Quoted 17 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +17
-0+int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain, +struct arm_smmu_device *smmu, +struct arm_smmu_inv *tag) +{ +/* Decide the type of the iotlb cache tag */ +switch (smmu_domain->stage) { +case ARM_SMMU_DOMAIN_SVA : +case ARM_SMMU_DOMAIN_S1 : + tag->type =INV_TYPE_S1_ASID ; +break ; +case ARM_SMMU_DOMAIN_S2 : + tag->type =INV_TYPE_S2_VMID ; +break ; +default : +return -EINVAL ; + } +
Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_disable_iopf(struct arm_smmu_master *master, · line 3142
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6
-0+ tag->smmu = smmu; + +if (tag->type ==INV_TYPE_S1_ASID ) + tag->id = smmu_domain->cd.asid; +else + tag->id = smmu_domain->s2_cfg.vmid;Would be tidier to move these up into the case
This will act as a fake allocation path in PATCh-3 and eventually get replaced with a real allocation piece in PATCH-4. So, I wrote like this in a transitory manner.
I can add a line of comments to clarify it.
Thanks
Nicolin
Re: [PATCH v2 01/10] iommu/arm-smmu-v3: Store IOTLB cache tags in struct arm_smmu_attach_state
Quoted 28 lines · Nicolin Chen
Quoted 18 lines · Jason Gunthorpe
Quoted 17 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +17
-0+int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain, +struct arm_smmu_device *smmu, +struct arm_smmu_inv *tag) +{ +/* Decide the type of the iotlb cache tag */ +switch (smmu_domain->stage) { +case ARM_SMMU_DOMAIN_SVA : +case ARM_SMMU_DOMAIN_S1 : + tag->type =INV_TYPE_S1_ASID ; +break ; +case ARM_SMMU_DOMAIN_S2 : + tag->type =INV_TYPE_S2_VMID ; +break ; +default : +return -EINVAL ; + } +Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_disable_iopf(struct arm_smmu_master *master, · line 3142
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6
-0+ tag->smmu = smmu; + +if (tag->type ==INV_TYPE_S1_ASID ) + tag->id = smmu_domain->cd.asid; +else + tag->id = smmu_domain->s2_cfg.vmid;Would be tidier to move these up into the case
Quoted 3 lines
This will act as a fake allocation path in PATCh-3 and eventually get replaced with a real allocation piece in PATCH-4. So, I wrote like this in a transitory manner.
I'd still write it in the nice way, you can remove the lines later when you add the allocation logic.
Jason
Re: [PATCH v2 01/10] iommu/arm-smmu-v3: Store IOTLB cache tags in struct arm_smmu_attach_state
Quoted 29 lines · Jason Gunthorpe
Quoted 28 lines · Nicolin Chen
Quoted 26 lines · Jason Gunthorpe
Quoted 13 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +13
-0+int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain, +struct arm_smmu_device *smmu, +struct arm_smmu_inv *tag) +{ +/* Decide the type of the iotlb cache tag */ +switch (smmu_domain->stage) { +case ARM_SMMU_DOMAIN_SVA : +case ARM_SMMU_DOMAIN_S1 : + tag->type =INV_TYPE_S1_ASID ; +break ; +case ARM_SMMU_DOMAIN_S2 : + tag->type =INV_TYPE_S2_VMID ; +break ;drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_disable_iopf(struct arm_smmu_master *master, · line 3142
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10
-0+default : +return -EINVAL ; + } + + tag->smmu = smmu; + +if (tag->type ==INV_TYPE_S1_ASID ) + tag->id = smmu_domain->cd.asid; +else + tag->id = smmu_domain->s2_cfg.vmid;Would be tidier to move these up into the case
Quoted 6 lines
Quoted 3 lines
This will act as a fake allocation path in PATCh-3 and eventually get replaced with a real allocation piece in PATCH-4. So, I wrote like this in a transitory manner.
I'd still write it in the nice way, you can remove the lines later when you add the allocation logic.
OK. I will keep them in the first switch statement.
This would instead make the get() in PATCH-3 a fake logic until PATCH-4 replacing them with a real allocation logic. Should work in the same way.
Thanks
Nicolin
PATCH v2 2/10 iommu/arm-smmu-v3: Pass in IOTLB cache tag to CD and STE
In reply to Nicolin ChenNow, struct arm_smmu_attach_state has the IOTLB cache tags copied from the cd->asid or s2_cfg->vmid of an smmu_domain.
Pass it down to arm_smmu_make_s1_cd() and arm_smmu_make_s2_domain_ste() to set in the CD and STE, removing the references of smmu_domain for its asid or vmid.
Note the two set_dev_pasid callbacks finalize CDs in arm_smmu_set_pasid(). So, it is safe for arm_smmu_make_sva_cd() and arm_smmu_make_s1_cd() to use a dummy iotlb tag (asid=0) because arm_smmu_set_pasid() will fix it.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 +-- .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 26 ++++++++++------ .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 22 ++++++++++--- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c | 12 +++++-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 31 +++++++++++-------- 5 files changed, 65 insertions(+), 31 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +3 -2
@@ -1005,7 +1005,7 @@ void arm_smmu_make_abort_ste(struct arm_smmu_ste *target);void arm_smmu_make_s2_domain_ste (struct arm_smmu_ste *target,struct arm_smmu_master *master,struct arm_smmu_domain *smmu_domain, -bool ats_enabled); +struct arm_smmu_inv *tag,bool ats_enabled);#if IS_ENABLED (CONFIG_KUNIT )void arm_smmu_get_ste_used (const __le64 *ent,__le64 *used_bits);
@@ -1070,7 +1070,8 @@ struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid);void arm_smmu_make_s1_cd (struct arm_smmu_cd *target,struct arm_smmu_master *master, -struct arm_smmu_domain *smmu_domain); +struct arm_smmu_domain *smmu_domain, +struct arm_smmu_inv *tag);void arm_smmu_write_cd_entry (struct arm_smmu_master *master,int ssid,struct arm_smmu_cd *cdptr,const struct arm_smmu_cd *target);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +16 -10
@@ -39,12 +39,15 @@ void *arm_smmu_hw_info(struct device *dev, u32 *length,return info; } -static void arm_smmu_make_nested_cd_table_ste( - struct arm_smmu_ste *target, struct arm_smmu_master *master, - struct arm_smmu_nested_domain *nested_domain, bool ats_enabled) +static void +arm_smmu_make_nested_cd_table_ste(struct arm_smmu_ste *target, + struct arm_smmu_master *master, + struct arm_smmu_nested_domain *nested_domain, + struct arm_smmu_inv *tag, bool ats_enabled) { - arm_smmu_make_s2_domain_ste( - target, master, nested_domain->vsmmu->s2_parent, ats_enabled); + arm_smmu_make_s2_domain_ste(target, master, + nested_domain->vsmmu->s2_parent, tag, + ats_enabled); target->data[0] = cpu_to_le64(STRTAB_STE_0_V | FIELD_PREP(STRTAB_STE_0_CFG,
@@ -64,9 +67,11 @@ static void arm_smmu_make_nested_cd_table_ste(* - Bypass STE (install the S2, no CD table) * - CD table STE (install the S2 and the userspace CD table) */ -static void arm_smmu_make_nested_domain_ste ( -struct arm_smmu_ste *target,struct arm_smmu_master *master, -struct arm_smmu_nested_domain *nested_domain,bool ats_enabled) +static void +arm_smmu_make_nested_domain_ste (struct arm_smmu_ste *target, +struct arm_smmu_master *master, +struct arm_smmu_nested_domain *nested_domain, +struct arm_smmu_inv *tag,bool ats_enabled) {unsigned int cfg =FIELD_GET (STRTAB_STE_0_CFG ,le64_to_cpu (nested_domain->ste[0 ]));
@@ -82,12 +87,12 @@ static void arm_smmu_make_nested_domain_ste(switch (cfg) {case STRTAB_STE_0_CFG_S1_TRANS :arm_smmu_make_nested_cd_table_ste (target, master, nested_domain, - ats_enabled); + tag, ats_enabled);break ;case STRTAB_STE_0_CFG_BYPASS :arm_smmu_make_s2_domain_ste (target, master, nested_domain->vsmmu->s2_parent, - ats_enabled); + tag, ats_enabled);break ;case STRTAB_STE_0_CFG_ABORT :default :
@@ -185,6 +190,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain, }arm_smmu_make_nested_domain_ste (&ste, master, nested_domain, + &state.new_domain_invst.tag, state.ats_enabled);arm_smmu_install_ste_for_dev (master, &ste);arm_smmu_attach_commit (&state);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +18 -4
@@ -24,12 +24,18 @@ arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain)list_for_each_entry (master_domain, &smmu_domain->devices, devices_elm) {struct arm_smmu_master *master = master_domain->master;struct arm_smmu_cd *cdptr; +struct arm_smmu_inv tag; cdptr =arm_smmu_get_cd_ptr (master, master_domain->ssid);if (WARN_ON (!cdptr))continue ; -arm_smmu_make_s1_cd (&target_cd, master, smmu_domain); +if (WARN_ON (arm_smmu_domain_get_iotlb_tag (smmu_domain, + master->smmu, &tag))) +continue ; +if (WARN_ON (tag.type !=INV_TYPE_S1_ASID )) +continue ; +arm_smmu_make_s1_cd (&target_cd, master, smmu_domain, &tag);arm_smmu_write_cd_entry (master, master_domain->ssid, cdptr, &target_cd); }
@@ -158,12 +164,18 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)struct arm_smmu_master *master = master_domain->master;struct arm_smmu_cd target;struct arm_smmu_cd *cdptr; +struct arm_smmu_inv tag; cdptr =arm_smmu_get_cd_ptr (master, master_domain->ssid);if (WARN_ON (!cdptr))continue ; -arm_smmu_make_sva_cd (&target, master,NULL , - smmu_domain->cd.asid); + +if (WARN_ON (arm_smmu_domain_get_iotlb_tag (smmu_domain, + master->smmu, &tag))) +continue ; +if (WARN_ON (tag.type !=INV_TYPE_S1_ASID )) +continue ; +arm_smmu_make_sva_cd (&target, master,NULL , tag.id);arm_smmu_write_cd_entry (master, master_domain->ssid, cdptr, &target); }
@@ -262,10 +274,12 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,return -EINVAL ;/* +* Use a dummy asid and fix it in arm_smmu_set_pasid(). +* * This does not need the arm_smmu_asid_lock because SVA domains never * get reassigned */ -arm_smmu_make_sva_cd (&target, master, domain->mm, smmu_domain->cd.asid); +arm_smmu_make_sva_cd (&target, master, domain->mm,0 ); ret =arm_smmu_set_pasid (master, smmu_domain, id, &target, old);mmput (domain->mm);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c +10 -2
@@ -306,6 +306,9 @@ static void arm_smmu_test_make_s2_ste(struct arm_smmu_ste *ste,struct arm_smmu_domain smmu_domain = { .pgtbl_ops = &io_pgtable.ops, }; +struct arm_smmu_inv tag = { + .type =INV_TYPE_S2_VMID , + }; io_pgtable.cfg.arm_lpae_s2_cfg.vttbr =0xdaedbeefdeadbeefULL ; io_pgtable.cfg.arm_lpae_s2_cfg.vtcr.ps =1 ;
@@ -316,7 +319,8 @@ static void arm_smmu_test_make_s2_ste(struct arm_smmu_ste *ste, io_pgtable.cfg.arm_lpae_s2_cfg.vtcr.sl =3 ; io_pgtable.cfg.arm_lpae_s2_cfg.vtcr.tsz =4 ; -arm_smmu_make_s2_domain_ste (ste, &master, &smmu_domain, ats_enabled); +arm_smmu_make_s2_domain_ste (ste, &master, &smmu_domain, &tag, + ats_enabled); }static void arm_smmu_v3_write_ste_test_s2_to_abort (struct kunit *test)
@@ -461,6 +465,10 @@ static void arm_smmu_test_make_s1_cd(struct arm_smmu_cd *cd, unsigned int asid) .asid = asid, }, }; +struct arm_smmu_inv tag = { + .type =INV_TYPE_S1_ASID , + .id = asid, + }; io_pgtable.cfg.arm_lpae_s1_cfg.ttbr =0xdaedbeefdeadbeefULL ; io_pgtable.cfg.arm_lpae_s1_cfg.tcr.ips =1 ;
@@ -471,7 +479,7 @@ static void arm_smmu_test_make_s1_cd(struct arm_smmu_cd *cd, unsigned int asid) io_pgtable.cfg.arm_lpae_s1_cfg.tcr.tsz =4 ; io_pgtable.cfg.arm_lpae_s1_cfg.mair =0xabcdef012345678ULL ; -arm_smmu_make_s1_cd (cd, &master, &smmu_domain); +arm_smmu_make_s1_cd (cd, &master, &smmu_domain, &tag); }static void arm_smmu_v3_write_cd_test_s1_clear (struct kunit *test)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +18 -13
@@ -1613,14 +1613,16 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,void arm_smmu_make_s1_cd (struct arm_smmu_cd *target,struct arm_smmu_master *master, -struct arm_smmu_domain *smmu_domain) +struct arm_smmu_domain *smmu_domain, +struct arm_smmu_inv *tag) { -struct arm_smmu_ctx_desc *cd = &smmu_domain->cd;const struct io_pgtable_cfg *pgtbl_cfg = &io_pgtable_ops_to_pgtable (smmu_domain->pgtbl_ops)->cfg; typeof(&pgtbl_cfg->arm_lpae_s1_cfg.tcr) tcr = &pgtbl_cfg->arm_lpae_s1_cfg.tcr; +WARN_ON (tag->type !=INV_TYPE_S1_ASID ); +memset (target,0 ,sizeof (*target)); target->data[0] = cpu_to_le64(
@@ -1640,7 +1642,7 @@ void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,CTXDESC_CD_0_R |CTXDESC_CD_0_A |CTXDESC_CD_0_ASET | -FIELD_PREP (CTXDESC_CD_0_ASID , cd->asid) +FIELD_PREP (CTXDESC_CD_0_ASID , tag->id) );/* To enable dirty flag update, set both Access flag and dirty state update */
@@ -1897,9 +1899,8 @@ EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_cdtable_ste);void arm_smmu_make_s2_domain_ste (struct arm_smmu_ste *target,struct arm_smmu_master *master,struct arm_smmu_domain *smmu_domain, -bool ats_enabled) +struct arm_smmu_inv *tag,bool ats_enabled) { -struct arm_smmu_s2_cfg *s2_cfg = &smmu_domain->s2_cfg;const struct io_pgtable_cfg *pgtbl_cfg = &io_pgtable_ops_to_pgtable (smmu_domain->pgtbl_ops)->cfg; typeof(&pgtbl_cfg->arm_lpae_s2_cfg.vtcr) vtcr =
@@ -1907,6 +1908,8 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,u64 vtcr_val;struct arm_smmu_device *smmu = master->smmu; +WARN_ON (tag->type !=INV_TYPE_S2_VMID ); +memset (target,0 ,sizeof (*target)); target->data[0] = cpu_to_le64( STRTAB_STE_0_V |
@@ -1930,7 +1933,7 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,FIELD_PREP (STRTAB_STE_2_VTCR_S2TG , vtcr->tg) |FIELD_PREP (STRTAB_STE_2_VTCR_S2PS , vtcr->ps); target->data[2 ] =cpu_to_le64 ( -FIELD_PREP (STRTAB_STE_2_S2VMID , s2_cfg->vmid) | +FIELD_PREP (STRTAB_STE_2_S2VMID , tag->id) |FIELD_PREP (STRTAB_STE_2_VTCR , vtcr_val) |STRTAB_STE_2_S2AA64 | #ifdef __BIG_ENDIAN
@@ -3671,7 +3674,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev,case ARM_SMMU_DOMAIN_S1 : {struct arm_smmu_cd target_cd; -arm_smmu_make_s1_cd (&target_cd, master, smmu_domain); +arm_smmu_make_s1_cd (&target_cd, master, smmu_domain, + &state.new_domain_invst.tag);arm_smmu_write_cd_entry (master,IOMMU_NO_PASID , cdptr, &target_cd); arm_smmu_make_cdtable_ste(&target, master, state.ats_enabled,
@@ -3681,6 +3685,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev, }case ARM_SMMU_DOMAIN_S2 :arm_smmu_make_s2_domain_ste (&target, master, smmu_domain, + &state.new_domain_invst.tag, state.ats_enabled);arm_smmu_install_ste_for_dev (master, &target);arm_smmu_clear_cd (master,IOMMU_NO_PASID );
@@ -3702,6 +3707,9 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain);struct arm_smmu_master *master =dev_iommu_priv_get (dev);struct arm_smmu_device *smmu = master->smmu; +struct arm_smmu_inv tag = { + .type =INV_TYPE_S1_ASID , + };struct arm_smmu_cd target_cd;if (smmu_domain->smmu != smmu)
@@ -3710,11 +3718,8 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,if (smmu_domain->stage !=ARM_SMMU_DOMAIN_S1 )return -EINVAL ; -/* -* We can read cd.asid outside the lock because arm_smmu_set_pasid() -* will fix it -*/ -arm_smmu_make_s1_cd (&target_cd, master, smmu_domain); +/* Use a dummy asid and fix it in arm_smmu_set_pasid() */ +arm_smmu_make_s1_cd (&target_cd, master, smmu_domain, &tag);return arm_smmu_set_pasid (master,to_smmu_domain (domain), id, &target_cd, old); }
@@ -3782,7 +3787,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,*/ cd->data[0 ] &= ~cpu_to_le64 (CTXDESC_CD_0_ASID ); cd->data[0 ] |=cpu_to_le64 ( -FIELD_PREP (CTXDESC_CD_0_ASID , smmu_domain->cd.asid)); +FIELD_PREP (CTXDESC_CD_0_ASID , state.new_domain_invst.tag.id));arm_smmu_write_cd_entry (master, pasid, cdptr, cd);arm_smmu_update_ste (master, sid_domain, state.ats_enabled);
Signature · 1 line
2.43.0
Re: [PATCH v2 02/10] iommu/arm-smmu-v3: Pass in IOTLB cache tag to CD and STE
Quoted 10 lines · Nicolin Chen
Now, struct
arm_smmu_attach_statehas the IOTLB cache tags copied from thecd->asidors2_cfg->vmidof ansmmu_domain.Pass it down to
arm_smmu_make_s1_cd()andarm_smmu_make_s2_domain_ste()to set in the CD and STE, removing the references ofsmmu_domainfor its asid or vmid.Note the two
set_dev_pasidcallbacks finalize CDs inarm_smmu_set_pasid(). So, it is safe forarm_smmu_make_sva_cd()andarm_smmu_make_s1_cd()to use a dummy iotlb tag (asid=0) becausearm_smmu_set_pasid()will fix it.
Maybe it is time to fix that up?
I think if you make arm_smmu_set_pasid() take in a function pointer of signature:
void (*func)(struct arm_smmu_cd *target,
struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, struct arm_smmu_inv *tag)
It can accept both arm_smmu_make_s1_cd() and arm_smmu_make_sva_cd() then call them at the right point once the tag is finally known.
It may have been tortured like this because of the (removed) BTM support, but when we bring that back the ASID for BTM should come from arm_smmu_domain_get_iotlb_tag().
Otherwise this looks OK
Jason
Re: [PATCH v2 02/10] iommu/arm-smmu-v3: Pass in IOTLB cache tag to CD and STE
Quoted 16 lines · Jason Gunthorpe
Quoted 10 lines · Nicolin Chen
Now, struct
arm_smmu_attach_statehas the IOTLB cache tags copied from thecd->asidors2_cfg->vmidof ansmmu_domain.Pass it down to
arm_smmu_make_s1_cd()andarm_smmu_make_s2_domain_ste()to set in the CD and STE, removing the references ofsmmu_domainfor its asid or vmid.Note the two
set_dev_pasidcallbacks finalize CDs inarm_smmu_set_pasid(). So, it is safe forarm_smmu_make_sva_cd()andarm_smmu_make_s1_cd()to use a dummy iotlb tag (asid=0) becausearm_smmu_set_pasid()will fix it.Maybe it is time to fix that up?
I think if you make
arm_smmu_set_pasid()take in a function pointer of signature:
Quoted 7 lines
void (*func)(struct
arm_smmu_cd*target,
structarm_smmu_master*master, structarm_smmu_domain*smmu_domain, structarm_smmu_inv*tag)It can accept both
arm_smmu_make_s1_cd()andarm_smmu_make_sva_cd()then call them at the right point once the tag is finally known.
Ack. That's cleaner. I will add a preparatory patch doing this.
Thanks!
Nicolin
PATCH v2 3/10 iommu/arm-smmu-v3: Look for existing iotlb tag in smmu_domain->invs
In reply to Nicolin ChenOnce arm_smmu_attach_prepare() installs an iotlb tag to smmu_domain->invs, arm_smmu_domain_get_iotlb_tag() callers should get the tag from the array.
Only the arm_smmu_domain_get_iotlb_tag() caller for new_smmu_domain in the arm_smmu_attach_prepare_invs() will be allowed to allocate a new tag.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 8 ++--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 +++++++++++++++++-- 3 files changed, 34 insertions(+), 8 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -1
@@ -1137,7 +1137,7 @@ struct arm_smmu_attach_state {int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain,struct arm_smmu_device *smmu, -struct arm_smmu_inv *tag); +struct arm_smmu_inv *tag,bool alloc);int arm_smmu_attach_prepare (struct arm_smmu_attach_state *state,struct iommu_domain *new_domain);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +4 -4
@@ -30,8 +30,8 @@ arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain)if (WARN_ON (!cdptr))continue ; -if (WARN_ON (arm_smmu_domain_get_iotlb_tag (smmu_domain, - master->smmu, &tag))) +if (WARN_ON (arm_smmu_domain_get_iotlb_tag ( + smmu_domain, master->smmu, &tag, false)))continue ;if (WARN_ON (tag.type !=INV_TYPE_S1_ASID ))continue ;
@@ -170,8 +170,8 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)if (WARN_ON (!cdptr))continue ; -if (WARN_ON (arm_smmu_domain_get_iotlb_tag (smmu_domain, - master->smmu, &tag))) +if (WARN_ON (arm_smmu_domain_get_iotlb_tag ( + smmu_domain, master->smmu, &tag, false)))continue ;if (WARN_ON (tag.type !=INV_TYPE_S1_ASID ))continue ;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +29 -3
@@ -3120,10 +3120,31 @@ static void arm_smmu_disable_iopf(struct arm_smmu_master *master,iopf_queue_remove_device (master->smmu->evtq.iopf, master->dev); } +static int __arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain, +struct arm_smmu_inv *tag) +{ +struct arm_smmu_invs *invs =rcu_dereference_protected ( + smmu_domain->invs,lockdep_is_held (&arm_smmu_asid_lock)); +size_t i; + +for (i =0 ; i != invs->num_invs; i++) { +if (invs->inv[i].type == tag->type && + invs->inv[i].smmu == tag->smmu && +refcount_read (&invs->inv[i].users)) { + *tag = invs->inv[i]; +return 0 ; + } + } + +return -ENOENT ; +} +int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain,struct arm_smmu_device *smmu, -struct arm_smmu_inv *tag) +struct arm_smmu_inv *tag,bool alloc) { +int ret; +/* Decide the type of the iotlb cache tag */ switch (smmu_domain->stage) {case ARM_SMMU_DOMAIN_SVA :
@@ -3139,6 +3160,11 @@ int arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain, tag->smmu = smmu; +/* Re-use an existing IOTLB cache tag in invs (users counter != 0) */ + ret =__arm_smmu_domain_get_iotlb_tag (smmu_domain, tag); +if (!ret || !alloc) +return ret; +if (tag->type ==INV_TYPE_S1_ASID ) tag->id = smmu_domain->cd.asid;else
@@ -3314,7 +3340,7 @@ static int arm_smmu_attach_prepare_invs(struct arm_smmu_attach_state *state, lockdep_is_held(&arm_smmu_asid_lock)); ret =arm_smmu_domain_get_iotlb_tag (new_smmu_domain, smmu, - &invst->tag); + &invst->tag, true);if (ret)return ret;
@@ -3344,7 +3370,7 @@ static int arm_smmu_attach_prepare_invs(struct arm_smmu_attach_state *state, lockdep_is_held(&arm_smmu_asid_lock)); ret =arm_smmu_domain_get_iotlb_tag (old_smmu_domain, smmu, - &invst->tag); + &invst->tag, false);if (WARN_ON (ret))return ret;
Signature · 1 line
2.43.0
Re: [PATCH v2 03/10] iommu/arm-smmu-v3: Look for existing iotlb tag in smmu_domain->invs
Quoted 5 lines · Nicolin Chen
Once
arm_smmu_attach_prepare()installs an iotlb tag tosmmu_domain->invs,arm_smmu_domain_get_iotlb_tag()callers should get the tag from the array.Only the
arm_smmu_domain_get_iotlb_tag()caller fornew_smmu_domainin thearm_smmu_attach_prepare_invs()will be allowed to allocate a new tag.
It would be nicer to have two functions 'alloc' and 'find' instead of the bool. Alloc would call find.
The logic looks sound otherwise
Jason
Re: [PATCH v2 03/10] iommu/arm-smmu-v3: Look for existing iotlb tag in smmu_domain->invs
Quoted 9 lines · Jason Gunthorpe
Quoted 5 lines · Nicolin Chen
Once
arm_smmu_attach_prepare()installs an iotlb tag tosmmu_domain->invs,arm_smmu_domain_get_iotlb_tag()callers should get the tag from the array.Only the
arm_smmu_domain_get_iotlb_tag()caller fornew_smmu_domainin thearm_smmu_attach_prepare_invs()will be allowed to allocate a new tag.It would be nicer to have two functions 'alloc' and 'find' instead of the bool. Alloc would call find.
Yea, that just came to me as well after seeing Will's comments in another thread.
Nicolin
PATCH v2 4/10 iommu/arm-smmu-v3: Allocate IOTLB cache tag if no id to reuse
In reply to Nicolin ChenAn IOTLB tag now is forwarded from arm_smmu_domain_get_iotlb_tag() to its final destination (a CD or STE entry).
Thus, arm_smmu_domain_get_iotlb_tag() can safely delink its references to the cd->asid and s2_cfg->vmid in the smmu_domain. Instead, allocate a new IOTLB cache tag from the xarray/ida.
The old asid and vmid in the smmu_domain will be deprecated, once VMID is decoupled from the vSMMU use case.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 38 ++++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +33 -5
@@ -1210,6 +1210,8 @@ void arm_smmu_invs_unref(struct arm_smmu_invs *invs,/* KUNIT test doesn't pass in a free_fn */ if (free_fn)free_fn (&invs->inv[i]); +/* Notify the caller to free the iotlb tag */ +refcount_set (&to_unref->inv[j].users,0 ); invs->num_trashes++; }else {/* item in to_unref is not in invs or already a trash */
@@ -3165,12 +3167,31 @@ int arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain,if (!ret || !alloc)return ret; -if (tag->type ==INV_TYPE_S1_ASID ) - tag->id = smmu_domain->cd.asid; -else - tag->id = smmu_domain->s2_cfg.vmid; +/* Allocate a new IOTLB cache tag (users counter == 0) */ +lockdep_assert_held (&arm_smmu_asid_lock); -return 0 ; +if (tag->type ==INV_TYPE_S1_ASID ) { + ret =xa_alloc (&arm_smmu_asid_xa, &tag->id, smmu_domain, +XA_LIMIT (1 , (1 << smmu->asid_bits) -1 ), +GFP_KERNEL ); + }else { + ret =ida_alloc_range (&smmu->vmid_map,1 , + (1 << smmu->vmid_bits) -1 ,GFP_KERNEL ); +if (ret >0 ) { + tag->id = ret;/* int is good for 16-bit VMID */ + ret =0 ; + } + } + +return ret; +} + +static void arm_smmu_iotlb_tag_free (struct arm_smmu_inv *tag) +{ +if (tag->type ==INV_TYPE_S1_ASID ) +xa_erase (&arm_smmu_asid_xa, tag->id); +else if (tag->type ==INV_TYPE_S2_VMID ) +ida_free (&tag->smmu->vmid_map, tag->id); } static struct arm_smmu_inv *
@@ -3220,6 +3241,9 @@ arm_smmu_master_build_inv(struct arm_smmu_master *master,break ; } +/* Set a default users counter */ +refcount_set (&cur->users,1 ); +return cur; }
@@ -3453,6 +3477,8 @@ arm_smmu_install_old_domain_invs(struct arm_smmu_attach_state *state)arm_smmu_invs_unref (old_invs, invst->new_invs, arm_smmu_inv_flush_iotlb_tag); +if (!refcount_read (&invst->new_invs->inv[0 ].users)) +arm_smmu_iotlb_tag_free (&invst->tag); new_invs =arm_smmu_invs_purge (old_invs);if (!new_invs)
@@ -3615,6 +3641,8 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state, err_free_vmaster:kfree (state->vmaster); err_unprepare_invs: +if (!refcount_read (&state->new_domain_invst.tag.users)) +arm_smmu_iotlb_tag_free (&state->new_domain_invst.tag);kfree (state->new_domain_invst.new_invs);return ret; }
Signature · 1 line
2.43.0
Re: [PATCH v2 04/10] iommu/arm-smmu-v3: Allocate IOTLB cache tag if no id to reuse
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_master_build_inv(struct arm_smmu_master *master, · line 3245
Quoted 6 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2
-0@@ -3220,6 +3241,9 @@ arm_smmu_master_build_inv(struct arm_smmu_master *master,break ; } +/* Set a default users counter */ +refcount_set (&cur->users,1 );
I think abusing users here is a little too hard to read..
Can we just keep track in the state somehow with a flag?
Or maybe union in a "bool needs_free" that is for the on-stack version of this structure?
Jason
Re: [PATCH v2 04/10] iommu/arm-smmu-v3: Allocate IOTLB cache tag if no id to reuse
Quoted 10 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_master_build_inv(struct arm_smmu_master *master, · line 3245
Quoted 6 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2
-0@@ -3220,6 +3241,9 @@ arm_smmu_master_build_inv(struct arm_smmu_master *master,break ; } +/* Set a default users counter */ +refcount_set (&cur->users,1 );I think abusing users here is a little too hard to read..
Quoted 4 lines
Can we just keep track in the state somehow with a flag?
Or maybe union in a "bool
needs_free" that is for the on-stack version of this structure?
This doesn't only apply to the case when a tag is newly allocated during the attach, but it can be used when an old tag has users=0 during a detach, right?
Hmm, perhaps I should have just followed your suggestion, letting arm_smmu_invs_unref() set the users at all:
https://lore.kernel.org/all/[email protected]/
FWIW, I am hoping to squash this into the base series (v10), so we wouldn't add a free_fn function pointer in the base series and then remove it in this followup series.
Nicolin
Re: [PATCH v2 04/10] iommu/arm-smmu-v3: Allocate IOTLB cache tag if no id to reuse
Quoted 11 lines · Nicolin Chen
Quoted 10 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_master_build_inv(struct arm_smmu_master *master, · line 3245
Quoted 6 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2
-0@@ -3220,6 +3241,9 @@ arm_smmu_master_build_inv(struct arm_smmu_master *master,break ; } +/* Set a default users counter */ +refcount_set (&cur->users,1 );I think abusing users here is a little too hard to read..
Quoted 8 lines
Quoted 4 lines
Can we just keep track in the state somehow with a flag?
Or maybe union in a "bool
needs_free" that is for the on-stack version of this structure?This doesn't only apply to the case when a tag is newly allocated during the attach, but it can be used when an old tag has users=0 during a detach, right?
Oh, IDK, I didn't look so closely..
I though this was just about cleaning up the tag during attach, the detach flow worked differently and did use users = 0?
Jason
Re: [PATCH v2 04/10] iommu/arm-smmu-v3: Allocate IOTLB cache tag if no id to reuse
Quoted 17 lines · Jason Gunthorpe
Quoted 16 lines · Nicolin Chen
Quoted 14 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_master_build_inv(struct arm_smmu_master *master, · line 3245
Quoted 6 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2
-0@@ -3220,6 +3241,9 @@ arm_smmu_master_build_inv(struct arm_smmu_master *master,break ; } +/* Set a default users counter */ +refcount_set (&cur->users,1 );I think abusing users here is a little too hard to read..
Can we just keep track in the state somehow with a flag?
Or maybe union in a "bool
needs_free" that is for the on-stack version of this structure?
Quoted 8 lines
Quoted 3 lines
This doesn't only apply to the case when a tag is newly allocated during the attach, but it can be used when an old tag has users=0 during a detach, right?
Oh, IDK, I didn't look so closely..
I though this was just about cleaning up the tag during attach, the detach flow worked differently and did use users = 0?
Yes. Detach doesn't allocate a new tag, as it gets the old tag and decrease its users counter in arm_smmu_invs_unref():
+ * This function will not fail. Any entry with users=0 will be marked as trash,
+ * and caller will be notified about the trashed entry via @to_unref by setting
+ * a users=0.
With that being said, we may still add a "bool is_trash", if we want. I just don't feel very necessary since we check "is_trash" throughout the driver via the users counter already..
FWIW, I kept three refcount_set calls in the latest base series v10, copied from your earlier suggestion. I moved that to this arm_smmu_master_build_inv() as I felt a bit redundant. Yet, it is probably better for readability reason:
+ arm_smmu_invs_for_each_cmp(invs, i, to_unref, j, cmp) {
+ if (cmp < 0) {
+ /* not found in to_unref, leave alone */
+ WRITE_ONCE(to_unref->inv[j].users, 1);
+ num_invs = i + 1;
+ } else if (cmp == 0) {
+ int users = READ_ONCE(invs->inv[i].users) - 1;
+
+ if (WARN_ON(users < 0))
+ continue;
+
+ /* same item */
+ WRITE_ONCE(invs->inv[i].users, users);
+ if (users) {
+ WRITE_ONCE(to_unref->inv[j].users, 1);
+ num_invs = i + 1;
+ continue;
+ }
+
+ /* Notify the caller about the trash entry */
+ WRITE_ONCE(to_unref->inv[j].users, 0);
+ invs->num_trashes++;
+ } else {
+ /* item in to_unref is not in invs or already a trash */
+ WARN_ON(true);
+ }
+ }
Nicolin
PATCH v2 5/10 iommu/arm-smmu-v3: Flush iotlb in arm_smmu_iotlb_tag_free()
In reply to Nicolin ChenIf the IOTLB cache tag is no longer used by any device, flush the entries tagged with the ASID/VMID, before the tag gets free-ed.
Drop the free_fn callback accordingly since it's the same thing.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c | 6 +-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 46 +++++-------------- 3 files changed, 16 insertions(+), 39 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -2
@@ -1024,8 +1024,7 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,struct arm_smmu_invs *arm_smmu_invs_merge (struct arm_smmu_invs *invs,struct arm_smmu_invs *to_merge);void arm_smmu_invs_unref (struct arm_smmu_invs *invs, -struct arm_smmu_invs *to_unref, -void (*free_fn)(struct arm_smmu_inv *inv)); +struct arm_smmu_invs *to_unref);struct arm_smmu_invs *arm_smmu_invs_purge (struct arm_smmu_invs *invs);#endif
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c +3 -3
@@ -635,7 +635,7 @@ static void arm_smmu_v3_invs_test(struct kunit *test) results2[0], results2[1]);/* Test3: unref invs2 (same array) */ -arm_smmu_invs_unref (test_a, &invs2,NULL ); +arm_smmu_invs_unref (test_a, &invs2);arm_smmu_v3_invs_test_verify (test, test_a,ARRAY_SIZE (results3[0 ]), results3[0 ], results3[1 ]);KUNIT_EXPECT_EQ (test, test_a->num_trashes,0 );
@@ -647,7 +647,7 @@ static void arm_smmu_v3_invs_test(struct kunit *test) results4[0], results4[1]);/* Test5: unref invs1 (same array) */ -arm_smmu_invs_unref (test_b, &invs1,NULL ); +arm_smmu_invs_unref (test_b, &invs1);arm_smmu_v3_invs_test_verify (test, test_b,ARRAY_SIZE (results5[0 ]), results5[0 ], results5[1 ]);KUNIT_EXPECT_EQ (test, test_b->num_trashes,2 );
@@ -659,7 +659,7 @@ static void arm_smmu_v3_invs_test(struct kunit *test) results6[0], results6[1]);/* Test7: unref invs3 (same array) */ -arm_smmu_invs_unref (test_a, &invs3,NULL ); +arm_smmu_invs_unref (test_a, &invs3);KUNIT_EXPECT_EQ (test, test_a->num_invs,0 );KUNIT_EXPECT_EQ (test, test_a->num_trashes,0 );
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +12 -34
82 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v2 05/10] iommu/arm-smmu-v3: Flush iotlb in arm_smmu_iotlb_tag_free()
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c int arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain, · line 3193
Quoted 11 lines · Nicolin Chen
static void
arm_smmu_iotlb_tag_free(struct arm_smmu_inv *tag){drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +9
-0+struct arm_smmu_cmdq_ent cmd = { + .opcode = tag->nsize_opcode, + }; + +if (tag->type ==INV_TYPE_S1_ASID ) + cmd.tlbi.asid = tag->id; +else + cmd.tlbi.vmid = tag->id; +arm_smmu_cmdq_issue_cmd_with_sync (tag->smmu, &cmd);
I think in all these places checking the tag->type it is probably a good idea to not use a catch all else for vmid? We have many tag types and some should never come to this, or other, functions.
Jason
Re: [PATCH v2 05/10] iommu/arm-smmu-v3: Flush iotlb in arm_smmu_iotlb_tag_free()
Quoted 16 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c int arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain, · line 3193
Quoted 11 lines · Nicolin Chen
static void
arm_smmu_iotlb_tag_free(struct arm_smmu_inv *tag){drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +9
-0+struct arm_smmu_cmdq_ent cmd = { + .opcode = tag->nsize_opcode, + }; + +if (tag->type ==INV_TYPE_S1_ASID ) + cmd.tlbi.asid = tag->id; +else + cmd.tlbi.vmid = tag->id; +arm_smmu_cmdq_issue_cmd_with_sync (tag->smmu, &cmd);I think in all these places checking the
tag->typeit is probably a good idea to not use a catch all else for vmid? We have many tag types and some should never come to this, or other, functions.
Or maybe we can add an assert function?
static inline void arm_smmu_inv_assert_iotlb_tag(struct arm_smmu_inv *inv)
{
WARN_ON(inv != INV_TYPE_S1_ASID && inv != INV_TYPE_S1_VMID &&
inv != INV_TYPE_S1_VMID_VSMMU);
}Nicolin
Re: [PATCH v2 05/10] iommu/arm-smmu-v3: Flush iotlb in arm_smmu_iotlb_tag_free()
Quoted 18 lines · Nicolin Chen
Quoted 16 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c int arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain, · line 3193
Quoted 11 lines · Nicolin Chen
static void
arm_smmu_iotlb_tag_free(struct arm_smmu_inv *tag){drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +9
-0+struct arm_smmu_cmdq_ent cmd = { + .opcode = tag->nsize_opcode, + }; + +if (tag->type ==INV_TYPE_S1_ASID ) + cmd.tlbi.asid = tag->id; +else + cmd.tlbi.vmid = tag->id; +arm_smmu_cmdq_issue_cmd_with_sync (tag->smmu, &cmd);I think in all these places checking the
tag->typeit is probably a good idea to not use a catch all else for vmid? We have many tag types and some should never come to this, or other, functions.
Quoted 7 lines
Or maybe we can add an assert function?
static inline void
arm_smmu_inv_assert_iotlb_tag(struct arm_smmu_inv *inv){ WARN_ON(inv != INV_TYPE_S1_ASID && inv != INV_TYPE_S1_VMID && inv != INV_TYPE_S1_VMID_VSMMU); }
That works too
Jason
PATCH v2 6/10 iommu/arm-smmu-v3: Allocate vmid in arm_vsmmu_init
In reply to Nicolin ChenVMID owned by a vSMMU should be allocated in the viommu_init callback, as HW like tegra241-cmdqv needs to setup VINTF with the VMID.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 21 +++++++++++++++++-- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 1 + 3 files changed, 21 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -1190,6 +1190,7 @@ size_t arm_smmu_get_viommu_size(struct device *dev,int arm_vsmmu_init (struct iommufd_viommu *viommu,struct iommu_domain *parent_domain,const struct iommu_user_data *user_data); +void arm_vsmmu_destroy (struct iommufd_viommu *viommu);int arm_smmu_attach_prepare_vmaster (struct arm_smmu_attach_state *state,struct arm_smmu_nested_domain *nested_domain);void arm_smmu_attach_commit_vmaster (struct arm_smmu_attach_state *state);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +19 -2
@@ -404,7 +404,17 @@ int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,return ret; } +void arm_vsmmu_destroy (struct iommufd_viommu *viommu) +{ + struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core); + +mutex_lock (&arm_smmu_asid_lock); +ida_free (&vsmmu->smmu->vmid_map, vsmmu->vmid); +mutex_unlock (&arm_smmu_asid_lock); +} +static const struct iommufd_viommu_ops arm_vsmmu_ops = { + .destroy = arm_vsmmu_destroy, .alloc_domain_nested = arm_vsmmu_alloc_domain_nested, .cache_invalidate = arm_vsmmu_cache_invalidate, };
@@ -454,14 +464,21 @@ int arm_vsmmu_init(struct iommufd_viommu *viommu,struct arm_smmu_device *smmu = container_of(viommu->iommu_dev, struct arm_smmu_device, iommu);struct arm_smmu_domain *s2_parent =to_smmu_domain (parent_domain); +int id;if (s2_parent->smmu != smmu)return -EINVAL ; +mutex_lock (&arm_smmu_asid_lock); + id =ida_alloc_range (&smmu->vmid_map,1 , (1 << smmu->vmid_bits) -1 , +GFP_KERNEL ); +mutex_unlock (&arm_smmu_asid_lock); +if (id <0 ) +return id; + + vsmmu->vmid = id; vsmmu->smmu = smmu; vsmmu->s2_parent = s2_parent; -/* FIXME Move VMID allocation from the S2 domain allocation to here */ - vsmmu->vmid = s2_parent->s2_cfg.vmid;if (viommu->type ==IOMMU_VIOMMU_TYPE_ARM_SMMUV3 ) { viommu->ops = &arm_vsmmu_ops;
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +1 -0
@@ -1209,6 +1209,7 @@ static void tegra241_cmdqv_destroy_vintf_user(struct iommufd_viommu *viommu)iommufd_viommu_destroy_mmap (&vintf->vsmmu.core, vintf->mmap_offset);tegra241_cmdqv_remove_vintf (vintf->cmdqv, vintf->idx); +arm_vsmmu_destroy (viommu); }static void tegra241_vintf_destroy_vsid (struct iommufd_vdevice *vdev)
Signature · 1 line
2.43.0
Re: [PATCH v2 06/10] iommu/arm-smmu-v3: Allocate vmid in arm_vsmmu_init
Quoted 2 lines · Nicolin Chen
VMID owned by a vSMMU should be allocated in the
viommu_initcallback, as HW like tegra241-cmdqv needs to setup VINTF with the VMID.
Even the architected SMMU needs this, the VMS (not implemented in Linux) should all share the same VMID for the same VM.
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu, · line 413
Quoted 7 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +7
-0+void arm_vsmmu_destroy (struct iommufd_viommu *viommu) +{ + struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core); + +mutex_lock (&arm_smmu_asid_lock); +ida_free (&vsmmu->smmu->vmid_map, vsmmu->vmid); +mutex_unlock (&arm_smmu_asid_lock);
Need a comment explaining where the flush is.
It looks like arm_smmu_iotlb_tag_free() does the free in the case of INV_TYPE_S2_VMID_VSMMU?
But this patch doesn't have that code yet. So maybe this should be merged with the next patch..
Jason
Re: [PATCH v2 06/10] iommu/arm-smmu-v3: Allocate vmid in arm_vsmmu_init
Quoted 6 lines · Jason Gunthorpe
Quoted 2 lines · Nicolin Chen
VMID owned by a vSMMU should be allocated in the
viommu_initcallback, as HW like tegra241-cmdqv needs to setup VINTF with the VMID.Even the architected SMMU needs this, the VMS (not implemented in Linux) should all share the same VMID for the same VM.
But for standard SMMU, the allocation/sharing could happen at the device attachment to a nested (maybe bypass proxy) domain, right?
vmid is only needed for STE (attach) and invalidation.
Or do you see some other case where vmid must be allocated during viommu_init?
Quoted 9 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu, · line 413
Quoted 7 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +7
-0+void arm_vsmmu_destroy (struct iommufd_viommu *viommu) +{ + struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core); + +mutex_lock (&arm_smmu_asid_lock); +ida_free (&vsmmu->smmu->vmid_map, vsmmu->vmid); +mutex_unlock (&arm_smmu_asid_lock);Need a comment explaining where the flush is.
Ack.
Quoted 5 lines
It looks like
arm_smmu_iotlb_tag_free()does the free in the case ofINV_TYPE_S2_VMID_VSMMU?But this patch doesn't have that code yet. So maybe this should be merged with the next patch..
Or maybe I should re-order the sequence of the patches. I'll see what works the best.
Thanks
Nicolin
Re: [PATCH v2 06/10] iommu/arm-smmu-v3: Allocate vmid in arm_vsmmu_init
Quoted 8 lines · Nicolin Chen
Quoted 6 lines · Jason Gunthorpe
Quoted 2 lines · Nicolin Chen
VMID owned by a vSMMU should be allocated in the
viommu_initcallback, as HW like tegra241-cmdqv needs to setup VINTF with the VMID.Even the architected SMMU needs this, the VMS (not implemented in Linux) should all share the same VMID for the same VM.
Quoted 7 lines
But for standard SMMU, the allocation/sharing could happen at the device attachment to a nested (maybe bypass proxy) domain, right?
vmid is only needed for STE (attach) and invalidation.
Or do you see some other case where vmid must be allocated during
viommu_init?
It has to be tied to the VSMMU, it doesn't matter if a single VMID is allocated on-demand for the VSMMU or for the lifetime, it has to be a single ID and shared across all STEs.
It is easy to understand that lifecycle by just allocating it for the lifetime of the VSMMU..
Jason
Re: [PATCH v2 06/10] iommu/arm-smmu-v3: Allocate vmid in arm_vsmmu_init
Quoted 12 lines · Jason Gunthorpe
Quoted 11 lines · Nicolin Chen
Quoted 6 lines · Jason Gunthorpe
Quoted 2 lines · Nicolin Chen
VMID owned by a vSMMU should be allocated in the
viommu_initcallback, as HW like tegra241-cmdqv needs to setup VINTF with the VMID.Even the architected SMMU needs this, the VMS (not implemented in Linux) should all share the same VMID for the same VM.
But for standard SMMU, the allocation/sharing could happen at the device attachment to a nested (maybe bypass proxy) domain, right?
Quoted 8 lines
Quoted 4 lines
vmid is only needed for STE (attach) and invalidation.
Or do you see some other case where vmid must be allocated during
viommu_init?It has to be tied to the VSMMU, it doesn't matter if a single VMID is allocated on-demand for the VSMMU or for the lifetime, it has to be a single ID and shared across all STEs.
Attachment threads are mutexed. Any device that attaches firstly will allocate a shareable tag and add it to the array. Any other device will get the tag from the array and reuse it. And nothing will change this.
Quoted 2 lines
It is easy to understand that lifecycle by just allocating it for the lifetime of the VSMMU..
Yea, it makes sense for that matter.
So, perhaps:
VMID owned by a vSMMU should be allocated in the viommu_init callback for
- a straightforward lifecycle for a VMID used by a vSMMU
- HW like tegra241-cmdqv needs to setup VINTF with the VMID
Thanks
Nicolin
Re: [PATCH v2 06/10] iommu/arm-smmu-v3: Allocate vmid in arm_vsmmu_init
Quoted 10 lines · Nicolin Chen
Quoted 2 lines
It is easy to understand that lifecycle by just allocating it for the lifetime of the VSMMU..
Yea, it makes sense for that matter.
So, perhaps:
VMID owned by a vSMMU should be allocated in the
viommu_initcallback for
- a straightforward lifecycle for a VMID used by a vSMMU
- HW like tegra241-cmdqv needs to setup VINTF with the VMID
Yeah, the cmdqv should take the VMID from the vSMMU and that is a good reason to pre-allocate it since it won't go through the invalidation list.
Jason
PATCH v2 7/10 iommu/arm-smmu-v3: Pass in vsmmu to arm_smmu_domain_get_iotlb_tag()
In reply to Nicolin ChenWhen a device attaches to a nested domain, VMID is held by its associated vSMMU instance. So arm_smmu_domain_get_iotlb_tag() should return the VMID from the vSMMU instance rather than allocating a new one. Pass in a vsmmu pointer to arm_smmu_domain_get_iotlb_tag(), to prepare for this.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 +++++++++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 4 ++-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 7 +++++-- 4 files changed, 17 insertions(+), 4 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +9 -0
@@ -1124,6 +1124,7 @@ struct arm_smmu_attach_state {/* Inputs */ struct iommu_domain *old_domain;struct arm_smmu_master *master; +struct arm_vsmmu *vsmmu;bool cd_needs_ats;bool disable_ats;ioasid_t ssid;
@@ -1136,6 +1137,7 @@ struct arm_smmu_attach_state {int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain,struct arm_smmu_device *smmu, +struct arm_vsmmu *vsmmu,struct arm_smmu_inv *tag,bool alloc); int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
@@ -1182,6 +1184,13 @@ struct arm_vsmmu {u16 vmid; }; +static inline struct arm_vsmmu *to_vsmmu (struct iommu_domain *domain) +{ +if (domain->type ==IOMMU_DOMAIN_NESTED ) +return to_smmu_nested_domain (domain)->vsmmu; +return NULL ; +} +#if IS_ENABLED (CONFIG_ARM_SMMU_V3_IOMMUFD )void *arm_smmu_hw_info (struct device *dev,u32 *length,enum iommu_hw_info_type *type);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +1 -0
@@ -162,6 +162,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,struct arm_smmu_master *master =dev_iommu_priv_get (dev);struct arm_smmu_attach_state state = { .master = master, + .vsmmu = nested_domain->vsmmu, .old_domain = old_domain, .ssid =IOMMU_NO_PASID , };
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +2 -2
@@ -31,7 +31,7 @@ arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain)continue ;if (WARN_ON (arm_smmu_domain_get_iotlb_tag ( - smmu_domain, master->smmu, &tag, false))) + smmu_domain, master->smmu,NULL , &tag, false)))continue ;if (WARN_ON (tag.type !=INV_TYPE_S1_ASID ))continue ;
@@ -171,7 +171,7 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)continue ;if (WARN_ON (arm_smmu_domain_get_iotlb_tag ( - smmu_domain, master->smmu, &tag, false))) + smmu_domain, master->smmu,NULL , &tag, false)))continue ;if (WARN_ON (tag.type !=INV_TYPE_S1_ASID ))continue ;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5 -2
@@ -3137,6 +3137,7 @@ static int __arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain,int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain,struct arm_smmu_device *smmu, +struct arm_vsmmu *vsmmu,struct arm_smmu_inv *tag,bool alloc) {int ret;
@@ -3360,6 +3361,7 @@ static int arm_smmu_attach_prepare_invs(struct arm_smmu_attach_state *state,*/ if (new_smmu_domain) {struct arm_smmu_inv_state *invst = &state->new_domain_invst; +struct arm_vsmmu *vsmmu = state->vsmmu;struct arm_smmu_invs *build_invs; invst->invs_ptr = &new_smmu_domain->invs;
@@ -3368,7 +3370,7 @@ static int arm_smmu_attach_prepare_invs(struct arm_smmu_attach_state *state, lockdep_is_held(&arm_smmu_asid_lock)); ret =arm_smmu_domain_get_iotlb_tag (new_smmu_domain, smmu, - &invst->tag, true); + vsmmu, &invst->tag, true);if (ret)return ret;
@@ -3387,6 +3389,7 @@ static int arm_smmu_attach_prepare_invs(struct arm_smmu_attach_state *state,if (old_smmu_domain) {struct arm_smmu_inv_state *invst = &state->old_domain_invst; +struct arm_vsmmu *vsmmu =to_vsmmu (state->old_domain); invst->invs_ptr = &old_smmu_domain->invs;/* A re-attach case might have a different ats_enabled state */
@@ -3398,7 +3401,7 @@ static int arm_smmu_attach_prepare_invs(struct arm_smmu_attach_state *state, lockdep_is_held(&arm_smmu_asid_lock)); ret =arm_smmu_domain_get_iotlb_tag (old_smmu_domain, smmu, - &invst->tag, false); + vsmmu, &invst->tag, false);if (WARN_ON (ret))return ret;
Signature · 1 line
2.43.0
Re: [PATCH v2 07/10] iommu/arm-smmu-v3: Pass in vsmmu to arm_smmu_domain_get_iotlb_tag()
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int __arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain, · line 3143
Quoted 12 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1
-0@@ -3137,6 +3137,7 @@ static int __arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain,int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain,struct arm_smmu_device *smmu, +struct arm_vsmmu *vsmmu,struct arm_smmu_inv *tag,bool alloc) {int ret;
This all seems too complicated, the domain passed in should have been a iommu_domain, not a smmu_domain, then you can just do
if (domain->Type == IOMMU_DOMAIN_NESTED) {
struct arm_smmu_nested_domain *nested = to_smmu_nested_domain(domain);
tag->type = INV_TYPE_S2_VMID_VSMMU;
tag->id = nested->vsmmu->vmid;
tag->smmu = nested->vsmmu->smmu;
return 0;
}
Here and everything is simple.
Jason
Re: [PATCH v2 07/10] iommu/arm-smmu-v3: Pass in vsmmu to arm_smmu_domain_get_iotlb_tag()
Quoted 18 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int __arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain, · line 3143
Quoted 12 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1
-0@@ -3137,6 +3137,7 @@ static int __arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain,int arm_smmu_domain_get_iotlb_tag (struct arm_smmu_domain *smmu_domain,struct arm_smmu_device *smmu, +struct arm_vsmmu *vsmmu,struct arm_smmu_inv *tag,bool alloc) {int ret;This all seems too complicated, the domain passed in should have been a
iommu_domain, not asmmu_domain, then you can just doif (
domain->Type==IOMMU_DOMAIN_NESTED) {
Quoted 8 lines
struct arm_smmu_nested_domain *nested = to_smmu_nested_domain(domain); tag->type = INV_TYPE_S2_VMID_VSMMU; tag->id = nested->vsmmu->vmid; tag->smmu = nested->vsmmu->smmu; return 0; }Here and everything is simple.
Hmm, should have squashed this in the base series v10 that I just sent. Anyway, I can change this in v3.
Thanks
Nicolin
PATCH v2 8/10 iommu/arm-smmu-v3: Introduce INV_TYPE_S2_VMID_VSMMU
In reply to Nicolin ChenA VMID held by a vSMMU is required to setup hardware (e.g. tegra241-cmdqv) during its initialization. So, it should be allocated in the ->viommu_init callback. This makes the VMID lifecycle unique than a VMID allocated for a naked S2 attachment.
Introduce an INV_TYPE_S2_VMID_VSMMU to accommodate this case.
Note that a second device attaching to a nested domain associated with the same vSMMU instance will have an INV_TYPE_S2_VMID_VSMMU and reuse the VMID held by the vSMMU. Devices attaching directly to the nesting parent domain will have an INV_TYPE_S2_VMID and shouldn't resue the VMID from the vSMMU.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -656,6 +656,7 @@ struct arm_smmu_cmdq_batch {enum arm_smmu_inv_type {INV_TYPE_S1_ASID ,INV_TYPE_S2_VMID , +INV_TYPE_S2_VMID_VSMMU ,INV_TYPE_S2_VMID_S1_CLEAR ,INV_TYPE_ATS ,INV_TYPE_ATS_FULL ,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +15 -3
@@ -1904,7 +1904,8 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,u64 vtcr_val;struct arm_smmu_device *smmu = master->smmu; -WARN_ON (tag->type !=INV_TYPE_S2_VMID ); +WARN_ON (tag->type !=INV_TYPE_S2_VMID && + tag->type !=INV_TYPE_S2_VMID_VSMMU );memset (target,0 ,sizeof (*target)); target->data[0] = cpu_to_le64(
@@ -2592,6 +2593,7 @@ static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs, granule);break ;case INV_TYPE_S2_VMID : +case INV_TYPE_S2_VMID_VSMMU : cmd.tlbi.vmid = cur->id; cmd.tlbi.leaf = leaf; arm_smmu_inv_to_cmdq_batch(cur, &cmds, &cmd, iova, size,
@@ -3149,7 +3151,10 @@ int arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain, tag->type =INV_TYPE_S1_ASID ;break ;case ARM_SMMU_DOMAIN_S2 : - tag->type =INV_TYPE_S2_VMID ; +if (vsmmu) + tag->type =INV_TYPE_S2_VMID_VSMMU ; +else + tag->type =INV_TYPE_S2_VMID ;break ;default :return -EINVAL ;
@@ -3163,6 +3168,12 @@ int arm_smmu_domain_get_iotlb_tag(struct arm_smmu_domain *smmu_domain,return ret;/* Allocate a new IOTLB cache tag (users counter == 0) */ +if (tag->type ==INV_TYPE_S2_VMID_VSMMU ) { +/* Use the pre-allocated VMID from vSMMU */ + tag->id = vsmmu->vmid; +return 0 ; + } +lockdep_assert_held (&arm_smmu_asid_lock);if (tag->type ==INV_TYPE_S1_ASID ) {
@@ -3233,6 +3244,7 @@ arm_smmu_master_build_inv(struct arm_smmu_master *master, }break ;case INV_TYPE_S2_VMID : +case INV_TYPE_S2_VMID_VSMMU : cur->size_opcode =CMDQ_OP_TLBI_S2_IPA ; cur->nsize_opcode =CMDQ_OP_TLBI_S12_VMALL ;break ;
@@ -3280,7 +3292,7 @@ arm_smmu_master_build_invs(struct arm_smmu_master *master, bool ats_enabled,return NULL ;/* All the nested S1 ASIDs have to be flushed when S2 parent changes */ -if (nesting) { +if (tag->type ==INV_TYPE_S2_VMID_VSMMU ) {if (!arm_smmu_master_build_inv (master,INV_TYPE_S2_VMID_S1_CLEAR , tag->id,IOMMU_NO_PASID ,0 ))
Signature · 1 line
2.43.0
Re: [PATCH v2 08/10] iommu/arm-smmu-v3: Introduce INV_TYPE_S2_VMID_VSMMU
Quoted 24 lines · Nicolin Chen
A VMID held by a vSMMU is required to setup hardware (e.g. tegra241-cmdqv) during its initialization. So, it should be allocated in the ->viommu_init callback. This makes the VMID lifecycle unique than a VMID allocated for a naked S2 attachment.
Introduce an
INV_TYPE_S2_VMID_VSMMUto accommodate this case.Note that a second device attaching to a nested domain associated with the same vSMMU instance will have an
INV_TYPE_S2_VMID_VSMMUand reuse the VMID held by the vSMMU. Devices attaching directly to the nesting parent domain will have anINV_TYPE_S2_VMIDand shouldn't resue the VMID from the vSMMU.Suggested-by Jason Gunthorpe <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0
-0
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_cmdq_batch { · line 662
Quoted 7 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1
-0@@ -656,6 +656,7 @@ struct arm_smmu_cmdq_batch {enum arm_smmu_inv_type {INV_TYPE_S1_ASID ,INV_TYPE_S2_VMID , +INV_TYPE_S2_VMID_VSMMU ,INV_TYPE_S2_VMID_S1_CLEAR ,INV_TYPE_ATS ,INV_TYPE_ATS_FULL ,
This should be moved earlier and probably some of the hunks merged with the patch introducing the vsmmu vmid.
Jason
PATCH v2 9/10 iommu/arm-smmu-v3: Remove ASID/VMID from arm_smmu_domain
In reply to Nicolin ChenNow ASID/VMID are stored in the arm_smmu_master. These are dead code now.
Remove all.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 8 --- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 20 +------ .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c | 3 - drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 58 ------------------- 4 files changed, 1 insertion(+), 88 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0 -8
@@ -784,10 +784,6 @@ static inline bool arm_smmu_ssids_in_use(struct arm_smmu_ctx_desc_cfg *cd_table)return cd_table->used_ssids; } -struct arm_smmu_s2_cfg { -u16 vmid; -}; - struct arm_smmu_strtab_cfg { union { struct {
@@ -964,10 +960,6 @@ struct arm_smmu_domain {atomic_t nr_ats_masters;enum arm_smmu_domain_stage stage; -union { -struct arm_smmu_ctx_desc cd; -struct arm_smmu_s2_cfg s2_cfg; - };struct iommu_domain domain;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +1 -19
@@ -295,14 +295,6 @@ static void arm_smmu_sva_domain_free(struct iommu_domain *domain)*/ arm_smmu_domain_inv (smmu_domain); -/* -* Notice that the arm_smmu_mm_arch_invalidate_secondary_tlbs op can -* still be called/running at this point. We allow the ASID to be -* reused, and if there is a race then it just suffers harmless -* unnecessary invalidation. -*/ -xa_erase (&arm_smmu_asid_xa, smmu_domain->cd.asid); -/* * Actual free is defered to the SRCU callback * arm_smmu_mmu_notifier_free()
@@ -321,7 +313,6 @@ struct iommu_domain *arm_smmu_sva_domain_alloc(struct device *dev,struct arm_smmu_master *master =dev_iommu_priv_get (dev);struct arm_smmu_device *smmu = master->smmu;struct arm_smmu_domain *smmu_domain; -u32 asid;int ret;if (!(master->smmu->features &ARM_SMMU_FEAT_SVA ))
@@ -340,22 +331,13 @@ struct iommu_domain *arm_smmu_sva_domain_alloc(struct device *dev, smmu_domain->domain.pgsize_bitmap =PAGE_SIZE ; smmu_domain->stage =ARM_SMMU_DOMAIN_SVA ; smmu_domain->smmu = smmu; - - ret =xa_alloc (&arm_smmu_asid_xa, &asid, smmu_domain, -XA_LIMIT (1 , (1 << smmu->asid_bits) -1 ),GFP_KERNEL ); -if (ret) - goto err_free; - - smmu_domain->cd.asid = asid; smmu_domain->mmu_notifier.ops = &arm_smmu_mmu_notifier_ops; ret =mmu_notifier_register (&smmu_domain->mmu_notifier, mm);if (ret) - goto err_asid; + goto err_free;return &smmu_domain->domain; -err_asid: -xa_erase (&arm_smmu_asid_xa, smmu_domain->cd.asid); err_free:arm_smmu_domain_free (smmu_domain);return ERR_PTR (ret);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c +0 -3
@@ -461,9 +461,6 @@ static void arm_smmu_test_make_s1_cd(struct arm_smmu_cd *cd, unsigned int asid)struct io_pgtable io_pgtable = {};struct arm_smmu_domain smmu_domain = { .pgtbl_ops = &io_pgtable.ops, - .cd = { - .asid = asid, - }, };struct arm_smmu_inv tag = { .type =INV_TYPE_S1_ASID ,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -58
93 lines not on this page · raw message
Signature · 1 line
2.43.0
PATCH v2 10/10 iommu/arm-smmu-v3: Allow sharing domain across SMMUs
In reply to Nicolin ChenVMM needs a domain holding the mappings between gPA to hPA. It can be an S1 domain or an S2 nesting parent domain, depending on whether the VM is built with a vSMMU or not.
Given that the IOAS for this gPA mapping is the same across SMMU instances, this domain can be shared across devices even if they sit behind different SMMUs, so long as the underlying page table is compatible between the SMMU instances.
There is no direct information about the page table from the master device, but a comparison can be done between the physical SMMU that the domain was allocated for and the physical SMMU that the device is behind.
Replace the smmu test in arm_smmu_attach_dev() and arm_vsmmu_init() with a compatibility test for the S1 and S2 cases respectively. The compatibility test goes through the physical SMMU parameters that were used to decide the page table formats.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 20 +++++++++++++++++++ .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 2 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +- 3 files changed, 22 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +20 -0
@@ -982,6 +982,26 @@ struct arm_smmu_nested_domain {__le64 ste[2 ]; }; +static inline bool +arm_smmu_domain_can_share (struct arm_smmu_domain *smmu_domain, +struct arm_smmu_device *new_smmu) +{ +struct arm_smmu_device *base_smmu = smmu_domain->smmu; + +if (base_smmu == new_smmu) +return true; +/* Only support identical SMMUs for now */ +if (base_smmu->features != new_smmu->features) +return false; +if (base_smmu->iommu.ops != new_smmu->iommu.ops) +return false; +if (base_smmu->pgsize_bitmap != new_smmu->pgsize_bitmap) +return false; +if (base_smmu->ias > new_smmu->ias || base_smmu->oas > new_smmu->oas) +return false; +return true; +} +/* The following are exposed for testing purposes. */ struct arm_smmu_entry_writer_ops ;struct arm_smmu_entry_writer {
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +1 -1
@@ -467,7 +467,7 @@ int arm_vsmmu_init(struct iommufd_viommu *viommu,struct arm_smmu_domain *s2_parent =to_smmu_domain (parent_domain);int id; -if (s2_parent->smmu != smmu) +if (!arm_smmu_domain_can_share (s2_parent, smmu))return -EINVAL ;mutex_lock (&arm_smmu_asid_lock);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -1
@@ -3635,7 +3635,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev, state.master = master =dev_iommu_priv_get (dev); smmu = master->smmu; -if (smmu_domain->smmu != smmu) +if (!arm_smmu_domain_can_share (smmu_domain, smmu))return -EINVAL ;if (smmu_domain->stage ==ARM_SMMU_DOMAIN_S1 ) {
Signature · 1 line
2.43.0
Re: [PATCH v2 10/10] iommu/arm-smmu-v3: Allow sharing domain across SMMUs
Quoted 4 lines · Nicolin Chen
VMM needs a domain holding the mappings between gPA to hPA. It can be an S1 domain or an S2 nesting parent domain, depending on whether the VM is built with a vSMMU or not.
Quoted 8 lines
Given that the IOAS for this gPA mapping is the same across SMMU instances, this domain can be shared across devices even if they sit behind different SMMUs, so long as the underlying page table is compatible between the SMMU instances.
There is no direct information about the page table from the master device, but a comparison can be done between the physical SMMU that the domain was allocated for and the physical SMMU that the device is behind.
I would very much prefer this works by inspecting the cfg from the iopgtable..
You can get it by doing
struct io_pgtable_cfg *pgtbl_cfg = &io_pgtable_ops_to_pgtable(domain->pgtbl_ops)->cfg;
I think it is important that the domain->smmu pointer be removed as well
Jason
Re: [PATCH v2 10/10] iommu/arm-smmu-v3: Allow sharing domain across SMMUs
Quoted 14 lines · Jason Gunthorpe
Quoted 4 lines · Nicolin Chen
VMM needs a domain holding the mappings between gPA to hPA. It can be an S1 domain or an S2 nesting parent domain, depending on whether the VM is built with a vSMMU or not.
Quoted 8 lines
Given that the IOAS for this gPA mapping is the same across SMMU instances, this domain can be shared across devices even if they sit behind different SMMUs, so long as the underlying page table is compatible between the SMMU instances.
There is no direct information about the page table from the master device, but a comparison can be done between the physical SMMU that the domain was allocated for and the physical SMMU that the device is behind.
Quoted 7 lines
I would very much prefer this works by inspecting the cfg from the iopgtable..
You can get it by doing
struct io_pgtable_cfg *pgtbl_cfg = &io_pgtable_ops_to_pgtable(domain->pgtbl_ops)->cfg;
OK. I will make it a detailed scan
Quoted 2 lines
I think it is important that the
domain->smmupointer be removed as well
I will try that too.
There are smmu_domain->smmu validations in two SVA functions, which presumably might be replaced with this can_share() too.
Thanks
Nicolin
Re: [PATCH v2 10/10] iommu/arm-smmu-v3: Allow sharing domain across SMMUs
Quoted 25 lines · Nicolin Chen
Quoted 14 lines · Jason Gunthorpe
Quoted 4 lines · Nicolin Chen
VMM needs a domain holding the mappings between gPA to hPA. It can be an S1 domain or an S2 nesting parent domain, depending on whether the VM is built with a vSMMU or not.
Quoted 8 lines
Given that the IOAS for this gPA mapping is the same across SMMU instances, this domain can be shared across devices even if they sit behind different SMMUs, so long as the underlying page table is compatible between the SMMU instances.
There is no direct information about the page table from the master device, but a comparison can be done between the physical SMMU that the domain was allocated for and the physical SMMU that the device is behind.
Quoted 7 lines
I would very much prefer this works by inspecting the cfg from the iopgtable..
You can get it by doing
struct io_pgtable_cfg *pgtbl_cfg = &io_pgtable_ops_to_pgtable(domain->pgtbl_ops)->cfg;OK. I will make it a detailed scan
Quoted 7 lines
Quoted 2 lines
I think it is important that the
domain->smmupointer be removed as wellI will try that too.
There are
smmu_domain->smmuvalidations in two SVA functions, which presumably might be replaced with thiscan_share()too.
Those are because we replicate the mmu notifier for every instance. That can now be revised too, I think. Let's leave that for another series after this and leave the smmu pointer for now, but don't use it outside that SVA stuff.
Jason