Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Date:   Tue, 27 Jun 2023 17:13:14 -0700
From:   Nicolin Chen <nicolinc@nvidia.com>
To:     Robin Murphy <robin.murphy@arm.com>
CC:     <will@kernel.org>, <jgg@nvidia.com>, <joro@8bytes.org>,
        <shameerali.kolothum.thodi@huawei.com>, <yangyicong@hisilicon.com>,
        <jean-philippe@linaro.org>, <linux-arm-kernel@lists.infradead.org>,
        <iommu@lists.linux.dev>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1] iommu/arm-smmu-v3: Allow default substream bypass
 with a pasid support
Message-ID: <ZJt7GhV4IOLvRhhr@Asurada-Nvidia>
References: <20230627033326.5236-1-nicolinc@nvidia.com>
 <826cbb86-ac7d-c40b-f7e3-51681cda50b8@arm.com>
 <ZJsXLFtH8WkRK41w@Asurada-Nvidia>
 <1b0c9677-4e2a-6767-f8f2-23081e6af3e0@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1b0c9677-4e2a-6767-f8f2-23081e6af3e0@arm.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jun 2023 00:13:26.0860
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 9d119017-ffa9-403a-b1c7-08db776c7e52
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT094.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5064
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Thanks for the reply.

On Wed, Jun 28, 2023 at 12:29:52AM +0100, Robin Murphy wrote:

> > > > 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;
> > +     }
> 
> What I mean is we don't need a cfg->shcfg field at all - without loss of
> generality it can simply be hard-coded to 1 when S1 is active, same as
> for stream bypass.

OK.
--------------------------------------------------------------------------------------------------
@@ -1350,7 +1350,8 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
                dst[1] = cpu_to_le64(
-                        FIELD_PREP(STRTAB_STE_1_S1DSS, STRTAB_STE_1_S1DSS_SSID0) |
+                        FIELD_PREP(STRTAB_STE_1_S1DSS, s1_cfg->s1dss) |
+                        FIELD_PREP(STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING) |
                         FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
--------------------------------------------------------------------------------------------------

> The only case where explicitly setting STE.SHCFG=0 makes some sense is
> for a stage-2-only domain if a device's incoming attribute is stronger
> than it needs to be, but even then there are multiple levels of
> IMP-DEFness around whether SHCFG actually does anything anyway.

I see. Thanks for elaborating.

> > > > @@ -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.
> 
> I think if there's a good reason to support this then it's worth

There is an unignorable perf difference that we see on a real HW.
So the reason or (I should say) the requirement is pretty strong.

> supporting properly, i.e. refactor a bit harder to separate the CD table
> parts which are common to both S1DSS bypass and S1 translation, from the
> CD/pagetable parts that are only relevant for translation. S1DSS bypass
> remains the same as Stream bypass in the sense that there is no
> structure corresponding to the identity domain itself, so not only does
> it not make sense to have a pagetable, there's also no valid place to
> put one anyway - touching the CD belonging to SSID 0 is strictly wrong.

I can try that. Yet, I think the S1DSS bypass case still belongs
to ARM_SMMU_DOMAIN_S1/arm_smmu_domain_finalise_s1, right?

I'd try keeping most of the parts intact while adding a pointer
to a structure holding pagetable stuff, to make it cleaner. Then
the S1DSS bypass case can be flagged by an empty pointer.

Thanks
Nic