| 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
| ||
|
Date: Mon, 17 Jun 2024 21:43:08 +0800
From: Icenowy Zheng <uwu@...nowy.me>
To: Christian König <christian.koenig@....com>, Alex
Deucher <alexander.deucher@....com>, Pan Xinhui <Xinhui.Pan@....com>, David
Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, Pierre-Eric
Pelloux-Prayer <pierre-eric.pelloux-prayer@....com>, Huacai Chen
<chenhuacai@...nel.org>, WANG Xuerui <kernel@...0n.name>
Cc: amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev
Subject: Re: [PATCH 1/2] drm/amdgpu: make duplicated EOP packet for GFX7/8
have real content
在 2024-06-17星期一的 15:09 +0200，Christian König写道：
> Am 17.06.24 um 15:03 schrieb Icenowy Zheng:
> > 在 2024-06-17星期一的 14:35 +0200，Christian König写道：
> > > Am 17.06.24 um 12:58 schrieb Icenowy Zheng:
> > > > The duplication of EOP packets for GFX7/8, with the former one
> > > > have
> > > > seq-1 written and the latter one have seq written, seems to
> > > > confuse
> > > > some
> > > > hardware platform (e.g. Loongson 7A series PCIe controllers).
> > > >
> > > > Make the content of the duplicated EOP packet the same with the
> > > > real
> > > > one, only masking any possible interrupts.
> > > Well completely NAK to that, exactly that disables the
> > > workaround.
> > >
> > > The CPU needs to see two different values written here.
> > Why do the CPU need to see two different values here? Only the
> > second
> > packet will raise an interrupt before and after applying this
> > patch,
> > and the first packet's result should just be overriden on ordinary
> > platforms. The CPU won't see the first one, until it's polling for
> > the
> > address for a very short interval, so short that the GPU CP
> > couldn't
> > execute 2 commands.
>
> Yes exactly that. We need to make two writes, one with the old value
> (seq - 1) and a second with the real value (seq).
>
> Otherwise it is possible that a polling CPU would see the sequence
> before the second EOP is issued with results in incoherent view of
> memory.
In this case shouldn't we write seq-1 before any work, and then write
seq after work, like what is done in Mesa?
As what I see, Mesa uses another command buffer to emit a
EVENT_WRITE_EOP writing 0, and commit this command buffer before the
real command buffer.
>
> > Or do you mean the GPU needs to see two different values being
> > written,
> > or they will be merged into only one write request?
> >
> > Please give out more information about this workaround, otherwise
> > the
> > GPU hang problem on Loongson platforms will persist.
>
> Well if Loongson can't handle two consecutive write operations to the
> same address with different values then you have a massive platform
> bug.
I think the issue is triggered when two consecutive write operations
and one IRQ is present, which is exactly the case of this function.
>
> That is something which can happen all the time throughout the
> operation.
>
> Regards,
> Christian.
>
> >
> > > Regards,
> > > Christian.
> > >
> > > > Fixes: bf26da927a1c ("drm/amdgpu: add cache flush workaround to
> > > > gfx8 emit_fence")
> > > > Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts")
> > > > Signed-off-by: Icenowy Zheng <uwu@...nowy.me>
> > > > ---
> > > > drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 12 +++++-------
> > > > drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 12 ++++--------
> > > > 2 files changed, 9 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > > index 541dbd70d8c75..778f27f1a34fe 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > > @@ -2117,9 +2117,8 @@ static void
> > > > gfx_v7_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64
> > > > addr,
> > > > {
> > > > bool write64bit = flags & AMDGPU_FENCE_FLAG_64BIT;
> > > > bool int_sel = flags & AMDGPU_FENCE_FLAG_INT;
> > > > - /* Workaround for cache flush problems. First send a
> > > > dummy
> > > > EOP
> > > > - * event down the pipe with seq one below.
> > > > - */
> > > > +
> > > > + /* Workaround for cache flush problems, send EOP twice.
> > > > */
> > > > amdgpu_ring_write(ring,
> > > > PACKET3(PACKET3_EVENT_WRITE_EOP,
> > > > 4));
> > > > amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN |
> > > > EOP_TC_ACTION_EN |
> > > > @@ -2127,11 +2126,10 @@ static void
> > > > gfx_v7_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64
> > > > addr,
> > > > EVENT_INDEX(5)));
> > > > amdgpu_ring_write(ring, addr & 0xfffffffc);
> > > > amdgpu_ring_write(ring, (upper_32_bits(addr) & 0xffff)
> > > > |
> > > > - DATA_SEL(1) | INT_SEL(0));
> > > > - amdgpu_ring_write(ring, lower_32_bits(seq - 1));
> > > > - amdgpu_ring_write(ring, upper_32_bits(seq - 1));
> > > > + DATA_SEL(write64bit ? 2 : 1) |
> > > > INT_SEL(0));
> > > > + amdgpu_ring_write(ring, lower_32_bits(seq));
> > > > + amdgpu_ring_write(ring, upper_32_bits(seq));
> > > >
> > > > - /* Then send the real EOP event down the pipe. */
> > > > amdgpu_ring_write(ring,
> > > > PACKET3(PACKET3_EVENT_WRITE_EOP,
> > > > 4));
> > > > amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN |
> > > > EOP_TC_ACTION_EN |
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > > > b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > > > index 2f0e72caee1af..39a7d60f1fd69 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > > > @@ -6153,9 +6153,7 @@ static void
> > > > gfx_v8_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64
> > > > addr,
> > > > bool write64bit = flags & AMDGPU_FENCE_FLAG_64BIT;
> > > > bool int_sel = flags & AMDGPU_FENCE_FLAG_INT;
> > > >
> > > > - /* Workaround for cache flush problems. First send a
> > > > dummy
> > > > EOP
> > > > - * event down the pipe with seq one below.
> > > > - */
> > > > + /* Workaround for cache flush problems, send EOP twice.
> > > > */
> > > > amdgpu_ring_write(ring,
> > > > PACKET3(PACKET3_EVENT_WRITE_EOP,
> > > > 4));
> > > > amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN |
> > > > EOP_TC_ACTION_EN |
> > > > @@ -6164,12 +6162,10 @@ static void
> > > > gfx_v8_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64
> > > > addr,
> > > > EVENT_INDEX(5)));
> > > > amdgpu_ring_write(ring, addr & 0xfffffffc);
> > > > amdgpu_ring_write(ring, (upper_32_bits(addr) & 0xffff)
> > > > |
> > > > - DATA_SEL(1) | INT_SEL(0));
> > > > - amdgpu_ring_write(ring, lower_32_bits(seq - 1));
> > > > - amdgpu_ring_write(ring, upper_32_bits(seq - 1));
> > > > + DATA_SEL(write64bit ? 2 : 1) |
> > > > INT_SEL(0));
> > > > + amdgpu_ring_write(ring, lower_32_bits(seq));
> > > > + amdgpu_ring_write(ring, upper_32_bits(seq));
> > > >
> > > > - /* Then send the real EOP event down the pipe:
> > > > - * EVENT_WRITE_EOP - flush caches, send int */
> > > > amdgpu_ring_write(ring,
> > > > PACKET3(PACKET3_EVENT_WRITE_EOP,
> > > > 4));
> > > > amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN |
> > > > EOP_TC_ACTION_EN |
>
Powered by blists - more mailing lists