linux-kernel - Re: [PATCH v2 4/8] iommu/arm-smmu-v3: move stall

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]

Date:   Tue, 1 Aug 2023 10:31:02 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Michael Shavit <mshavit@...gle.com>
Cc:     Nicolin Chen <nicolinc@...dia.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 v2 4/8] iommu/arm-smmu-v3: move stall_enabled to the cd
 table

On Tue, Aug 01, 2023 at 04:09:52PM +0800, Michael Shavit wrote:
> > On Mon, Jul 31, 2023 at 09:28:40PM -0700, Nicolin Chen wrote:
> > > This cd_table->stall_enabled comes from master->stall_enabled, and
> > > cd_table will be in master structure. Also, struct arm_smmu_master
> > > pointer will be passed in to arm_smmu_write_ctx_desc(). So, there
> > > seems to be no need of master->cd_table.stall_enabled in the end;
> > > just use master->stall_enabled directly?
> 
> Yes it's correct that this change isn't strictly necessary. Thoughts jgg@ ?

I don't have a strong feeling here

The stall bits in the STE/CDTE should be set only for masters that
operate in stall mode.

I would hope a a single domain should be mixable between stall and PRI
masters?

If the cd_table is 1:1 with a master then keeping it in the master is
logical enough. If we ever imagine a CD table with multiple masters
then we'd need to have a bit in the cd_table too.


> On Tue, Aug 1, 2023 at 12:52 PM Nicolin Chen <nicolinc@...dia.com> wrote:
> > Actually the stall_enabled might still need to be per-CD/domain.
> > If a domain is attached by two masters. The domain->stall_enabled
> > is initialized with the first master->stall_enabled. Then, the
> > second master->stall_enabled would be required to match with the
> > domain->stall_enabled. arm_smmu_attach_dev() has such a sanity.
> >
> > So, I think we might not need this patch.
> 
> But why force domains attached to different masters to have the same
> stall_enabled setting? Whether stall is enabled is strictly a property
> of the master, not the domain. IMO the fact that it was stored in
> domain and checked in attach_dev was only because the previous design
> required it, not because it's more appropriate.

Yes, definately remove stall from the domains, the faulting flow
should learn the faulting mode based on the master that triggered the
fault, not the domain that received it.

Jason