Hi all, I've discussed the issue with Alex today.
The current approach of keeping full GPU access while discovery has been intentionally added this way over 6 years ago after some back and forth discussion. So that has been working reliable for quite a while and we don't see any justification to change that. If the ESXi hypervisor has a problem with this then that needs to be handled on the hypervisor side. This patch is rejected. Regards, Christian. On 7/9/26 12:35, Li, Chong(Alan) wrote: > AMD General > > Hi, Lijo. > > Yes, the current GIM driver ensures the early init data is placed within the > VF’s visible BAR0 aperture. > > > Thanks, > Chong. > > -----Original Message----- > From: Lazar, Lijo <[email protected]> > Sent: Thursday, July 9, 2026 3:56 PM > To: Li, Chong(Alan) <[email protected]>; Koenig, Christian > <[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 08-Jul-26 6:12 PM, Li, Chong(Alan) wrote: >> AMD General >> >> Hi, Lijo. >> >> " In this commit, the statement is different." >> " It says FB access is enabled if init_data request is sent." >> >> Yes, VF FB access is enabled if init_data request is sent. >> >> However, without full GPU access, the guest cannot access the >> MM_INDEX/MM_DATA registers. >> >> With the legacy/existing read path, VF_FB_EN must be enabled so the >> host can dump early init data into the VF FB, and full GPU access is >> required so the guest can copy that data through the MM_INDEX/MM_DATA >> registers. >> >> I submitted patch " [PATCH 1/2] drm/amdgpu: read FB through BAR0 when >> aperture is unavailable " , With the new >> amdgpu_device_read_fb_via_bar0() path, the guest can read the VF FB >> without full GPU access >> > > Yes, I saw that one. One other question - does host driver guarantee that > discovery data will always be copied to visible BAR aperture of VF? > If not, it will still require mmindex/data regardless of FB access enablement. > > Thanks, > Lijo > >> >> Thanks, >> Chong. >> >> >> -----Original Message----- >> From: Lazar, Lijo <[email protected]> >> Sent: Wednesday, July 8, 2026 7:18 PM >> To: Koenig, Christian <[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 08-Jul-26 4:18 PM, Christian König wrote: >>> 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. >>> >> >> It could also be because the legacy/existing way used mmindex/data, and that >> required full access including mmio register access. >> >> In this commit, the statement is different. >> >> "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" >> >> It says FB access is enabled if init_data request is sent. >> >> Thanks >> Lijo >> >>> 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; >>>>>>> >>>>>> >>>>> >>>> >>> >> >
