Re: [PATCH v2] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support

[Jason Gunthorpe]

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@xxxxxxxxxx>
> ---
> 
> Changelog
> v2:
>  * Rebased on top of Michael's series reworking CD table ownership:
>    https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@xxxxxxxxxx/
>  * Added a new ARM_SMMU_DOMAIN_BYPASS_S1DSS stage to tag the use case
> v1: https://lore.kernel.org/all/20230627033326.5236-1-nicolinc@xxxxxxxxxx/

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