linux-kernel - Re: [PATCH] drm/amdgpu: avoid integer overflow warning in amdgpu_device_resize_fb

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: <7b55040f-f2d2-372e-cf8a-5ac4a456fdb1@amd.com>
Date:   Tue, 4 Jul 2023 16:51:10 +0200
From:   Christian König <christian.koenig@....com>
To:     Arnd Bergmann <arnd@...db.de>, Arnd Bergmann <arnd@...nel.org>,
        Alex Deucher <alexander.deucher@....com>,
        "Pan, Xinhui" <Xinhui.Pan@....com>,
        Dave Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     Hawking Zhang <Hawking.Zhang@....com>,
        Lijo Lazar <lijo.lazar@....com>,
        Mario Limonciello <mario.limonciello@....com>,
        YiPeng Chai <YiPeng.Chai@....com>, Le Ma <le.ma@....com>,
        Bokun Zhang <Bokun.Zhang@....com>,
        Srinivasan Shanmugam <srinivasan.shanmugam@....com>,
        Shiwu Zhang <shiwu.zhang@....com>,
        amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/amdgpu: avoid integer overflow warning in
 amdgpu_device_resize_fb_bar()

Am 04.07.23 um 16:31 schrieb Arnd Bergmann:
> On Tue, Jul 4, 2023, at 14:33, Christian König wrote:
>> Am 04.07.23 um 14:24 schrieb Arnd Bergmann:
>>> On Tue, Jul 4, 2023, at 08:54, Christian König wrote:
>>>> Am 03.07.23 um 14:35 schrieb Arnd Bergmann:
>>> Not sure I understand the specific requirement. Do you mean the entire
>>> amdgpu driver requires 64-bit BAR addressing, or just the bits that
>>> resize the BARs?
>> Well a bit of both.
>>
>> Modern AMD GPUs have 16GiB of local memory (VRAM), making those
>> accessible to a CPU which can only handle 32bit addresses by resizing
>> the BAR is impossible to begin with.
>>
>> But going a step further even without resizing it is pretty hard to get
>> that hardware working on such an architecture.
> I'd still like to understand this part better, as we have a lot of
> arm64 chips with somewhat flawed PCIe implementations, often with
> a tiny 64-bit memory space, but otherwise probably capable of
> using a GPU.

Yeah, those are unfortunately very well known to us :(

> What exactly do you expect to happen here?
>
> a) Use only part of the VRAM but otherwise work as expected
> b) Access all of the VRAM, but at a performance cost for
>     bank switching?

We have tons of x86 systems where we can't resize the BAR (because of 
lack of BIOS setup of the root PCIe windows). So bank switching is still 
perfectly supported.

> c) Require kernel changes to make a) or b) work, otherwise
>     fail to load
> d) have no chance of working even with driver changes

Yeah, that is usually what happens on those arm64 system with flawed 
PCIe implementations.

The problem is not even BAR resize, basically we already had tons of 
customers which came to us and complained that amdgpu doesn't load or 
crashes the system after a few seconds.

After investigating (which sometimes even includes involving engineers 
from ARM) we usually find that those boards doesn't even remotely comply 
to the PCIe specification, both regarding power as well as functional 
things like DMA coherency.

Regards,
Christian.

>
>>>> It might be cleaner to just not build the whole driver on such systems
>>>> or at least leave out this function.
>>> How about this version? This also addresses the build failure, but
>>> I don't know if this makes any sense:
>>>
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -1325,6 +1325,9 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
>>>           u16 cmd;
>>>           int r;
>>>    
>>> +       if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
>>> +               return 0;
>>> +
>> Yes, if that suppresses the warning as well then that makes perfect
>> sense to me.
> Ok, I'll send that as a v2 then.
>
>      Arnd