linux-kernel - Re: [PATCH v4 6/8] iommu/arm-smmu-v3: Move CD table to arm_smmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZM2FrQcD8QzP6XM0@Asurada-Nvidia>
Date:   Fri, 4 Aug 2023 16:11:41 -0700
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     Michael Shavit <mshavit@...gle.com>, <iommu@...ts.linux.dev>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <will@...nel.org>,
        <robin.murphy@....com>, <jean-philippe@...aro.org>
Subject: Re: [PATCH v4 6/8] iommu/arm-smmu-v3: Move CD table to
 arm_smmu_master

On Fri, Aug 04, 2023 at 07:46:23PM -0300, Jason Gunthorpe wrote:
> On Fri, Aug 04, 2023 at 03:25:43PM -0700, Nicolin Chen wrote:
> > > @@ -2436,22 +2419,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> > >         if (!smmu_domain->smmu) {
> > >                 smmu_domain->smmu = smmu;
> > >                 ret = arm_smmu_domain_finalise(domain, master);
> > > -               if (ret) {
> > > +               if (ret)
> > >                         smmu_domain->smmu = NULL;
> > > -                       goto out_unlock;
> > > -               }
> > > -       } else if (smmu_domain->smmu != smmu) {
> > > +       } else if (smmu_domain->smmu != smmu)
> > >                 ret = -EINVAL;
> > > -               goto out_unlock;
> > > -       } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
> > > -                  master->ssid_bits != smmu_domain->cd_table.max_cds_bits) {
> > > -               ret = -EINVAL;
> > > -               goto out_unlock;
> > > -       } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
> > > -                  smmu_domain->cd_table.stall_enabled != master->stall_enabled) {
> > > -               ret = -EINVAL;
> > > -               goto out_unlock;
> > > -       }
> > 
> > ... then we remove this stall_enabled sanity also.
> > 
> > This means a shared domain (holding a shared CD) being inserted
> > to two CD tables from two masters would have two different CDTE
> > configurations at the stall bit.
> 
> I looked through the spec for a while and I thought this was fine..
> 
> Stall is basically a master specific behavior on how to operate page
> faulting. It makes sense that it follows the master and the IOPTEs in
> the domain can be used with both the faulting and non-faulting page
> faulting path.
>
> I would expect the page faulting path to figure out what to (if there
> is anything special to do) do based on the master that triggered the
> fault, not based on the domain that received it.

Yea, I went through the spec too yet didn't find anything that
could block us. And there is no SW dependency on the STALL bit
of the CDTE: actually it has an inverse relationship with the
S1STALLD bit in the STE, so following the STE/cd_table/master
makes sense. So long as a master has its own cd_table holding
its own CDTE for a shared domain, HW CD caching should be fine
as well.

With that being said, I think mentioning this behavior change
in the commit log wouldn't hurt. Someday people might want to
check this out in case something breaks.

Thanks
Nic