Re: [PATCH v4 3/4] drm/xe: Assert that VF will never use fixed placement of BOs

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

On 10/21/2025 12:48 AM, Lis, Tomasz wrote:
> 
> On 10/20/2025 11:59 PM, Wajdeczko, Michal wrote:
>>
>> On 10/20/2025 10:58 PM, Tomasz Lis wrote:
>>> Most BOs do not care at which offset they will be accessed within
>>> GGTT or PPGTT. The few which do care, should be only created
>>> on PF, and mapped within GGTT. On VFs, mapping at fixed offset
>>> would be problematic, as each VF is granted access to a range of
>>> GGTT address space.
>> it's not a matter of being "problematic" but this is just prohibited
>> as there is no assumption which GGTT range will be assigned to the VF
> will update to "prohibited". though that's a simplification. Fixed is not the same as constant.
>>> So, since fixed addresses of GGTT mapping can only be used on PF,
>>> we can add an assert which makes sure no attempt of fixed placement
>>> will happen for a driver probed on a VF.
>>>
>>> The assert will also ensure that VF migration can be properly
>>> performed without a need for special handling of the fixed placement
>>> addresses.
>> this last sentence can be dropped
> will drop, though I can't say I agree.
>>
>>> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_bo.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>>> index 7b6502081873..8e826a4aa574 100644
>>> --- a/drivers/gpu/drm/xe/xe_bo.c
>>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>>> @@ -2259,6 +2259,12 @@ static int __xe_bo_fixed_placement(struct xe_device *xe,
>>>       struct ttm_place *place = bo->placements;
>>>       u32 vram_flag, vram_stolen_flags;
>>>   +    /*
>>> +     * to allow fixed placement in GGTT of a VF, post-migration fixups
>>> +     * would have to include shifting the page ranges
>> it's not about implementation of fixups, this is just not allowed
> 
> "not allowed" is not an explanation. That is clear from the assert itself. The comment is here to tell why.
> 
> Having only a range of addresses is not enough of an explanation, as then we could still have fixed offset which was computed so that it conforms to the range limits. Fixed does not mean hard-coded. For such computed fixed offsets, the explanation in comment is correct.

any of your "valid computed placement" will have to take into account the VF's base/size anyway

and since this "base" cannot be guaranteed to be stable across the VF life, those computations that expects or use this absolute "fixed" final placement are illegal regardless of any missing "migration-fixups" implementation

OTOH the relative offset of one BO to other BO, will be satisfied after migration without any special work

so still I'm not sure how above comment might help

> 
> -Tomasz
> 
>>
>>> +     */
>>> +    xe_assert(xe, !IS_SRIOV_VF(xe) || !(bo->flags & XE_BO_FLAG_GGTT));
>> but the assert is fine, so with commit/comment updated,
>>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>
>>> +
>>>       if (flags & (XE_BO_FLAG_USER | XE_BO_FLAG_SYSTEM))
>>>           return -EINVAL;
>>>