[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZN5RzvGhwRvVKUDy@Asurada-Nvidia>
Date: Thu, 17 Aug 2023 09:58:54 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>,
Robin Murphy <robin.murphy@....com>
CC: <will@...nel.org>, <joro@...tes.org>, <mshavit@...gle.com>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <iommu@...ts.linux.dev>
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
Powered by blists - more mailing lists