Messages in this thread Patch in this message | | | Date | Thu, 17 Aug 2023 09:58:54 -0700 | From | Nicolin Chen <> | Subject | Re: [PATCH v2] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support |
| |
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
| |