Re: [PATCH v4 04/12] drm/xe/cper: Construct CPER record for hardware errors

["Nilawar, Badal" <badal.nilawar@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6344 bytes --]


On 26-10-2026 23:06, Ceraolo Spurio, Daniele wrote:
>
>
> On 10/26/2026 10:21 AM, Nilawar, Badal wrote:
>>
>>
>> On 24-10-2026 04:34, Matt Roper wrote:
>>> On Fri, Oct 23, 2026 at 04:32:48AM +0200, Raag Jadav wrote:
>>>> On Thu, Oct 22, 2026 at 10:24:45PM +0530, Nilawar, Badal wrote:
>>>>> On 22-10-2026 19:10, Nilawar, Badal wrote:
>>>>>> On 14-10-2026 16:00, Raag Jadav wrote:
>>>>>>>> +static const struct xe_platform_id_entry xe_platform_ids[] = {
>>>>>>>> +	/* 0x674C  platform/8086:674c */
>>>>>>>> +	{ 0x674C, GUID_INIT(0x9046afe5, 0x9041, 0x5124,
>>>>>>>> +			    0x86, 0x14, 0x92, 0x55, 0x0d, 0x9e, 0x9d, 0xa6) },
>>>>>>>> +	/* 0x674D  platform/8086:674d */
>>>>>>>> +	{ 0x674D, GUID_INIT(0xf95911b8, 0x6564, 0x5905,
>>>>>>>> +			    0xa6, 0xa7, 0xb9, 0xbf, 0x54, 0xab, 0x6c, 0x27) },
>>>>>>>> +	/* 0x674E  platform/8086:674e */
>>>>>>>> +	{ 0x674E, GUID_INIT(0xb8d24106, 0x0108, 0x59f1,
>>>>>>>> +			    0x8f, 0xfc, 0x9f, 0xc4, 0xa9, 0xc9, 0xb7, 0xb0) },
>>>>>>>> +	/* 0x674F  platform/8086:674f */
>>>>>>>> +	{ 0x674F, GUID_INIT(0xdd8dd88e, 0xef2f, 0x59b1,
>>>>>>>> +			    0x99, 0x15, 0xdb, 0x6f, 0x1d, 0xc9, 0xb4, 0x5d) },
>>>>>>>> +	/* 0x6750  platform/8086:6750 */
>>>>>>>> +	{ 0x6750, GUID_INIT(0x1dd66c44, 0x87d5, 0x524a,
>>>>>>>> +			    0xad, 0x6e, 0xfc, 0x18, 0x9d, 0x42, 0x6f, 0x1a) },
>>>>>>>> +	/* 0x6751  platform/8086:6751 */
>>>>>>>> +	{ 0x6751, GUID_INIT(0x17d0dd04, 0x2fad, 0x5400,
>>>>>>>> +			    0x99, 0xc8, 0x9b, 0x45, 0x51, 0x3b, 0x67, 0xe3) },
>>>>>>>> +};
>>>>>>> This is unrelated to cper and shouldn't be here. I'll rely on Matt's
>>>>>>> input on how to deal with these.
>>>>>> Agreed, I don't think this is the ideal place either. For now, I'll keep
>>>>>> it here and, based on Matt's input, consider moving it to xe_pci.c,
>>>>>> which maintains the PCI ID list for the platforms supported by Xe.
>>>>>>
>>>>> One reason I placed the table here is that these aren’t general-purpose
>>>>> platform GUIDs; they’re UUIDv5 values generated specifically for CPER’s
>>>>> platform_id field, using platform/8086:<device-id> as the name.
>>>>>
>>>>> Since CPER is their only consumer, keeping the mapping with the CPER code
>>>>> seems reasonable to me.  I’m happy to follow Matt’s input if there’s a
>>>>> preferred place for this kind of mapping.
>>>> My only concern is that we usually don't have device ids lying around in
>>>> random components, but that I think is upto the maintainers.
>>> Yeah, I'm not familiar with these structures.  If they're CPER-specific,
>>> then keeping them in the CPER file seems natural to me.
>> Fine, I will keep them here only.
>>> But it's also
>>> true that it could be easy for these to get out of sync with our general
>>> PCI ID list if PCI IDs are added/removed.
>>>
>>> If a new PCI ID shows up, how do we know what the values should be here?
>>> Are they documented in the bspec somewhere?
>>
>> No these are generated as using uuidgen utility as UUIDv5.
>>
>> $ INTEL_NS=$(uuidgen --sha1 -n @dns -N "intel.com")
>>
>> uuidgen --sha1 -n "$INTEL_NS" -N "platform/8086:674c"
>> 9046afe5-9041-5124-8614-92550d9e9da6
>>
>>
>>>    How do these values wind up
>>> getting used?
>>
>> This UUID is populated in CPER record to identify the platform/SKU 
>> associated with it.
>>
>>
>>> Are there uapi / ABI implications of these values?
>>
>> No uapi implications. As they are part of CPER record should remain 
>> stable for given platform/PCI ID.
>>
>> I am considering generating the platform UUID dynamically using 
>> UUIDv5, with the PCI vendor/device ID as the name.
>>
>> This keeps the UUID deterministic for a given PCI ID and avoids 
>> maintaining a separate mapping table when PCI IDs are added or removed.
>>
>> +/* Namespace: uuidgen --sha1 -n @dns -N "intel.com" */
>> +#define INTEL_CPER_NAMESPACE \
>> +       UUID_INIT(0x3e54f727, 0x549c, 0x5ab3, \
>> +                 0x90, 0x73, 0x52, 0x2a, 0x81, 0x15, 0x20, 0xe7)
>> +
>>
>> +/*
>> + * Platform GUID is UUIDv5 (RFC 9562) of "platform/8086:<devid>" in the
>> + * Intel CPER namespace, stored in the mixed-endian guid_t layout 
>> used by CPER.
>> + */
>> +static void cper_platform_id(const struct pci_dev *pdev, guid_t *guid)
>> +{
>> +       static const uuid_t ns = INTEL_CPER_NAMESPACE;
>> +       u8 digest[SHA1_DIGEST_SIZE];
>> +       struct sha1_ctx ctx;
>> +       char name[sizeof("platform/8086:ffff")];
>> +       int len;
>> +
>> +       len = scnprintf(name, sizeof(name), "platform/%04x:%04x",
>> +                       pdev->vendor, pdev->device);
>> +
>> +       sha1_init(&ctx);
>> +       sha1_update(&ctx, ns.b, UUID_SIZE);
>> +       sha1_update(&ctx, name, len);
>> +       sha1_final(&ctx, digest);
>> +
>> +       digest[6] = (digest[6] & 0x0f) | 0x50;
>> +       digest[8] = (digest[8] & 0x3f) | 0x80;
>> +
>> +       *guid = GUID_INIT(get_unaligned_be32(&digest[0]),
>> +                         get_unaligned_be16(&digest[4]),
>> +                         get_unaligned_be16(&digest[6]),
>> +                         digest[8], digest[9], digest[10], digest[11],
>> +                         digest[12], digest[13], digest[14], 
>> digest[15]);
>>  }
>>
> If we want to go this way, IMO this logic should be placed in 
> lib/uuid.c and not in Xe. But ...
>
>>> Aside from everything else, should we also add some kind of kunit test
>>> that makes sure we each PCI ID has an entry in this table and vice-versa
>>> to make sure we don't forget to update this table when platform PCI IDs
>>> are added/removed?
>>
>> Agree, Kunit can be added as follow up.
>>
> ... any reason the Kunit isn't enough with a static table? If we add 
> it immediately instead of later, we'd be sure to get a CI failure if a 
> new PCI ID is added without the matching GUID. Compared to the dynamic 
> generation, we wouldn't get the GUID for free if we get a new PCI ID, 
> but overall it should be less work IMO.
>

Agree, a static table makes more sense. I'll keep the static table and 
add a WARN_ONCE() for PCI IDs without a corresponding GUID. Since the 
current table covers all existing CRI PCI IDs, I'd prefer to add the 
KUnit coverage as a follow-up with the SW CPER work.

Badal

> Daniele
>
>> Thanks,
>> Badal
>>
>>> Matt
>>>
>>>> Raag

[-- Attachment #2: Type: text/html, Size: 10689 bytes --]