Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Fri, 22 Sep 2023 20:20:57 +0200
From:   Petr =?UTF-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
To:     Catalin Marinas <catalin.marinas@arm.com>
Cc:     Christoph Hellwig <hch@lst.de>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Robin Murphy <robin.murphy@arm.com>,
        "open list:DMA MAPPING HELPERS" <iommu@lists.linux.dev>,
        open list <linux-kernel@vger.kernel.org>,
        Roberto Sassu <roberto.sassu@huaweicloud.com>,
        Jonathan Corbet <corbet@lwn.net>
Subject: Re: [PATCH] swiotlb: fix the check whether a device has used
 software IO TLB
Message-ID: <20230922202057.47d93603@meshulam.tesarici.cz>
In-Reply-To: <20230922191213.379d440a@meshulam.tesarici.cz>
References: <20230913114016.17752-1-petr@tesarici.cz>
        <20230913121403.GB4544@lst.de>
        <20230913142656.29e135d6@meshulam.tesarici.cz>
        <ZQNQscYr0rQWdw66@arm.com>
        <20230915111343.01496320@meshulam.tesarici.cz>
        <ZQSPyBRjnSISNFmD@arm.com>
        <20230917114741.01a23364@meshulam.tesarici.cz>
        <ZQhwnhOQbDp3j8y7@arm.com>
        <20230922153129.69b26975@meshulam.tesarici.cz>
        <20230922191213.379d440a@meshulam.tesarici.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 22 Sep 2023 19:12:13 +0200
Petr Tesa=C5=99=C3=ADk <petr@tesarici.cz> wrote:

> Hi Catalin,
>=20
> On Fri, 22 Sep 2023 15:31:29 +0200
> Petr Tesa=C5=99=C3=ADk <petr@tesarici.cz> wrote:
>=20
> >[...]=20
> > On Mon, 18 Sep 2023 16:45:34 +0100
> > Catalin Marinas <catalin.marinas@arm.com> wrote:
> >  =20
> > > On Sun, Sep 17, 2023 at 11:47:41AM +0200, Petr Tesa=C5=99=C3=ADk wrot=
e:   =20
> >[...] =20
> > > > Ah... You may have a point after all if this sequence of events is
> > > > possible:
> > > >=20
> > > > - CPU 0 writes new value to mem->pools->next in swiotlb_dyn_alloc().
> > > >=20
> > > > - CPU 1 observes the new value in swiotlb_find_slots(), even though=
 it
> > > >   is not guaranteed by any barrier, allocates a slot and sets the
> > > >   dev->dma_uses_io_tlb flag.
> > > >=20
> > > > - CPU 1 (driver code) writes the returned buffer address into its
> > > >   private struct. This write is ordered after dev->dma_uses_io_tlb
> > > >   thanks to the smp_wmb() in swiotlb_find_slots().
> > > >=20
> > > > - CPU 2 (driver code) reads the buffer address, and DMA core passes=
 it
> > > >   to is_swiotlb_buffer(), which contains smp_rmb().
> > > >=20
> > > > - IIUC CPU 2 is guaranteed to observe the new value of
> > > >   dev->dma_uses_io_tlb, but it may still use the old value of
> > > >   mem->pools->next, because the write on CPU 0 was not ordered
> > > >   against anything. The fact that the new value was observed by CPU=
 1
> > > >   does not mean that it is also observed by CPU 2.     =20
> > >=20
> > > Yes, that's possible. On CPU 1 there is a control dependency between =
the
> > > read of mem->pools->next and the write of dev->dma_uses_io_tlb but I
> > > don't think this is sufficient to claim multi-copy atomicity (if CPU 1
> > > sees mem->pools->next write by CPU 0, CPU 2 must see it as well), at
> > > least not on all architectures supported by Linux. memory-barriers.txt
> > > says that a full barrier on CPU 1 is needed between the read and writ=
e,
> > > i.e. smp_mb() before WRITE_ONCE(dev->dma_uses_io_tlb). You could add =
it
> > > just before "goto found" in swiotlb_find_slots() since it's only need=
ed
> > > on this path.   =20
> >=20
> > Let me check my understanding. This smp_mb() is not needed to make sure
> > that the write to dev->dma_uses_io_tlb cannot be visible before the
> > read of mem->pools->next. Since stores are not speculated, that
> > ordering is provided by the control dependency alone.
> >=20
> > But a general barrier ensures that a third CPU will observe the write to
> > mem->pools->next after the read of mem->pools->next. Makes sense. =20
>=20
> Now that I'm writing the patch, I get your idea to replace WRITE_ONCE()
> with smp_store_release(). Since a full memory barrier is required for
> multicopy atomicity, it is not "more than I need". Instead, the
> ordering contraints may be possibly restricted to "CPUs participating
> in a release-acquire chain" if I also replace READ_ONCE() in
> is_swiotlb_buffer() with smp_read_acquire().
>=20
> I believe it does not matter that the CPU which writes a new value to
> mem->pools->next in swiotlb_dyn_alloc() does not participate in the
> chain, because the value must have been visible to the CPU which
> executes swiotlb_find_slots() and which does participate in the chain.
>=20
> Let me double-check this thinking with a litmus test.

Hm. I didn't have much luck with smp_store_release(), because I need
to ensure ordering of the SUBSEQUENT store (by a device driver).

However, inserting smp_mb() _after_ WRITE_ONCE(dev->dma_uses_io_tlb)
seems to be enough to ensure proper ordering. I could even remove the
write memory barrier in swiotlb_dyn_alloc().

This is my first time using herd7, so I can only hope I got everything
right. FWIW this is my litmus test:

C swiotlb-new-pool

(*
 * Result: Never
 *
 * Check that a newly allocated pool is always visible when the correspondi=
ng
 * swiotlb buffer is visible.
 *)

{}

P0(int *pool)
{
	/* swiotlb_dyn_alloc() */
	WRITE_ONCE(*pool, 999);
}

P1(int *pool, int *flag, int *buf)
{
	/* swiotlb_find_slots() */
	int r0 =3D READ_ONCE(*pool);
	if (r0) {
		WRITE_ONCE(*flag, 1);
		smp_mb();
	}

	/* device driver (presumed) */
	WRITE_ONCE(*buf, r0);
}

P2(int *pool, int *flag, int *buf)
{
	/* device driver (presumed) */
	int r1 =3D READ_ONCE(*buf);

	/* is_swiotlb_buffer() */
	int r2;
	int r3;

	smp_rmb();
	r2 =3D READ_ONCE(*flag);
	if (r2) {
		r3 =3D READ_ONCE(*pool);
	}
}

exists (2:r1<>0 /\ 2:r3=3D0) (* Not flagged or pool not found. *)

Petr T