Re: [PATCH v3 07/23] drm/xe: Read VF GMD_ID with a specifically-allocated dummy GT

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Michal Wajdeczko (2025-10-01 07:07:04-03:00)
>
>
>On 10/1/2025 12:56 AM, Matt Roper wrote:
>> SRIOV VF initialization has a bit of a chicken and egg design problem.
>> Determining the IP version of the graphics and media IPs can't be done
>> via direct register reads as it is on PF or native and instead requires
>> querying the GuC.  However initialization of the GT, including its GuC,
>> needs to wait until after we know the IP versions so that the proper
>> initialization steps for the platform/IP are followed.
>> 
>> Currently the (somewhat hacky) solution is to manually fill out just
>> enough fields in tile 0's primary GT structure to make it look as if the
>> GT has been initialized so that the GuC can be partially initialized and
>> queried to obtain the GMD_ID values.  When the GT gets properly
>> initialized during the regular flows, the hacked-up values will get
>> overwritten as part of the general initialization flows.
>> 
>> Rather than using tile 0's primary GT structure to hold the hacked up
>> values for querying every GT on every tile, instead allocate a dedicated
>> dummy structure.  This will allow us to move the tile->primary_gt's
>> allocation to a more consistent place later in the initialization flow
>> in future patches (i.e., we shouldn't even allocate this GT structure if
>> the GT is disabled/unavailable).  It also helps ensure there can't be
>> any accidental leakage of initialization or state between the dummy
>> initialization for GMD_ID and the real driver initialization of the GT.
>> 
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/xe/tests/xe_pci.c |  6 ++-
>>  drivers/gpu/drm/xe/xe_pci.c       | 61 +++++++++++++++++--------------
>>  2 files changed, 38 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/tests/xe_pci.c b/drivers/gpu/drm/xe/tests/xe_pci.c
>> index 0f136bc85b76..969f1dacade8 100644
>> --- a/drivers/gpu/drm/xe/tests/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/tests/xe_pci.c
>> @@ -307,8 +307,8 @@ const void *xe_pci_id_gen_param(const void *prev, char *desc)
>>  }
>>  EXPORT_SYMBOL_IF_KUNIT(xe_pci_id_gen_param);
>>  
>> -static void fake_read_gmdid(struct xe_device *xe, enum xe_gmdid_type type,
>> -                            u32 *ver, u32 *revid)
>> +static int fake_read_gmdid(struct xe_device *xe, enum xe_gmdid_type type,
>> +                           u32 *ver, u32 *revid)
>>  {
>>          struct kunit *test = kunit_get_current_test();
>>          struct xe_pci_fake_data *data = test->priv;
>> @@ -320,6 +320,8 @@ static void fake_read_gmdid(struct xe_device *xe, enum xe_gmdid_type type,
>>                  *ver = data->graphics_verx100;
>>                  *revid = xe_step_to_gmdid(data->step.graphics);
>>          }
>> +
>> +        return 0;
>>  }
>>  
>>  static void fake_xe_info_probe_tile_count(struct xe_device *xe)
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 37ae49f4b648..9fb5df10844d 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -464,7 +464,7 @@ enum xe_gmdid_type {
>>          GMDID_MEDIA
>>  };
>>  
>> -static void read_gmdid(struct xe_device *xe, enum xe_gmdid_type type, u32 *ver, u32 *revid)
>> +static int read_gmdid(struct xe_device *xe, enum xe_gmdid_type type, u32 *ver, u32 *revid)
>>  {
>>          struct xe_mmio *mmio = xe_root_tile_mmio(xe);
>>          struct xe_reg gmdid_reg = GMD_ID;
>> @@ -473,21 +473,19 @@ static void read_gmdid(struct xe_device *xe, enum xe_gmdid_type type, u32 *ver,
>>          KUNIT_STATIC_STUB_REDIRECT(read_gmdid, xe, type, ver, revid);
>>  
>>          if (IS_SRIOV_VF(xe)) {
>> -                struct xe_gt *gt = xe_root_mmio_gt(xe);
>> -
>>                  /*
>>                   * To get the value of the GMDID register, VFs must obtain it
>>                   * from the GuC using MMIO communication.
>>                   *
>> -                 * Note that at this point the xe_gt is not fully uninitialized
>> -                 * and only basic access to MMIO registers is possible. To use
>> -                 * our existing GuC communication functions we must perform at
>> -                 * least basic xe_gt and xe_guc initialization.
>> -                 *
>> -                 * Since to obtain the value of GMDID_MEDIA we need to use the
>> -                 * media GuC, temporarily tweak the gt type.
>> +                 * Note that at this point the GTs are not initialized and only
>> +                 * tile-level access to MMIO registers is possible. To use our
>> +                 * existing GuC communication functions we must create a dummy
>> +                 * GT structure and perform at least basic xe_gt and xe_guc
>> +                 * initialization.
>>                   */
>> -                xe_gt_assert(gt, gt->info.type == XE_GT_TYPE_UNINITIALIZED);
>> +                struct xe_gt *gt = kzalloc(sizeof(*gt), GFP_KERNEL);
>
>nit: we can use __free(kfree) here ...

Taking a step back, do we really need to allocate this? I get the
impression that keeping the dummy gt on the stack should be fine since
we are keeping its lifetime constrained within this function.

--
Gustavo Sousa

>
>> +                if (!gt)
>> +                        return -ENOMEM;
>>  
>>                  if (type == GMDID_MEDIA) {
>>                          gt->info.id = 1;
>
>it looks that it is still crashing in xe_gt_mmio_init() called from below
>
><1> [146.543510] BUG: kernel NULL pointer dereference, address: 0000000000000028
><1> [146.543528] #PF: supervisor read access in kernel mode
><1> [146.543538] #PF: error_code(0x0000) - not-present page
><4> [146.543611] RIP: 0010:xe_gt_mmio_init+0x27/0x1b0 [xe]
><4> [146.544175] Call Trace:
><4> [146.544182]  <TASK>
><4> [146.544196]  read_gmdid+0x12b/0x2a0 [xe]
><4> [146.544601]  xe_info_init+0x50e/0xc90 [xe]
><4> [146.544948]  ? drmm_kmalloc+0x87/0x100
><4> [146.544968]  ? xe_device_probe_early+0xdf/0x230 [xe]
><4> [146.545253]  ? pci_write_config_word+0x27/0x50
><4> [146.545274]  xe_pci_probe+0x163/0x600 [xe]
>
>likely due to NULL gt->tile
>
>> @@ -503,12 +501,7 @@ static void read_gmdid(struct xe_device *xe, enum xe_gmdid_type type, u32 *ver,
>>                  /* Don't bother with GMDID if failed to negotiate the GuC ABI */
>>                  val = xe_gt_sriov_vf_bootstrap(gt) ? 0 : xe_gt_sriov_vf_gmdid(gt);
>
>... so in case of bootstrap error we can easily return actual error,
>instead of returning plain 0.00 version
>
>>  
>> -                /*
>> -                 * Only undo xe_gt.info here, the remaining changes made above
>> -                 * will be overwritten as part of the regular initialization.
>> -                 */
>> -                gt->info.id = 0;
>> -                gt->info.type = XE_GT_TYPE_UNINITIALIZED;
>> +                kfree(gt);
>>          } else {
>>                  /*
>>                   * GMD_ID is a GT register, but at this point in the driver
>> @@ -526,6 +519,8 @@ static void read_gmdid(struct xe_device *xe, enum xe_gmdid_type type, u32 *ver,
>>  
>>          *ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val) * 100 + REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
>>          *revid = REG_FIELD_GET(GMD_ID_REVID, val);
>> +
>> +        return 0;
>>  }
>>  
>>  static const struct xe_ip *find_graphics_ip(unsigned int verx100)
>> @@ -552,18 +547,21 @@ static const struct xe_ip *find_media_ip(unsigned int verx100)
>>   * Read IP version from hardware and select graphics/media IP descriptors
>>   * based on the result.
>>   */
>> -static void handle_gmdid(struct xe_device *xe,
>> -                         const struct xe_ip **graphics_ip,
>> -                         const struct xe_ip **media_ip,
>> -                         u32 *graphics_revid,
>> -                         u32 *media_revid)
>> +static int handle_gmdid(struct xe_device *xe,
>> +                        const struct xe_ip **graphics_ip,
>> +                        const struct xe_ip **media_ip,
>> +                        u32 *graphics_revid,
>> +                        u32 *media_revid)
>>  {
>>          u32 ver;
>> +        int ret;
>>  
>>          *graphics_ip = NULL;
>>          *media_ip = NULL;
>>  
>> -        read_gmdid(xe, GMDID_GRAPHICS, &ver, graphics_revid);
>> +        ret = read_gmdid(xe, GMDID_GRAPHICS, &ver, graphics_revid);
>> +        if (ret)
>> +                return ret;
>>  
>>          *graphics_ip = find_graphics_ip(ver);
>>          if (!*graphics_ip) {
>> @@ -571,16 +569,21 @@ static void handle_gmdid(struct xe_device *xe,
>>                          ver / 100, ver % 100);
>>          }
>>  
>> -        read_gmdid(xe, GMDID_MEDIA, &ver, media_revid);
>> +        ret = read_gmdid(xe, GMDID_MEDIA, &ver, media_revid);
>> +        if (ret)
>> +                return ret;
>> +
>>          /* Media may legitimately be fused off / not present */
>>          if (ver == 0)
>> -                return;
>> +                return 0;
>>  
>>          *media_ip = find_media_ip(ver);
>>          if (!*media_ip) {
>>                  drm_err(&xe->drm, "Hardware reports unknown media version %u.%02u\n",
>>                          ver / 100, ver % 100);
>>          }
>> +
>> +        return 0;
>>  }
>>  
>>  /*
>> @@ -690,6 +693,7 @@ static int xe_info_init(struct xe_device *xe,
>>          const struct xe_media_desc *media_desc;
>>          struct xe_tile *tile;
>>          struct xe_gt *gt;
>> +        int ret;
>>          u8 id;
>>  
>>          /*
>> @@ -705,8 +709,11 @@ static int xe_info_init(struct xe_device *xe,
>>                  xe->info.step = xe_step_pre_gmdid_get(xe);
>>          } else {
>>                  xe_assert(xe, !desc->pre_gmdid_media_ip);
>> -                handle_gmdid(xe, &graphics_ip, &media_ip,
>> -                             &graphics_gmdid_revid, &media_gmdid_revid);
>> +                ret = handle_gmdid(xe, &graphics_ip, &media_ip,
>> +                                   &graphics_gmdid_revid, &media_gmdid_revid);
>> +                if (ret)
>> +                        return ret;
>> +
>>                  xe->info.step = xe_step_gmdid_get(xe,
>>                                                    graphics_gmdid_revid,
>>                                                    media_gmdid_revid);
>