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

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

[Nicolin Chen]

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(-)

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 {

base-commit: aed5d77d0c3d55d1949db89f27cf7a3981261ef4
-- 
2.41.0

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@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

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

[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

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

[Jason Gunthorpe]

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

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

[Nicolin Chen]

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

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

[Jason Gunthorpe]

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