Re: [PATCH v2 4/8] iommu/arm-smmu-v3: move stall_enabled to the cd table

[Jason Gunthorpe <jgg@nvidia.com>]

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