Re: [PATCH v3 02/36] drm/xe/vf: Lock querying GGTT config during driver init

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 9/30/2025 2:42 AM, Lis, Tomasz wrote:
> 
> On 9/29/2025 2:15 PM, Matthew Brost wrote:
>> On Mon, Sep 29, 2025 at 09:42:55AM +0200, Michal Wajdeczko wrote:
>>>
>>> On 9/29/2025 4:55 AM, Matthew Brost wrote:
>>>> From: Tomasz Lis <tomasz.lis@intel.com>
>>>>
>>>> Protect access to GGTT config as this is non-static information.
>>>>
>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/xe/xe_gt_sriov_vf.c       | 96 ++++++++++++++++++-----
>>>>   drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h |  3 +
>>>>   drivers/gpu/drm/xe/xe_sriov_vf.c          |  6 ++
>>>>   3 files changed, 84 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>>>> index 0461d5513487..016c867e5e2b 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>>>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>>>> @@ -440,18 +440,21 @@ static int vf_get_ggtt_info(struct xe_gt *gt)
>>>>         xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>   +    down_write(&config->lock);
>>>> +
>>> still didn't get answer to my earlier question [1]
>>>
>>> [1] https://patchwork.freedesktop.org/patch/676375/?series=154627&rev=2#comment_1240924
>>>
>> Again, this isn't a patch, so I believe Tomasz will need to chime in to
>> provide an answer or conclusion here.
> Now answered.
>>
>>>>       err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_GGTT_START_KEY, &start);
>>>>       if (unlikely(err))
>>>> -        return err;
>>>> +        goto out;
>>>>         err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_GGTT_SIZE_KEY, &size);
>>>>       if (unlikely(err))
>>>> -        return err;
>>>> +        goto out;
>>>>         if (config->ggtt_size && config->ggtt_size != size) {
>>>>           xe_gt_sriov_err(gt, "Unexpected GGTT reassignment: %lluK != %lluK\n",
>>>>                   size / SZ_1K, config->ggtt_size / SZ_1K);
>>>> -        return -EREMCHG;
>>>> +        err = -EREMCHG;
>>>> +        goto out;
>>>>       }
>>>>         xe_gt_sriov_dbg_verbose(gt, "GGTT %#llx-%#llx = %lluK\n",
>>>> @@ -460,8 +463,11 @@ static int vf_get_ggtt_info(struct xe_gt *gt)
>>>>       config->ggtt_shift = start - (s64)config->ggtt_base;
>>>>       config->ggtt_base = start;
>>>>       config->ggtt_size = size;
>>>> +    err = config->ggtt_size ? 0 : -ENODATA;
>>>>   -    return config->ggtt_size ? 0 : -ENODATA;
>>>> +out:
>>>> +    up_write(&config->lock);
>>>> +    return err;
>>>>   }
>>>>     static int vf_get_lmem_info(struct xe_gt *gt)
>>>> @@ -474,22 +480,28 @@ static int vf_get_lmem_info(struct xe_gt *gt)
>>>>         xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>   +    down_write(&config->lock);
>>>> +
>>> also, commit message says "Protect access to GGTT config "
>>> while the patch seems to apply locking to the whole config ...
>>>
>>> what's the rationale to extend this protection?
>>> just unification?
> 
> That's true, the title doesn't match.
> 
> During post-migration recovery we call the whole `xe_gt_sriov_vf_query_config()`, which was the main reason I went for protecting the whole provisioning.
> 
> Since we can query GuC multiple times, narrowing the protection would work as well - it would just be a bit unusual to allow two threads a race over writing provisioning info.
> 
> But since these values never change, both would write the same, so no visible problem.
> 
> If you want, I can remove the protection of anything other than GGTT. The solution would be then a little confusing maybe, but would work the same.
> 
> So, which way do we go? Fix patch name+comment, or fix locking range?

if we make the VF GGTT data as tile-level, as suggested in [1], then it
can be protected with it's own tile-level lock

then GT level query can be as-is, just updates to the tile.ggtt will
quarded by the lock

[1] https://patchwork.freedesktop.org/patch/677304/?series=154627&rev=3#comment_1242644

> 
>>>
>>>>       err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_LMEM_SIZE_KEY, &size);
>>>>       if (unlikely(err))
>>>> -        return err;
>>>> +        goto out;
>>>>         if (config->lmem_size && config->lmem_size != size) {
>>>>           xe_gt_sriov_err(gt, "Unexpected LMEM reassignment: %lluM != %lluM\n",
>>>>                   size / SZ_1M, config->lmem_size / SZ_1M);
>>>> -        return -EREMCHG;
>>>> +        err = -EREMCHG;
>>>> +        goto out;
>>>>       }
>>>>         string_get_size(size, 1, STRING_UNITS_2, size_str, sizeof(size_str));
>>>>       xe_gt_sriov_dbg_verbose(gt, "LMEM %lluM %s\n", size / SZ_1M, size_str);
>>>>         config->lmem_size = size;
>>>> +    err = config->lmem_size ? 0 : -ENODATA;
>>>>   -    return config->lmem_size ? 0 : -ENODATA;
>>>> +out:
>>>> +    up_write(&config->lock);
>>>> +    return err;
>>>>   }
>>>>     static int vf_get_submission_cfg(struct xe_gt *gt)
>>>> @@ -501,23 +513,27 @@ static int vf_get_submission_cfg(struct xe_gt *gt)
>>>>         xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>   +    down_write(&config->lock);
>>>> +
>>>>       err = guc_action_query_single_klv32(guc, GUC_KLV_VF_CFG_NUM_CONTEXTS_KEY, &num_ctxs);
>>>>       if (unlikely(err))
>>>> -        return err;
>>>> +        goto out;
>>>>         err = guc_action_query_single_klv32(guc, GUC_KLV_VF_CFG_NUM_DOORBELLS_KEY, &num_dbs);
>>>>       if (unlikely(err))
>>>> -        return err;
>>>> +        goto out;
>>>>         if (config->num_ctxs && config->num_ctxs != num_ctxs) {
>>>>           xe_gt_sriov_err(gt, "Unexpected CTXs reassignment: %u != %u\n",
>>>>                   num_ctxs, config->num_ctxs);
>>>> -        return -EREMCHG;
>>>> +        err = -EREMCHG;
>>>> +        goto out;
>>>>       }
>>>>       if (config->num_dbs && config->num_dbs != num_dbs) {
>>>>           xe_gt_sriov_err(gt, "Unexpected DBs reassignment: %u != %u\n",
>>>>                   num_dbs, config->num_dbs);
>>>> -        return -EREMCHG;
>>>> +        err = -EREMCHG;
>>>> +        goto out;
>>>>       }
>>>>         xe_gt_sriov_dbg_verbose(gt, "CTXs %u DBs %u\n", num_ctxs, num_dbs);
>>>> @@ -525,7 +541,11 @@ static int vf_get_submission_cfg(struct xe_gt *gt)
>>>>       config->num_ctxs = num_ctxs;
>>>>       config->num_dbs = num_dbs;
>>>>   -    return config->num_ctxs ? 0 : -ENODATA;
>>>> +    err = config->num_ctxs ? 0 : -ENODATA;
>>>> +
>>>> +out:
>>>> +    up_write(&config->lock);
>>>> +    return err;
>>>>   }
>>>>     static void vf_cache_gmdid(struct xe_gt *gt)
>>>> @@ -579,11 +599,18 @@ int xe_gt_sriov_vf_query_config(struct xe_gt *gt)
>>>>    */
>>>>   u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt)
>>>>   {
>>>> +    struct xe_gt_sriov_vf_selfconfig *config = &gt->sriov.vf.self_config;
>>>> +    u16 val;
>>>> +
>>>>       xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>       xe_gt_assert(gt, gt->sriov.vf.guc_version.major);
>>>> -    xe_gt_assert(gt, gt->sriov.vf.self_config.num_ctxs);
>>>>   -    return gt->sriov.vf.self_config.num_ctxs;
>>>> +    down_read(&config->lock);
>>>> +    xe_gt_assert(gt, config->num_ctxs);
>>>> +    val = config->num_ctxs;
>>>> +    up_read(&config->lock);
>>>> +
>>>> +    return val;
>>>>   }
>>>>     /**
>>>> @@ -596,11 +623,18 @@ u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt)
>>>>    */
>>>>   u64 xe_gt_sriov_vf_lmem(struct xe_gt *gt)
>>>>   {
>>>> +    struct xe_gt_sriov_vf_selfconfig *config = &gt->sriov.vf.self_config;
>>>> +    u64 val;
>>>> +
>>>>       xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>       xe_gt_assert(gt, gt->sriov.vf.guc_version.major);
>>>> -    xe_gt_assert(gt, gt->sriov.vf.self_config.lmem_size);
>>>>   -    return gt->sriov.vf.self_config.lmem_size;
>>>> +    down_read(&config->lock);
>>>> +    xe_gt_assert(gt, config->lmem_size);
>>>> +    val = config->lmem_size;
>>>> +    up_read(&config->lock);
>>>> +
>>>> +    return val;
>>>>   }
>>>>     /**
>>>> @@ -613,11 +647,17 @@ u64 xe_gt_sriov_vf_lmem(struct xe_gt *gt)
>>>>    */
>>>>   u64 xe_gt_sriov_vf_ggtt(struct xe_gt *gt)
>>>>   {
>>>> +    struct xe_gt_sriov_vf_selfconfig *config = &gt->sriov.vf.self_config;
>>>> +    u64 val;
>>>> +
>>>>       xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>       xe_gt_assert(gt, gt->sriov.vf.guc_version.major);
>>>> -    xe_gt_assert(gt, gt->sriov.vf.self_config.ggtt_size);
>>>>   -    return gt->sriov.vf.self_config.ggtt_size;
>>>> +    down_read(&config->lock);
>>>> +    val = config->ggtt_size;
>>>> +    up_read(&config->lock);
>>>> +
>>>> +    return val;
>>>>   }
>>>>     /**
>>>> @@ -630,11 +670,18 @@ u64 xe_gt_sriov_vf_ggtt(struct xe_gt *gt)
>>>>    */
>>>>   u64 xe_gt_sriov_vf_ggtt_base(struct xe_gt *gt)
>>>>   {
>>>> +    struct xe_gt_sriov_vf_selfconfig *config = &gt->sriov.vf.self_config;
>>>> +    u64 val;
>>>> +
>>>>       xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>       xe_gt_assert(gt, gt->sriov.vf.guc_version.major);
>>>> -    xe_gt_assert(gt, gt->sriov.vf.self_config.ggtt_size);
>>>>   -    return gt->sriov.vf.self_config.ggtt_base;
>>>> +    down_read(&config->lock);
>>>> +    xe_gt_assert(gt, config->ggtt_size);
>>>> +    val = config->ggtt_base;
>>>> +    up_read(&config->lock);
>>>> +
>>>> +    return val;
>>>>   }
>>>>     /**
>>>> @@ -648,11 +695,16 @@ u64 xe_gt_sriov_vf_ggtt_base(struct xe_gt *gt)
>>>>   s64 xe_gt_sriov_vf_ggtt_shift(struct xe_gt *gt)
>>>>   {
>>>>       struct xe_gt_sriov_vf_selfconfig *config = &gt->sriov.vf.self_config;
>>>> +    s64 val;
>>>>         xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>       xe_gt_assert(gt, xe_gt_is_main_type(gt));
>>>>   -    return config->ggtt_shift;
>>>> +    down_read(&config->lock);
>>>> +    val = config->ggtt_shift;
>>>> +    up_read(&config->lock);
>>>> +
>>>> +    return val;
>>>>   }
>>>>     static int relay_action_handshake(struct xe_gt *gt, u32 *major, u32 *minor)
>>>> @@ -1044,6 +1096,7 @@ void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct drm_printer *p)
>>>>         xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>>>   +    down_read(&config->lock);
>>>>       drm_printf(p, "GGTT range:\t%#llx-%#llx\n",
>>>>              config->ggtt_base,
>>>>              config->ggtt_base + config->ggtt_size - 1);
>>>> @@ -1060,6 +1113,7 @@ void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct drm_printer *p)
>>>>         drm_printf(p, "GuC contexts:\t%u\n", config->num_ctxs);
>>>>       drm_printf(p, "GuC doorbells:\t%u\n", config->num_dbs);
>>>> +    up_read(&config->lock);
>>>>   }
>>>>     /**
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
>>>> index 298dedf4b009..d95857bd789b 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf_types.h
>>>> @@ -6,6 +6,7 @@
>>>>   #ifndef _XE_GT_SRIOV_VF_TYPES_H_
>>>>   #define _XE_GT_SRIOV_VF_TYPES_H_
>>>>   +#include <linux/rwsem.h>
>>>>   #include <linux/types.h>
>>>>   #include "xe_uc_fw_types.h"
>>>>   @@ -25,6 +26,8 @@ struct xe_gt_sriov_vf_selfconfig {
>>>>       u16 num_ctxs;
>>>>       /** @num_dbs: assigned number of GuC doorbells IDs. */
>>>>       u16 num_dbs;
>>>> +    /** @lock: lock for protecting access to all selfconfig fields. */
>>>> +    struct rw_semaphore lock;
>>>>   };
>>>>     /**
>>>> diff --git a/drivers/gpu/drm/xe/xe_sriov_vf.c b/drivers/gpu/drm/xe/xe_sriov_vf.c
>>>> index cdd9f8e78b2a..d6e2ed9b9bbc 100644
>>>> --- a/drivers/gpu/drm/xe/xe_sriov_vf.c
>>>> +++ b/drivers/gpu/drm/xe/xe_sriov_vf.c
>>>> @@ -197,6 +197,12 @@ static void vf_migration_init_early(struct xe_device *xe)
>>>>    */
>>>>   void xe_sriov_vf_init_early(struct xe_device *xe)
>>>>   {
>>>> +    struct xe_gt *gt;
>>>> +    unsigned int id;
>>>> +
>>>> +    for_each_gt(gt, xe, id)
>>>> +        init_rwsem(&gt->sriov.vf.self_config.lock);
>>> as before, this should be done in
>>>
>>>     xe_gt_sriov_vf_init_early
>>>
>> I pick up this change but again I think Michal's need some answers to
>> his questions.
>>
>> Matt
> 
> Sure, that is also very early call so we can move it there. (As Matt actually did, just not in this patch.)
> 
> -Tomasz
> 
>>
>>>> +
>>>>       vf_migration_init_early(xe);
>>>>   }
>>>>