| Message ID | 20230817042135.32822-1-nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support | expand |
On Wed, Aug 16, 2023 at 09:21:35PM -0700, Nicolin Chen wrote: > When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver sets the > arm_smmu_domain->stage to ARM_SMMU_DOMAIN_BYPASS and skips the allocation > of a CD table, and then sets STRTAB_STE_0_CFG_BYPASS to the CONFIG field > of the STE. This works well for devices that only have one substream, i.e. > pasid disabled. > > With a pasid-capable device, however, there could be a use case where it > allows an IDENTITY domain attachment without disabling its pasid feature. > This requires the driver to allocate a multi-entry CD table to attach the > IDENTITY domain to its default substream and to configure the S1DSS filed > of the STE to STRTAB_STE_1_S1DSS_BYPASS. So, there is a missing link here > between the STE setup and an IDENTITY domain attachment. > > Add a new stage ARM_SMMU_DOMAIN_BYPASS_S1DSS to tag this configuration by > overriding the ARM_SMMU_DOMAIN_BYPASS if the device has pasid capability. > This new tag will allow the driver allocating a CD table yet skipping an > CD insertion from the IDENTITY domain, and setting up the STE accordingly. > > In a use case of ARM_SMMU_DOMAIN_BYPASS_S1DSS, the SHCFG field of the STE > should be set to STRTAB_STE_1_SHCFG_INCOMING. In other cases of having a > CD table, the shareability comes from a CD, not the SHCFG field: according > to "13.5 Summary of attribute/permission configuration fields" in the spec > the SHCFG field value is irrelevant. So, always configure the SHCFG field > of the STE to STRTAB_STE_1_SHCFG_INCOMING when a CD table is present, for > simplification. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > > Changelog > v2: > * Rebased on top of Michael's series reworking CD table ownership: > https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@google.com/ > * Added a new ARM_SMMU_DOMAIN_BYPASS_S1DSS stage to tag the use case > v1: https://lore.kernel.org/all/20230627033326.5236-1-nicolinc@nvidia.com/ After rebasing there really shouldn't be a ARM_SMMU_DOMAIN_BYPASS_S1DSS. I want to get to a model where the identity domain is a global static, so it can't be changing depending on how it is attched. I continue to think that the right way to think about this is to have the CD table code generate the STE it wants and when doing so it will inspect what SSID0 is. If it is the IDENTITY domain then it fills s1dss / etc > 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 b27011b2bec9..860db4fbb995 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1271,6 +1271,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, > * 3. Update Config, sync > */ > u64 val = le64_to_cpu(dst[0]); > + u8 s1dss = STRTAB_STE_1_S1DSS_SSID0; > bool ste_live = false; > struct arm_smmu_device *smmu = NULL; > struct arm_smmu_ctx_desc_cfg *cd_table = NULL; > @@ -1290,6 +1291,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, > > if (smmu_domain) { > switch (smmu_domain->stage) { > + case ARM_SMMU_DOMAIN_BYPASS_S1DSS: > + s1dss = STRTAB_STE_1_S1DSS_BYPASS; > + fallthrough; > case ARM_SMMU_DOMAIN_S1: > cd_table = &master->cd_table; > break; Eg, I think the code looks much nicer if the logic here is more like: if (master->cd_table.cdtab) arm_smmu_cd_table_get_ste(master->cd_table, &ste) else if (master->domain) arm_smmu_domain_get_ste(master->domain, &ste); else ste = not attached And you'd check in arm_smmu_cd_table_get_ste() to learn the CD parameters and also what SSID=0 is. If SSID=0 is IDENTITY then arm_smmu_cd_table_get_ste would return with S1DSS set. arm_smmu_domain_get_ste() would multiplex based on the domain type. > @@ -2435,6 +2440,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > } else if (smmu_domain->smmu != smmu) > ret = -EINVAL; > > + /* > + * When attaching an IDENTITY domain to a master with pasid capability, > + * the master can still enable SVA feature by allocating a multi-entry > + * CD table and attaching the IDENTITY domain to its default substream > + * that alone can be byassed using the S1DSS field of the STE. > + */ > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS && master->ssid_bits && > + smmu->features & ARM_SMMU_FEAT_TRANS_S1) > + smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS_S1DSS; Then you don't technically need to do this. Though if we can't atomically change the STE from IDENTITY to IDENTIY with a CD then you still have to do something here, but really what we want is to force a CD table for all cases if PASID is enabled, and force DMA domains to be S2 domains as well. > mutex_unlock(&smmu_domain->init_mutex); > if (ret) > return ret; > @@ -2456,7 +2471,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > list_add(&master->domain_head, &smmu_domain->devices); > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 || > + smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS_S1DSS) { > if (!master->cd_table.cdtab) { > ret = arm_smmu_alloc_cd_tables(master); > if (ret) { So more like: if (smmu_domain == IDENTIY && arm_smmu_support_ssid(dev)) arm_smmu_alloc_cd_tables() Jason
On 2023-08-17 16:20, Jason Gunthorpe wrote: > On Wed, Aug 16, 2023 at 09:21:35PM -0700, Nicolin Chen wrote: >> When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver sets the >> arm_smmu_domain->stage to ARM_SMMU_DOMAIN_BYPASS and skips the allocation >> of a CD table, and then sets STRTAB_STE_0_CFG_BYPASS to the CONFIG field >> of the STE. This works well for devices that only have one substream, i.e. >> pasid disabled. >> >> With a pasid-capable device, however, there could be a use case where it >> allows an IDENTITY domain attachment without disabling its pasid feature. >> This requires the driver to allocate a multi-entry CD table to attach the >> IDENTITY domain to its default substream and to configure the S1DSS filed >> of the STE to STRTAB_STE_1_S1DSS_BYPASS. So, there is a missing link here >> between the STE setup and an IDENTITY domain attachment. >> >> Add a new stage ARM_SMMU_DOMAIN_BYPASS_S1DSS to tag this configuration by >> overriding the ARM_SMMU_DOMAIN_BYPASS if the device has pasid capability. >> This new tag will allow the driver allocating a CD table yet skipping an >> CD insertion from the IDENTITY domain, and setting up the STE accordingly. >> >> In a use case of ARM_SMMU_DOMAIN_BYPASS_S1DSS, the SHCFG field of the STE >> should be set to STRTAB_STE_1_SHCFG_INCOMING. In other cases of having a >> CD table, the shareability comes from a CD, not the SHCFG field: according >> to "13.5 Summary of attribute/permission configuration fields" in the spec >> the SHCFG field value is irrelevant. So, always configure the SHCFG field >> of the STE to STRTAB_STE_1_SHCFG_INCOMING when a CD table is present, for >> simplification. >> >> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> >> --- >> >> Changelog >> v2: >> * Rebased on top of Michael's series reworking CD table ownership: >> https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@google.com/ >> * Added a new ARM_SMMU_DOMAIN_BYPASS_S1DSS stage to tag the use case >> v1: https://lore.kernel.org/all/20230627033326.5236-1-nicolinc@nvidia.com/ > > After rebasing there really shouldn't be a > ARM_SMMU_DOMAIN_BYPASS_S1DSS. I want to get to a model where the > identity domain is a global static, so it can't be changing depending > on how it is attched. > > I continue to think that the right way to think about this is to have > the CD table code generate the STE it wants and when doing so it will > inspect what SSID0 is. If it is the IDENTITY domain then it fills > s1dss / etc Indeed, that's what I was getting at with "generalisation of ARM_SMMU_DOMAIN_BYPASS based on s1cdmax" - just one type all the way down to the bowels of arm_smmu_write_strtab_ent(), which then decides whether it means touching S1DSS or Config in the given STE. >> 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 b27011b2bec9..860db4fbb995 100644 >> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> @@ -1271,6 +1271,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, >> * 3. Update Config, sync >> */ >> u64 val = le64_to_cpu(dst[0]); >> + u8 s1dss = STRTAB_STE_1_S1DSS_SSID0; >> bool ste_live = false; >> struct arm_smmu_device *smmu = NULL; >> struct arm_smmu_ctx_desc_cfg *cd_table = NULL; >> @@ -1290,6 +1291,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, >> >> if (smmu_domain) { >> switch (smmu_domain->stage) { >> + case ARM_SMMU_DOMAIN_BYPASS_S1DSS: >> + s1dss = STRTAB_STE_1_S1DSS_BYPASS; >> + fallthrough; >> case ARM_SMMU_DOMAIN_S1: >> cd_table = &master->cd_table; >> break; > > Eg, I think the code looks much nicer if the logic here is more like: > > if (master->cd_table.cdtab) > arm_smmu_cd_table_get_ste(master->cd_table, &ste) > else if (master->domain) > arm_smmu_domain_get_ste(master->domain, &ste); > else > ste = not attached > > And you'd check in arm_smmu_cd_table_get_ste() to learn the CD > parameters and also what SSID=0 is. If SSID=0 is IDENTITY then > arm_smmu_cd_table_get_ste would return with S1DSS set. > > arm_smmu_domain_get_ste() would multiplex based on the domain type. > >> @@ -2435,6 +2440,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) >> } else if (smmu_domain->smmu != smmu) >> ret = -EINVAL; >> >> + /* >> + * When attaching an IDENTITY domain to a master with pasid capability, >> + * the master can still enable SVA feature by allocating a multi-entry >> + * CD table and attaching the IDENTITY domain to its default substream >> + * that alone can be byassed using the S1DSS field of the STE. >> + */ >> + if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS && master->ssid_bits && >> + smmu->features & ARM_SMMU_FEAT_TRANS_S1) >> + smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS_S1DSS; > > Then you don't technically need to do this. > > Though if we can't atomically change the STE from IDENTITY to IDENTIY > with a CD then you still have to do something here, Strictly I think we are safe to do that - fill in all the S1* fields while Config[0] is still 0 and they're ignored, sync, then set Config[0]. Adding a CD table under a translation domain should be achievable as well, since S1CDMax, S1ContextPtr and S1Fmt can all be updated together atomically (although it's still the kind of switcheroo where I'd be scared of a massive boulder suddenly rolling out of the ceiling...) > but really what we > want is to force a CD table for all cases if PASID is enabled, and > force DMA domains to be S2 domains as well. Wut? No, DMA domains really want to be stage 1, for many reasons. Implementing them with stage 2 when stage 1 isn't supported was always a bit of a bodge, but thankfully I'm not aware of anyone ever building a stage-2-only SMMUv3 anyway. The most glaringly obvious one, though, is that I think people like PASID support and SVA to actually work ;) Thanks, Robin. >> mutex_unlock(&smmu_domain->init_mutex); >> if (ret) >> return ret; >> @@ -2456,7 +2471,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) >> list_add(&master->domain_head, &smmu_domain->devices); >> spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); >> >> - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { >> + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 || >> + smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS_S1DSS) { >> if (!master->cd_table.cdtab) { >> ret = arm_smmu_alloc_cd_tables(master); >> if (ret) { > > So more like: > > if (smmu_domain == IDENTIY && arm_smmu_support_ssid(dev)) > arm_smmu_alloc_cd_tables() > > Jason
On Thu, Aug 17, 2023 at 05:24:51PM +0100, Robin Murphy wrote: > > > @@ -2435,6 +2440,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > > } else if (smmu_domain->smmu != smmu) > > > ret = -EINVAL; > > > + /* > > > + * When attaching an IDENTITY domain to a master with pasid capability, > > > + * the master can still enable SVA feature by allocating a multi-entry > > > + * CD table and attaching the IDENTITY domain to its default substream > > > + * that alone can be byassed using the S1DSS field of the STE. > > > + */ > > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS && master->ssid_bits && > > > + smmu->features & ARM_SMMU_FEAT_TRANS_S1) > > > + smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS_S1DSS; > > > > Then you don't technically need to do this. > > > > Though if we can't atomically change the STE from IDENTITY to IDENTIY > > with a CD then you still have to do something here, > > Strictly I think we are safe to do that - fill in all the S1* fields while > Config[0] is still 0 and they're ignored, sync, then set Config[0]. Adding a > CD table under a translation domain should be achievable as well, since > S1CDMax, S1ContextPtr and S1Fmt can all be updated together atomically > (although it's still the kind of switcheroo where I'd be scared of a massive > boulder suddenly rolling out of the ceiling...) That sounds pretty good then we don't have to force staying in CD mode > > but really what we > > want is to force a CD table for all cases if PASID is enabled, and > > force DMA domains to be S2 domains as well. > > Wut? No, DMA domains really want to be stage 1, for many reasons. > Implementing them with stage 2 when stage 1 isn't supported was always a bit > of a bodge, but thankfully I'm not aware of anyone ever building a > stage-2-only SMMUv3 anyway. > > The most glaringly obvious one, though, is that I think people like PASID > support and SVA to actually work ;) Blah, I keep doing this and swapping S1/S2 in this confusing naming scheme (and it doesn't help that AMD is backwards, sigh) - I ment what you said :) Thanks, Jason
Hi, Thank you both for the reviews! On Thu, Aug 17, 2023 at 05:24:51PM +0100, Robin Murphy wrote: > > > Changelog > > > v2: > > > * Rebased on top of Michael's series reworking CD table ownership: > > > https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@google.com/ > > > * Added a new ARM_SMMU_DOMAIN_BYPASS_S1DSS stage to tag the use case > > > v1: https://lore.kernel.org/all/20230627033326.5236-1-nicolinc@nvidia.com/ > > > > After rebasing there really shouldn't be a > > ARM_SMMU_DOMAIN_BYPASS_S1DSS. I want to get to a model where the > > identity domain is a global static, so it can't be changing depending > > on how it is attched. > > > > I continue to think that the right way to think about this is to have > > the CD table code generate the STE it wants and when doing so it will > > inspect what SSID0 is. If it is the IDENTITY domain then it fills > > s1dss / etc > > Indeed, that's what I was getting at with "generalisation of > ARM_SMMU_DOMAIN_BYPASS based on s1cdmax" - just one type all the way > down to the bowels of arm_smmu_write_strtab_ent(), which then decides > whether it means touching S1DSS or Config in the given STE. Ack. Let me retry with that. > > > @@ -1290,6 +1291,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, > > > > > > if (smmu_domain) { > > > switch (smmu_domain->stage) { > > > + case ARM_SMMU_DOMAIN_BYPASS_S1DSS: > > > + s1dss = STRTAB_STE_1_S1DSS_BYPASS; > > > + fallthrough; > > > case ARM_SMMU_DOMAIN_S1: > > > cd_table = &master->cd_table; > > > break; > > > > Eg, I think the code looks much nicer if the logic here is more like: > > > > if (master->cd_table.cdtab) > > arm_smmu_cd_table_get_ste(master->cd_table, &ste) So, this means that cd_table is present, indicating either "S1 translated" or "S1 enabled but S1DSS"... > > else if (master->domain) > > arm_smmu_domain_get_ste(master->domain, &ste); ... and this means that cd_table isn't present, indicating S1 bypass or S2.... > > else > > ste = not attached > > > > And you'd check in arm_smmu_cd_table_get_ste() to learn the CD > > parameters and also what SSID=0 is. If SSID=0 is IDENTITY then > > arm_smmu_cd_table_get_ste would return with S1DSS set. > > > > arm_smmu_domain_get_ste() would multiplex based on the domain type. ... it then means we need arm_smmu_write_ctx_desc() also when attaching an IDENTITY domain. Will try with that. Thanks for the guidance! > > > @@ -2435,6 +2440,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > > } else if (smmu_domain->smmu != smmu) > > > ret = -EINVAL; > > > > > > + /* > > > + * When attaching an IDENTITY domain to a master with pasid capability, > > > + * the master can still enable SVA feature by allocating a multi-entry > > > + * CD table and attaching the IDENTITY domain to its default substream > > > + * that alone can be byassed using the S1DSS field of the STE. > > > + */ > > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS && master->ssid_bits && > > > + smmu->features & ARM_SMMU_FEAT_TRANS_S1) > > > + smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS_S1DSS; > > > > Then you don't technically need to do this. Yea, wasn't so confident about it either. Will drop. > > > @@ -2456,7 +2471,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > > list_add(&master->domain_head, &smmu_domain->devices); > > > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > > > > > - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 || > > > + smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS_S1DSS) { > > > if (!master->cd_table.cdtab) { > > > ret = arm_smmu_alloc_cd_tables(master); > > > if (ret) { > > > > So more like: > > > > if (smmu_domain == IDENTIY && arm_smmu_support_ssid(dev)) > > arm_smmu_alloc_cd_tables() OK. ARM_SMMU_DOMAIN_S1 with ssid=0 still needs a cd_table though. Thanks! Nic
On Thu, Aug 17, 2023 at 09:58:54AM -0700, Nicolin Chen wrote: > > > > @@ -1290,6 +1291,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, > > > > > > > > if (smmu_domain) { > > > > switch (smmu_domain->stage) { > > > > + case ARM_SMMU_DOMAIN_BYPASS_S1DSS: > > > > + s1dss = STRTAB_STE_1_S1DSS_BYPASS; > > > > + fallthrough; > > > > case ARM_SMMU_DOMAIN_S1: > > > > cd_table = &master->cd_table; > > > > break; > > > > > > Eg, I think the code looks much nicer if the logic here is more like: > > > > > > if (master->cd_table.cdtab) > > > arm_smmu_cd_table_get_ste(master->cd_table, &ste) > > So, this means that cd_table is present, indicating either "S1 > translated" or "S1 enabled but S1DSS"... Yes, if cd table is present then cd table always provides the STE. So this is S1 page tables on any SSID or SSID=0 IDENTITY > > > else if (master->domain) > > > arm_smmu_domain_get_ste(master->domain, &ste); > > ... and this means that cd_table isn't present, indicating S1 > bypass or S2.... Meaning S2 page table on the RID or IDENTITY. > > > And you'd check in arm_smmu_cd_table_get_ste() to learn the CD > > > parameters and also what SSID=0 is. If SSID=0 is IDENTITY then > > > arm_smmu_cd_table_get_ste would return with S1DSS set. > > > > > > arm_smmu_domain_get_ste() would multiplex based on the domain type. > > ... it then means we need arm_smmu_write_ctx_desc() also when > attaching an IDENTITY domain. Yes, if a cd_table is present then it would be good to always write a consistent CD table entry meaning IDENTITY for SSID. Even if all this does is zero out the CD table entry. > > > > @@ -2456,7 +2471,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > > > list_add(&master->domain_head, &smmu_domain->devices); > > > > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > > > > > > > - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > > > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 || > > > > + smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS_S1DSS) { > > > > if (!master->cd_table.cdtab) { > > > > ret = arm_smmu_alloc_cd_tables(master); > > > > if (ret) { > > > > > > So more like: > > > > > > if (smmu_domain == IDENTIY && arm_smmu_support_ssid(dev)) > > > arm_smmu_alloc_cd_tables() > > OK. ARM_SMMU_DOMAIN_S1 with ssid=0 still needs a cd_table though. Yes, but the atomic ste update is a bit nicer, though it can be something to do separately as it is just saving a bit of cache. Jason
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 b27011b2bec9..860db4fbb995 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1271,6 +1271,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, * 3. Update Config, sync */ u64 val = le64_to_cpu(dst[0]); + u8 s1dss = STRTAB_STE_1_S1DSS_SSID0; bool ste_live = false; struct arm_smmu_device *smmu = NULL; struct arm_smmu_ctx_desc_cfg *cd_table = NULL; @@ -1290,6 +1291,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, if (smmu_domain) { switch (smmu_domain->stage) { + case ARM_SMMU_DOMAIN_BYPASS_S1DSS: + s1dss = STRTAB_STE_1_S1DSS_BYPASS; + fallthrough; case ARM_SMMU_DOMAIN_S1: cd_table = &master->cd_table; break; @@ -1348,7 +1352,8 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, BUG_ON(ste_live); dst[1] = cpu_to_le64( - FIELD_PREP(STRTAB_STE_1_S1DSS, STRTAB_STE_1_S1DSS_SSID0) | + FIELD_PREP(STRTAB_STE_1_S1DSS, s1dss) | + FIELD_PREP(STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING) | FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) | FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_WBRA) | FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_ISH) | @@ -2435,6 +2440,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } else if (smmu_domain->smmu != smmu) ret = -EINVAL; + /* + * When attaching an IDENTITY domain to a master with pasid capability, + * the master can still enable SVA feature by allocating a multi-entry + * CD table and attaching the IDENTITY domain to its default substream + * that alone can be byassed using the S1DSS field of the STE. + */ + if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS && master->ssid_bits && + smmu->features & ARM_SMMU_FEAT_TRANS_S1) + smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS_S1DSS; + mutex_unlock(&smmu_domain->init_mutex); if (ret) return ret; @@ -2456,7 +2471,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) list_add(&master->domain_head, &smmu_domain->devices); spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 || + smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS_S1DSS) { if (!master->cd_table.cdtab) { ret = arm_smmu_alloc_cd_tables(master); if (ret) { @@ -2464,7 +2480,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) goto out_list_del; } } + } + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { /* * Prevent SVA from concurrently modifying the CD or writing to * the CD entry diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index b7a91c8e9b52..e9361f85c91c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -714,6 +714,7 @@ enum arm_smmu_domain_stage { ARM_SMMU_DOMAIN_S2, ARM_SMMU_DOMAIN_NESTED, ARM_SMMU_DOMAIN_BYPASS, + ARM_SMMU_DOMAIN_BYPASS_S1DSS, /* Bypass S1 default substream only */ }; struct arm_smmu_domain {
When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver sets the arm_smmu_domain->stage to ARM_SMMU_DOMAIN_BYPASS and skips the allocation of a CD table, and then sets STRTAB_STE_0_CFG_BYPASS to the CONFIG field of the STE. This works well for devices that only have one substream, i.e. pasid disabled. With a pasid-capable device, however, there could be a use case where it allows an IDENTITY domain attachment without disabling its pasid feature. This requires the driver to allocate a multi-entry CD table to attach the IDENTITY domain to its default substream and to configure the S1DSS filed of the STE to STRTAB_STE_1_S1DSS_BYPASS. So, there is a missing link here between the STE setup and an IDENTITY domain attachment. Add a new stage ARM_SMMU_DOMAIN_BYPASS_S1DSS to tag this configuration by overriding the ARM_SMMU_DOMAIN_BYPASS if the device has pasid capability. This new tag will allow the driver allocating a CD table yet skipping an CD insertion from the IDENTITY domain, and setting up the STE accordingly. In a use case of ARM_SMMU_DOMAIN_BYPASS_S1DSS, the SHCFG field of the STE should be set to STRTAB_STE_1_SHCFG_INCOMING. In other cases of having a CD table, the shareability comes from a CD, not the SHCFG field: according to "13.5 Summary of attribute/permission configuration fields" in the spec the SHCFG field value is irrelevant. So, always configure the SHCFG field of the STE to STRTAB_STE_1_SHCFG_INCOMING when a CD table is present, for simplification. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- Changelog v2: * Rebased on top of Michael's series reworking CD table ownership: https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@google.com/ * Added a new ARM_SMMU_DOMAIN_BYPASS_S1DSS stage to tag the use case v1: https://lore.kernel.org/all/20230627033326.5236-1-nicolinc@nvidia.com/ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 +++++++++++++++++++-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + 2 files changed, 21 insertions(+), 2 deletions(-) base-commit: aed5d77d0c3d55d1949db89f27cf7a3981261ef4