Re: [PATCH v7 4/4] drm/xe/vf: Fixup CTB send buffer messages after migration

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

On 09.04.2025 23:09, Lis, Tomasz wrote:
> 
> On 08.04.2025 16:23, Michal Wajdeczko wrote:
>> On 03.04.2025 20:40, Tomasz Lis wrote:

...

>>> +    u32 msg[1];
>>     u32 msg;
>> or
>>     u32 msg[GUC_HXG_MSG_MIN_LEN];
> I want an array, so will go with 2nd. Though this looks like unjustified
> obfuscation to me.

using magic "1" is an obfuscation to me

....

>>> +                     struct guc_ctb *h2g,
>> nit: this is redundant
> oh I see. by "this" you mean a spaced out separate line.

no, I just repeat that all your fixup is about H2G and since we pass ct
each helper function can easily find pointer to h2g

passing struct guc_ctb * as separate parameter may suggest that
something other than h2g could be handled (but we don't need this)

...

>>> +    /* Read header */
>> you should at least assert that avail > 0 before reading even single u32
>>
>>     xe_gt_assert(gt, avail >= GUC_CTB_MSG_MIN_LEN);
> This is a static function, and the caller is ensuring that. But sure, I
> can add the assert.
>> but since it is called in the loop, more appropriate would be:
>>
>>     if (avail < GUC_CTB_MSG_MIN_LEN)
>>         goto broken;
> 
> I don't know what you mean by that. There are not that many
> possibilities for an integer value greater than 0 but smaller than 1.

caller just checks for avail > 0
but we shall check for avail >= GUC_CTB_MSG_MIN_LEN

and we should not make any assumptions about GUC_CTB_MSG_MIN_LEN value

> 
> And by the changes I agreed to above and below, we've already ensured
> that GUC_CTB_MSG_MIN_LEN can only be equal to 1.

we are adding checks for 'avail'
we are not making new assumptions about ABI values

...

>> maybe it's time to introduce
>>
>>     u32 move_head(u32 head, u32 step, u32 size)
> We are overdefining a lot of things already. I don't think we should
> press even harder in that direction.

it's about avoiding duplicated code and wrap it into named helper

...

>>> +
>>> +/**
>>> + * xe_guc_ct_fixup_messages_with_ggtt - Fixup any pending H2G CTB
>>> messages
>>> + * @ct: pointer to CT struct of the target GuC
>>> + * @ggtt_shift: shift to be added to all GGTT addresses within the CTB
>>> + *
>>> + * Messages in guc-to-host CTB are owned by GuC and any fixups in them
>>> + * are made by GuC. But content of the host-to-guc CTB is owned by the
>>> + * KMD, so fixups to GGTT references in any pending messages need to be
>>> + * applied here.
>> s/guc-to-host/H2G
>>
>> like you have earlier and below
> Earlier is a short description. Below I am using shortcut after defining
> it in this line. This is how the long comments should look like,
> abbreviations should be defined.

but I guess you should be aware right now that your code is not the
first that introduces the CTB feature, including G2H and H2G concept and
naming, so really above kernel-doc is not a right place to suddenly use
"guc-to-host" term (which is a bad notation anyway)