Messages in this thread | | | Date | Thu, 17 Aug 2023 17:24:51 +0100 | Subject | Re: [PATCH v2] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support | From | Robin Murphy <> |
| |
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
| |