[PATCH v7 02/12] drm/xe: Correct BMG VSEC header sizing

[Ruhl, Michael J]

>-----Original Message-----
>From: David Box <david.e.box at linux.intel.com>
>Sent: Friday, July 11, 2025 1:59 PM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>
>Cc: platform-driver-x86 at vger.kernel.org; intel-xe at lists.freedesktop.org;
>hdegoede at redhat.com; ilpo.jarvinen at linux.intel.com; De Marchi, Lucas
><lucas.demarchi at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
>thomas.hellstrom at linux.intel.com; airlied at gmail.com; simona at ffwll.ch
>Subject: Re: [PATCH v7 02/12] drm/xe: Correct BMG VSEC header sizing
>
>On Wed, Jul 09, 2025 at 02:44:48PM -0400, Michael J. Ruhl wrote:
>> The intel_vsec_header information for the crashlog feature is
>> incorrect.
>>
>> Update the VSEC header with correct sizing and count.
>>
>> Since the crashlog entries are "merged" (num_entries = 2), the
>> separate capabilities entries must be merged as well.
>>
>> Acked-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Fixes: 0c45e76fcc62 ("drm/xe/vsec: Support BMG devices")
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_vsec.c | 20 +++++---------------
>>  1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index 3e573b0b7ebd..67238fc57a4d 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>> @@ -32,28 +32,18 @@ static struct intel_vsec_header bmg_telemetry = {
>>  	.offset = BMG_DISCOVERY_OFFSET,
>>  };
>>
>> -static struct intel_vsec_header bmg_punit_crashlog = {
>> -	.length = 0x10,
>> +static struct intel_vsec_header bmg_crashlog = {
>> +	.length = 0x18,
>
>This doesn't look right. The length field is meant to represent the size
>of the entire DVSEC, and it should remain 0x10 even for Crashlog.

Ok.  I see this in the PMT doc. (6.6).  And I now understand the value.
I will update.

>This field really ought to be validated in vsec.c, but I see that it
>isn’t currently. The rev field, which also isn’t set here, determines the
>length. When rev is 1, the length is 0x10. vsec does a basic revision
>check when reading the capability from config space but doesn’t validate
>this driver-provided structure. That’s a gap I’ll take care of
>separately.
>
>For now, even though this length value isn’t actually read by vsec.c,
>it should still be set correctly to 0x10. Also, add the rev field here
>and set it to 1. I see multiple headers missing the rev. That should be
>fixed in a separate patch ahead of this one. All Intel DVEC capabilities
>use revision 1.

Ok, I am updating the patch set and will repost.  Will re-test on Mondy.

M

>David
>
>>  	.id = VSEC_ID_CRASHLOG,
>> -	.num_entries = 1,
>> -	.entry_size = 4,
>> +	.num_entries = 2,
>> +	.entry_size = 6,
>>  	.tbir = 0,
>>  	.offset = BMG_DISCOVERY_OFFSET + 0x60,
>>  };
>>
>> -static struct intel_vsec_header bmg_oobmsm_crashlog = {
>> -	.length = 0x10,
>> -	.id = VSEC_ID_CRASHLOG,
>> -	.num_entries = 1,
>> -	.entry_size = 4,
>> -	.tbir = 0,
>> -	.offset = BMG_DISCOVERY_OFFSET + 0x78,
>> -};
>> -
>>  static struct intel_vsec_header *bmg_capabilities[] = {
>>  	&bmg_telemetry,
>> -	&bmg_punit_crashlog,
>> -	&bmg_oobmsm_crashlog,
>> +	&bmg_crashlog,
>>  	NULL
>>  };
>>
>> --
>> 2.50.0
>>
>>