[PATCH v6 4/6] drm/xe/guc: Add GuC log init config in LFD format

[John Harrison]

On 7/30/2025 4:40 PM, Dong, Zhanjun wrote:
> There are multiple comments, this reply is focus on 1 topic, see my 
> comments below.
>
> Regards,
> Zhanjun
>
> On 2025-07-29 8:27 p.m., John Harrison wrote:
>> On 7/21/2025 6:35 PM, Zhanjun Dong wrote:
>>> Add support to output GuC log init config (LIC) in LFD format.
>>>
>>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_guc_log.c | 204 
>>> ++++++++++++++++++++++++++++++++
>>>   1 file changed, 204 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/ 
>>> xe_guc_log.c
>>> index d2256773aaf3..609a9fd5ba0c 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
> ...
>>> +
>>> +static int
>>> +xe_guc_log_output_lfd_init(struct drm_printer *p, struct 
>>> xe_guc_log_snapshot *snapshot,
>>> +               struct guc_lic_save *config)
>>> +{
>>> +    int type, len;
>>> +    size_t size = 0;
>>> +    char buf[GUC_LFD_BUFFER_SIZE] = {0};
>> This will add 1KB of zeros as a global static object. You should 
>> never give large arrays an empty initialisation like that. I recall 
>> someone once doing that with a 16MB array and it took us ages to work 
>> out why the driver was so huge in debug builds! Also, this is putting 
>> a 1KB array on the stack. Kernel stack space is limited and you 
>> should never put large objects on the stack unless absolutely 
>> necessary. Just use kzalloc() for this.
>>
>
> I agree with you for static or global arrays cases, which will cause 
> .bss, .rodata or other(depends on platform) read-only segment 
> increase, that's because the static/global data is initialized by 
> loader, loader copy data from .bss/.rodata segment to memory, that's 
> how it works.
> While for this case, which is clear local stack memory to zero, that's 
> not load's task, that is compiler's job. Compiler will generate either 
> memset call or machine code to clear it to zero, for example on X86, 
> disassembly code is:
>
> xe_guc_log_output_lfd_init(struct drm_printer *p, struct 
> xe_guc_log_snapshot *snapshot,
>  501:    65 48 8b 04 25 28 00     mov    %gs:0x28,%rax
>  508:    00 00
> // I set GUC_LFD_BUFFER_SIZE set to 0x258(decimal 600), compiler has 
> some calculation and make it 0x268
>  50a:    48 89 84 24 68 02 00     mov    %rax,0x268(%rsp)
>  511:    00
>  512:    31 c0                    xor    %eax,%eax
>     char buf[GUC_LFD_BUFFER_SIZE] = {0};
>  514:    48 c7 44 24 10 00 00     movq   $0x0,0x10(%rsp)
>  51b:    00 00
>  51d:    f3 48 ab                 rep stos %rax,%es:(%rdi)
>         size += xe_guc_log_add_klv(&buf[size], GUC_LFD_BUFFER_SIZE - 
> size, type, config);
>  520:    49 8d 47 10              lea    0x10(%r15),%rax
>  524:    4c 8d 34 04              lea    (%rsp,%rax,1),%r14
>     index = xe_guc_log_add_lfd_header(buf, buf_size);
>
> Data cleared by instructions, there is no access to RO segment.
>
> Local buf={0} should be similar to a memset call. If you have concerns 
> about support of compiler/compiler options/processor combination, let 
> me know, we can change it to memset call.
>
> About the size, I see warning of stack usage is more than 1K, will 
> reduce it to 600 or 800 to avoid the warning.
> LFD header is small, 600/800 looks good.
>
Several hundred bytes of object is still a lot of object to be putting 
on the stack. For anything over a hundred bytes, you should be asking if 
this is the best way to do this. And in this case, it is not. Simple 
re-working of the data structures would mean you never need to create an 
oversized buffer for run-time construction of fields.

John.