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: <aLvs8WrxEHpykCT/@nvidia.com>
Date: Sat, 6 Sep 2025 01:12:33 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <will@...nel.org>, <robin.murphy@....com>, <joro@...tes.org>,
	<jean-philippe@...aro.org>, <miko.lenczewski@....com>, <balbirs@...dia.com>,
	<peterz@...radead.org>, <smostafa@...gle.com>, <kevin.tian@...el.com>,
	<praan@...gle.com>, <zhangzekun11@...wei.com>,
	<linux-arm-kernel@...ts.infradead.org>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH rfcv1 7/8] iommu/arm-smmu-v3: Add arm_smmu_invs based
 arm_smmu_domain_inv_range()

On Wed, Aug 27, 2025 at 03:49:23PM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 13, 2025 at 06:25:38PM -0700, Nicolin Chen wrote:
> > +again:
> > +	invs = rcu_dereference(smmu_domain->invs);
> > +
> > +	/* A concurrent attachment might have changed the array. Do a respin */
> > +	if (unlikely(!read_trylock(&invs->rwlock)))
> > +		goto again;
> > +	/* Only one retry. Otherwise, it would soft lockup on an empty array */
> > +	if (!retried && unlikely(!invs->num_invs)) {
> > +		read_unlock(&invs->rwlock);
> > +		retried = true;
> > +		goto again;
> > +	}
> 
> This has missed the point, it was to not get the unless we have
> ATS. Something like this:

I recall one of my earlier local versions put the ATS and blocked
conditions on the attach side. So, here it could be unconditional
because for most of the time this would be nearly a NOP, until an
attachment hits the FLR case.

Maybe those conditions got lost during the rework prior to rfcv1,
so here this ended up with missing a big thing...

> 	rcu_read_lock();
> 
> 	while (true) {
> 		invs = rcu_dereference(smmu_domain->invs);
> 
> 		/*
> 		 * Avoid locking unless ATS is being used. No ATS invalidate can
> 		 * be going on after a domain is detached.
> 		 */
> 		locked = false;
> 		if (invs->has_ats || READ_ONCE(invs->old)) {
> 			read_lock(&invs->rwlock);
> 			if (invs->old) {
> 				read_unlock(&invs->rwlock);
> 				continue;
> 			}
> 			locked = true;
> 		}
> 		break;
> 	}

I know that performance-wise, this piece will be a quick respin,
as the attach side releases the lock very fast. It still looks
a bit complicated. And practically, it would respin even if the
attachment removes a non-PCI device, right?

Applying the condition in the attachment on the other hand will
be accurate and simple "if (master->ats_enabled)"?

Thanks
Nicolin

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ