Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7b55040f-f2d2-372e-cf8a-5ac4a456fdb1@amd.com>
Date:   Tue, 4 Jul 2023 16:51:10 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] drm/amdgpu: avoid integer overflow warning in
 amdgpu_device_resize_fb_bar()
Content-Language: en-US
To:     Arnd Bergmann <arnd@arndb.de>, Arnd Bergmann <arnd@kernel.org>,
        Alex Deucher <alexander.deucher@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        Dave Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>
Cc:     Hawking Zhang <Hawking.Zhang@amd.com>,
        Lijo Lazar <lijo.lazar@amd.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        YiPeng Chai <YiPeng.Chai@amd.com>, Le Ma <le.ma@amd.com>,
        Bokun Zhang <Bokun.Zhang@amd.com>,
        Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>,
        Shiwu Zhang <shiwu.zhang@amd.com>,
        amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20230703123557.3355657-1-arnd@kernel.org>
 <f4bc3739-3ff1-6fa7-5d7d-890f451315bf@amd.com>
 <788147c6-defd-496a-8174-1571b73d1a71@app.fastmail.com>
 <f1b190cb-3af7-178b-baeb-b59363868779@amd.com>
 <9b03246d-b46e-4b91-968a-e9ffc2fc26db@app.fastmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <9b03246d-b46e-4b91-968a-e9ffc2fc26db@app.fastmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Sit3ZTdDQndaTUF3KzUvRmtDN2plQzZYY2RRbUxYeVZEQ1VsV3dVVUtKWEpj?=
 =?utf-8?B?ZlI5WXdkWHM5UWdjQ3lRczU4Wjd0RWVKL01YRUJ5Q3dQNkZXcHhjVUp6UmtW?=
 =?utf-8?B?ci9IUEpWaWtnbjJaRGs5RGpSYVpyZDhmS3BuVU9UUDlMa2M3TXBaTTdRVmE1?=
 =?utf-8?B?K2hLb1pRU0h4dzJYanhhbWo1bXpVUlZaV05mTGE5MEhtVERnd2RpOHNKTHJM?=
 =?utf-8?B?TFE5YnJEaXovSU9qMzV3OFErZ2hkazdUNkQ4by9FZ1JoY0xNaEtTbWVhZmtn?=
 =?utf-8?B?NGc2K081Y3BBUWQweTVaODZ0bVRWSTNyRXpkQW5MMmJjc2xNd3ZMM1JMc0d4?=
 =?utf-8?B?UlhiUDZrOVN3V2Z1dzNkeW5YQjZCME9LSnpia2x4MEszMWVick16M3lFUk9j?=
 =?utf-8?B?NlZrek83R2ZXNlRrL00xelRBRU9WTi9udlo1Um1rQ2xud3hDU2NuODFEcG1s?=
 =?utf-8?B?aGtBcU0vQk5kK3c2QkE3cFBIWXNtRU10bytTSm9Od3ZseUNwcm1uK2dTYjNK?=
 =?utf-8?B?ejRGV242MDFrY1pJS3h2dnVLZlcrdlZvS2trMkNQK3ljL3Ezd0xIMTFTNjA0?=
 =?utf-8?B?c1djWUVyT2E1TE5SNVFDQ0pBMzZpQjVFTlNvZEhGbHlSWk9rdWlxRDhBNkdI?=
 =?utf-8?B?NmI4bnJsNFphMjF5UG1QbE5IVlFuS0RTRUFFbTM3RE9aREdtQ0ZZUzFsdUZB?=
 =?utf-8?B?K0NXaU5YV1hhUWo0SFhaTGhHRHFpamwzbFRPSldlbnB1ZnRJODdoSU5BMHhV?=
 =?utf-8?B?RWEvbVNKWHgwL0dmelpMZmt4UldHQjBEeHpMWkh1QnNwRmJLVGg4bEorTE5i?=
 =?utf-8?B?L3JZOEJzSWdYMEtuY05sUURjbXl6Z0Z2VGlCWVRabmZKbDZDYXJIUXA3azA5?=
 =?utf-8?B?cmtjOWx6L1QrTGxJQmtnMXVWejB2aGx6bnYzcVloMmJtTlh5L0xScHhHdERP?=
 =?utf-8?B?R3ZJRzVLTXg3V055dStrOXBSeTZtcnRadjd0cVNFaWtOZjQvcUh3VklXeWlD?=
 =?utf-8?B?d2ZTUUxuajNHVzRNdFU0d1pTUDdrcWtxWFNRUGpkRy8vRFJwdWxMVkwwNzBR?=
 =?utf-8?B?dU5VK0Y0M0R2Tmhnc1Jkb3ZhdDBpck10eHZhQWJuNWJ4bFlFZzdZbFExWk5z?=
 =?utf-8?B?NzRyb0JoU3ZXZGlvNzBCRWhGWG00RmE5b0hUNG1iRGFJMlRac2ZSbWJuYkxp?=
 =?utf-8?B?SzJ2MzhuTHNCZFpIMjRzV1Z2cE92aGY4aE1hMVNBK3lFdzNkN3NobEdQTDNE?=
 =?utf-8?B?T285OWFBTlN2bUVwSTd1enh0K2EwMzlNb2MweS91WUpYelBCdVIzMGh5Z1Jq?=
 =?utf-8?B?SVBqVElDeWlVdnJzQi9XSmgvbE1MN0tHL3JSZFVZNC9peC9SaTVERzMwVXVh?=
 =?utf-8?B?UUVCZWQ0SUdlVlYzaTM3aHlQRE96Qjc1RlNLc0Y1bmptUUdBOHltZTRCaDgy?=
 =?utf-8?B?K3dhWXlVaERrTGsycUsvT05xZTFHMXplWVY0TWhXVjNyNG9rQittWjBPTEZ6?=
 =?utf-8?B?UEEzcGpFR3VLaDNVY1ppR0xsZEJSTVEyMVpRTHRzaDR0cVZ6aEZseG51cXU0?=
 =?utf-8?B?OVoxRjI1dkhVYjEvbmVZMzdONXliSXVlUnViVTVFY3FuN1lnNWNicktoOHQv?=
 =?utf-8?B?UEhXZGl5ZjQyNDlscVZ6R1lOL1M1TWNxaVkrK3RONFlVbjhpelVtMCtKZCtF?=
 =?utf-8?B?dm81SDJTMm5HSXNoTUcxdkNwc3FsVTNXQ2lsL0RFVktDRG1FKzFkTUxGaTRa?=
 =?utf-8?B?NVM3anFCMXFZbUowbkdmQS9vaEdPNjBlT2tCSTRSWVNQUHdBY1Y4enM5WjNs?=
 =?utf-8?B?RGQ1M1NSaEE5dnZ3eWxxbnNMbDBuTFpyTklmTVJxV3AxZVNzWEQzWkp0OXRh?=
 =?utf-8?B?aEM2SEMvRVozcUMrSlNCQTlMZmpHcENCVWRvcWV4Y0ZHREFobndXQnFYaFd2?=
 =?utf-8?B?UUZEbm9lNGpaejY0NU84T08rZHYraXd0SGFwaWhINkhlUXlsQ0JFN2FwSmdx?=
 =?utf-8?B?eXZMMjQ1QnFvTzdnN3VqbnZtVjJta0FrZVdYM1F6a1F6QVp6T2xNa1lmZHFJ?=
 =?utf-8?B?MUs0Z1dMQ1VtMEgrTU91Ly8rWFg1MFRPMXpTTnVhenQ3N2hOUGJodDZia0Q5?=
 =?utf-8?B?ZkxhZ01PTnAyak10ZGJEYWRicU1NZHJuM2tRK3pycDEvM0lOV0VyQ21jdTVl?=
 =?utf-8?Q?umFMpI3ZykCk9K7WzxYMMbbzzrvpHNNtfqVsU9BJVmYz?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: de471d65-046e-42ed-8603-08db7c9e1ee4
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jul 2023 14:51:16.9325
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PpaPvPofPZLFVLf/0LP9BNpD/QVBj2Vx6wDT5MbbWF3VtkJctbVv76Zfm0Y3yUD0
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR12MB8022
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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