linux-kernel - Re: [PATCH v5 5/7] iommu/arm-smmu-v3: Populate 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: <20251124231341.GO153257@nvidia.com>
Date: Mon, 24 Nov 2025 19:13:41 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Will Deacon <will@...nel.org>
Cc: Nicolin Chen <nicolinc@...dia.com>, jean-philippe@...aro.org,
	robin.murphy@....com, joro@...tes.org, balbirs@...dia.com,
	miko.lenczewski@....com, peterz@...radead.org, kevin.tian@...el.com,
	praan@...gle.com, linux-arm-kernel@...ts.infradead.org,
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/7] iommu/arm-smmu-v3: Populate smmu_domain->invs
 when attaching masters

On Mon, Nov 24, 2025 at 09:43:18PM +0000, Will Deacon wrote:
> > +	switch (smmu_domain->stage) {
> > +	case ARM_SMMU_DOMAIN_SVA:
> > +	case ARM_SMMU_DOMAIN_S1:
> > +		*cur = (struct arm_smmu_inv){
> > +			.smmu = master->smmu,
> > +			.type = INV_TYPE_S1_ASID,
> > +			.id = smmu_domain->cd.asid,
> > +			.size_opcode = e2h ? CMDQ_OP_TLBI_EL2_VA :
> > +					     CMDQ_OP_TLBI_NH_VA,
> > +			.nsize_opcode = e2h ? CMDQ_OP_TLBI_EL2_ASID :
> > +					      CMDQ_OP_TLBI_NH_ASID
> > +		};
> > +		break;
> > +	case ARM_SMMU_DOMAIN_S2:
> > +		*cur = (struct arm_smmu_inv){
> > +			.smmu = master->smmu,
> > +			.type = INV_TYPE_S2_VMID,
> > +			.id = smmu_domain->s2_cfg.vmid,
> > +			.size_opcode = CMDQ_OP_TLBI_S2_IPA,
> > +			.nsize_opcode = CMDQ_OP_TLBI_S12_VMALL,
> > +		};
> > +		break;
> 
> Having a helper to add an invalidation command would make this a little
> more compact and you could also check against the size of the array.

Yeah but it makes all the parameters positional instead of nicely
named..

> > +/* Must be installed before arm_smmu_install_ste_for_dev() */
> > +static void
> > +arm_smmu_install_new_domain_invs(struct arm_smmu_attach_state *state)
> > +{
> > +	struct arm_smmu_inv_state *invst = &state->new_domain_invst;
> > +
> > +	if (!invst->invs_ptr)
> > +		return;
> > +
> > +	rcu_assign_pointer(*invst->invs_ptr, invst->new_invs);
> > +	/*
> > +	 * We are committed to updating the STE. Ensure the invalidation array
> > +	 * is visable to concurrent map/unmap threads, and acquire any racying
> > +	 * IOPTE updates.
> > +	 */
> > +	smp_mb();
> 
> Sorry, but the comment hasn't really helped me here. We're ordering the
> publishing of the invalidation array above before ... what?

"concurrent map/unmap threads" means other threads calling
arm_smmu_iotlb_sync().. I think the matching comment describing this
and introducing the pair'd smp_mb() is in a later patch.

Either the HW observes the latest page table and needs no invalidation
or arm_smmu_iotlb_sync observes the invalidation list and issues
invalidation.

The smb_mb() is intended to make one of the two statements true prior
to storing the STE.

Nicolin maybe this barrier line should be moved to be added in the
patch that add's the pair'd barrier and description?

Jason