linux-kernel - Re: [PATCH] drm/amdgpu: Rearrange WREG32 operations in gfxhub_v2

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] [day] [month] [year] [list]

Message-ID: <2404b7f3-a465-a706-8af3-459105f0dc54@amd.com>
Date:   Wed, 31 May 2023 15:33:16 +0200
From:   Christian König <christian.koenig@....com>
To:     ghostfly233 <ghostfly23333@...il.com>, alexander.deucher@....com,
        Xinhui.Pan@....com
Cc:     amd-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        airlied@...il.com, daniel@...ll.ch
Subject: Re: [PATCH] drm/amdgpu: Rearrange WREG32 operations in gfxhub_v2_1.c

Am 20.05.23 um 13:59 schrieb ghostfly233:
> In gfxhub_v2_1_setup_vmid_config(), the GCVM_CONTEXT1_CNTL reg is
> written before related GCVM_CONTEXT1_PAGE_TABLE_START_ADDR and
> GCVM_CONTEXT1_PAGE_TABLE_END_ADDR regs are written, which may
> cause undefined behavior.

In which hw documentation did you found that?

>
> This patch rearranges WREG32 operations in gfxhub_v2_1_setup_vmid_config(),
> so that it can ensure the addresses are initialized before CNTL is enabled
> and reduce the risk of encountering undefined behavior.

I have absolutely no idea how you came to this conclusion, but as far as 
I know this is just bluntly incorrect.

The control register must be written before the other parameters are set 
or otherwise the hw wouldn't know what to do with those values and might 
even block writes to the registers or hang.

Regards,
Christian.

>
> Signed-off-by: Zibin Liu <ghostfly23333@...il.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> index 4aacbbec31e2..6d094e7315eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> @@ -321,8 +321,6 @@ static void gfxhub_v2_1_setup_vmid_config(struct amdgpu_device *adev)
>   		tmp = REG_SET_FIELD(tmp, GCVM_CONTEXT1_CNTL,
>   				    RETRY_PERMISSION_OR_INVALID_PAGE_FAULT,
>   				    !adev->gmc.noretry);
> -		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_CNTL,
> -				    i * hub->ctx_distance, tmp);
>   		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32,
>   				    i * hub->ctx_addr_distance, 0);
>   		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32,
> @@ -333,6 +331,8 @@ static void gfxhub_v2_1_setup_vmid_config(struct amdgpu_device *adev)
>   		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_END_ADDR_HI32,
>   				    i * hub->ctx_addr_distance,
>   				    upper_32_bits(adev->vm_manager.max_pfn - 1));
> +		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_CNTL,
> +				    i * hub->ctx_distance, tmp);
>   	}
>   
>   	hub->vm_cntx_cntl = tmp;