Hi Chong,
On 7/8/26 12:25, Li, Chong(Alan) wrote:
> AMD General
>
> Hi, Christian.
>
> The code " early_full_gpu_access = (adev->virt.req_init_data_ver == 0);" is
> related to the ASIC type, not the host platform:
>
>
> adev->virt.req_init_data_ver is initialized by
> amdgpu_virt_request_init_data().
>
> Some legacy ASICs, such as CHIP_VEGA20 and CHIP_ALDEBARAN, do not send
> amdgpu_virt_request_init_data() to the host.
>
> For those ASICs, the host dumps the early init data only after the guest
> requests full GPU access.
> Therefore, those ASICs still need to request full GPU access before the guest
> driver can read the early init data from the VF FB.
No, exactly that is not the case as far as I can see. See the commit I
mentioned below.
According to that starting from SIENNA_CICHLID the full GPU access is mandatory
for reading the discovery table.
So I absolutely clearly have to reject your patch here.
As far as I can see this is a hack for ESXi and that is not going to fly.
Regards,
Christian.
>
>
>
> static bool amdgpu_virt_init_req_data(struct amdgpu_device *adev, u32
> reg)
> {
> ...
> case CHIP_VEGA20:
> case CHIP_ARCTURUS:
> case CHIP_ALDEBARAN:
> soc15_set_virt_ops(adev);
> break;
> case CHIP_NAVI10:
> case CHIP_NAVI12:
> case CHIP_SIENNA_CICHLID:
> case CHIP_IP_DISCOVERY:
> nv_set_virt_ops(adev);
> /* try send GPU_INIT_DATA request to host */
> amdgpu_virt_request_init_data(adev);
> break;
> ...
> }
>
> Thanks,
> Chong.
>
> -----Original Message-----
> From: Koenig, Christian <[email protected]>
> Sent: Wednesday, July 8, 2026 6:24 PM
> To: Lazar, Lijo <[email protected]>; Li, Chong(Alan) <[email protected]>;
> [email protected]
> Cc: Deng, Emily <[email protected]>; Chang, HaiJun <[email protected]>;
> Skvortsov, Victor <[email protected]>; Cursor <[email protected]>
> Subject: Re: [PATCH] drm/amdgpu: improve the amdgpu device init progress in
> sriov mode
>
> On 7/8/26 12:03, Lazar, Lijo wrote:
>>
>>
>> On 08-Jul-26 3:23 PM, Christian König wrote:
>>> On 7/8/26 11:35, Li, Chong(Alan) wrote:
>>>> AMD General
>>>>
>>>> Hi, Christian.
>>>>
>>>> This version of the patch is not host-platform dependent; all host
>>>> platforms follow the same path.
>>>>
>>>> I verified this patch with KVM in my local environment and with ESXi on a
>>>> borrowed server.
>>>>
>>>> Distinguishing the host platform was a mistake, and I have explained
>>>> the situation at the end of the patch email.
>>>>
>>>> I have pasted the content below:
>>>>
>>>> Hi, Christian and Lijo.
>>>>
>>>> Sorry, I made a mistake.
>>>> Host access to the VF FB is not platform-dependent. After the
>>>> host driver sets VF_FB_EN in response to GPU_INIT_DATA, the host can
>>>> access the VF FB.
>>>>
>>>> I borrowed an ESXi server and tested this patch there.
>>>> The amdgpu driver works normally.
>>>>
>>>> I rewrote the background. Is anything still unclear?
>>>
>>> You still have this check here in the code: "early_full_gpu_access =
>>> (adev->virt.req_init_data_ver == 0);".
>>>
>>> As far as I can see that is Hypervisor specific and a NO-GO.
>>>
>>
>> I think this should be seen as a policy set by host driver on when a guest
>> operation is allowed. Host driver could do it based on other conditions
>> also, not specific to hypervisor environment.
>
> No, this was intentionally changed in 2020 because of a new feature. See this
> commit here:
>
> commit 00a979f3d69e0c275e88c741b854dbe0d5238ae0
> Author: Wenhui Sheng <[email protected]>
> Date: Tue Jun 23 13:43:49 2020 +0800
>
> drm/amdgpu: invoke req full access early enough
>
> From SIENNA_CICHLID, HW introduce a new protection
> feature which can control the FB, doorbell and MMIO
> write access for VF, so guest driver should request
> full access before ip discovery, or we couldn't access
> ip discovery data in FB.
>
> Signed-off-by: Wenhui Sheng <[email protected]>
> Reviewed-by: Hawking Zhang <[email protected]>
> Signed-off-by: Alex Deucher <[email protected]>
>
> So as far as I can see this change here will break older hypervisor versions
> and that is absolutely clear reason to NAK it.
>
> Regards,
> Christian.
>
>>
>> Thanks,
>> Lijo
>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Thanks,
>>>> Chong.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Koenig, Christian <[email protected]>
>>>> Sent: Wednesday, July 8, 2026 5:05 PM
>>>> To: Li, Chong(Alan) <[email protected]>;
>>>> [email protected]
>>>> Cc: Deng, Emily <[email protected]>; Chang, HaiJun
>>>> <[email protected]>; Skvortsov, Victor
>>>> <[email protected]>; Lazar, Lijo <[email protected]>; Cursor
>>>> <[email protected]>
>>>> Subject: Re: [PATCH] drm/amdgpu: improve the amdgpu device init
>>>> progress in sriov mode
>>>>
>>>> On 7/8/26 10:25, chong li wrote:
>>>>> v2:
>>>>> Some legacy ASICs do not send amdgpu_virt_request_init_data().
>>>>> Only keep the full GPU access request early when request_init_data is not
>>>>> sent.
>>>>>
>>>>> v1:
>>>>> Move the initialization of non-GPU resources out of the full GPU
>>>>> access region during AMDGPU device initialization.
>>>>>
>>>>> Background:
>>>>> In SR-IOV, the guest sends GPU_INIT_DATA, then the host enables
>>>>> VF_FB_EN and places early initialization data, such as IP
>>>>> discovery, VBIOS, and PF-VF exchange data, in the VF FB. The guest
>>>>> should then be able to read this data before requesting full GPU access.
>>>>>
>>>>> Before this patch, the VF still requested full GPU access in
>>>>> amdgpu_device_ip_early_init(). At that point TTM is not initialized
>>>>> yet, so the normal VRAM aperture mapping is unavailable and the
>>>>> guest falls back to MM_INDEX/MM_DATA register access. That register
>>>>> path requires full GPU access.
>>>>>
>>>>> Use the BAR0 framebuffer read path,
>>>>> amdgpu_device_read_fb_via_bar0(), for the early init-data copy
>>>>> instead of MM_INDEX/MM_DATA. This lets the driver delay the full
>>>>> GPU access request until after the early init data has been copied.
>>>>
>>>> That looks like it goes into the right direction, but as far as I can see
>>>> it is still an ESXi specific change.
>>>>
>>>> So once more: We can't adjust the driver to the hypervisor!
>>>>
>>>> Either this works on all hypervisors or I have to reject the change.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Signed-off-by: chong li <[email protected]>
>>>>> Co-authored-by: Cursor <[email protected]>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 21
>>>>> +++++++++++++++++----
>>>>> drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 4 ++--
>>>>> 2 files changed, 19 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> index 610d82b79de3..ac66796e8634 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> @@ -38,6 +38,10 @@
>>>>> #include <linux/apple-gmux.h>
>>>>> #include <linux/nospec.h>
>>>>>
>>>>> +#ifdef CONFIG_X86
>>>>> +#include <asm/hypervisor.h>
>>>>> +#endif
>>>>> +
>>>>> #include <drm/drm_atomic_helper.h>
>>>>> #include <drm/drm_client_event.h>
>>>>> #include <drm/drm_crtc_helper.h>
>>>>> @@ -1987,16 +1991,19 @@ static int
>>>>> amdgpu_device_ip_early_init(struct
>>>>> amdgpu_device *adev) {
>>>>> struct amdgpu_ip_block *ip_block;
>>>>> struct pci_dev *parent;
>>>>> - bool total, skip_bios;
>>>>> + bool total, skip_bios, early_full_gpu_access = false;
>>>>> uint32_t bios_flags;
>>>>> int i, r;
>>>>>
>>>>> amdgpu_device_enable_virtual_display(adev);
>>>>>
>>>>> if (amdgpu_sriov_vf(adev)) {
>>>>> - r = amdgpu_virt_request_full_gpu(adev, true);
>>>>> - if (r)
>>>>> - return r;
>>>>> + early_full_gpu_access = (adev->virt.req_init_data_ver
>>>>> +== 0);
>>>>> + if (early_full_gpu_access) {
>>>>> + r = amdgpu_virt_request_full_gpu(adev, true);
>>>>> + if (r)
>>>>> + return r;
>>>>> + }
>>>>>
>>>>> r = amdgpu_virt_init_critical_region(adev);
>>>>> if (r)
>>>>> @@ -2159,6 +2166,12 @@ static int
>>>>> amdgpu_device_ip_early_init(struct amdgpu_device *adev)
>>>>> if (!total)
>>>>> return -ENODEV;
>>>>>
>>>>> + if (amdgpu_sriov_vf(adev) && !early_full_gpu_access) {
>>>>> + r = amdgpu_virt_request_full_gpu(adev, true);
>>>>> + if (r)
>>>>> + return r;
>>>>> + }
>>>>> +
>>>>> if (adev->gmc.xgmi.supported)
>>>>> amdgpu_xgmi_early_init(adev);
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
>>>>> index 9a40107a0869..340703d89d6b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
>>>>> @@ -185,8 +185,8 @@ static int xgpu_ai_send_access_requests(struct
>>>>> amdgpu_device *adev,
>>>>> } else if (req == IDH_REQ_GPU_INIT_DATA){
>>>>> /* Dummy REQ_GPU_INIT_DATA handling */
>>>>> r = xgpu_ai_poll_msg(adev,
>>>>> IDH_REQ_GPU_INIT_DATA_READY);
>>>>> - /* version set to 0 since dummy */
>>>>> - adev->virt.req_init_data_ver = 0;
>>>>> + /* Version is set to 1 since GPU_CRIT_REGION_V1 */
>>>>> + adev->virt.req_init_data_ver = GPU_CRIT_REGION_V1;
>>>>> }
>>>>>
>>>>> return 0;
>>>>
>>>
>>
>
