Messages in this thread Patch in this message | | | Date | Tue, 27 Jun 2023 10:06:52 -0700 | From | Nicolin Chen <> | Subject | Re: [PATCH v1] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support |
| |
Hi Robin,
On Tue, Jun 27, 2023 at 10:00:18AM +0100, Robin Murphy wrote: > On 2023-06-27 04:33, Nicolin Chen wrote: > > When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver would > > skip the allocation of a CD table and set the CONFIG field of the STE > > to STRTAB_STE_0_CFG_BYPASS. This works well for devices that only have > > one substream, i.e. PASID disabled. > > > > However, there could be a use case, for a pasid capable device, that > > allows bypassing the translation at the default substream while still > > enabling the pasid feature, which means the driver should not skip the > > allocation of a CD table nor simply bypass the CONFIG field. Instead, > > the S1DSS field should be set to STRTAB_STE_1_S1DSS_BYPASS and the > > SHCFG field should be set to STRTAB_STE_1_SHCFG_INCOMING. > > > > Add s1dss and shcfg in struct arm_smmu_s1_cfg, to allow configurations > > in the finalise() to support that use case. Then, set them accordingly > > depending on the iommu_domain->type and the master->ssid_bits. > > > > Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration > > to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one. > > Why? The "default configuration" is that the S1 shareability attribute > is determined by the S1 translation itself, so the incoming value is > irrelevant.
That was for a consistency since the driver set the SHCFG field to 0x0 (STRTAB_STE_1_SHCFG_NONSHAREABLE). I was not quite sure, in a long run, if leaving an uncleared s1_cfg->shcfg potentially can result in an unexpected behavior if it's passed in the STE. Yet, we could be seemingly sure that the !IOMMU_DOMAIN_IDENTITY means the S1 translation must be enabled and so the SHCFG would be irrelevant?
If so, I make make it:
+ if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) { + cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS; + cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING; + } else { + cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0; + }
> > @@ -2198,7 +2206,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain, > > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > > struct arm_smmu_device *smmu = smmu_domain->smmu; > > > > - if (domain->type == IOMMU_DOMAIN_IDENTITY) { > > + /* > > + * A master with a pasid capability might need a CD table, so only set > > + * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master > > + */ > > + if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) { > > smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS; > > return 0; > > } > > This means we'll now go on to allocate a pagetable for an identity > domain, which doesn't seem ideal :/
Do you suggest to bypass alloc_io_pgtable_ops()? That would zero out the TCR fields in the CD. Not sure if it'd work seamlessly, but I can give it a try.
Thanks Nic
| |