[PATCH 1/4] drm/xe/guc: Update CSS header structures
John Harrison
john.c.harrison at intel.com
Fri Aug 1 17:10:28 UTC 2025
On 7/31/2025 4:11 PM, Daniele Ceraolo Spurio wrote:
> On 7/24/2025 5:21 PM, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> Rework the CSS header structure according to recent updates to the GuC
>> API spec. Also include more field definitions.
>>
>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_uc_fw.c | 20 +++++-----
>> drivers/gpu/drm/xe/xe_uc_fw_abi.h | 64 ++++++++++++++++++++-----------
>> 2 files changed, 51 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c
>> b/drivers/gpu/drm/xe/xe_uc_fw.c
>> index a236f1d37248..c8378f3e0032 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -344,11 +344,11 @@ static int guc_read_css_info(struct xe_uc_fw
>> *uc_fw, struct uc_css_header *css)
>
> Since we now have a specific type for it, it might be worth changing
> this function to accept struct uc_css_guc_info instead of struct
> uc_css_header.
That works.
>
>> return -EINVAL;
>> }
>> - compatibility->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR,
>> css->submission_version);
>> - compatibility->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR,
>> css->submission_version);
>> - compatibility->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
>> css->submission_version);
>> + compatibility->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR,
>> css->guc_info.submission_version);
>> + compatibility->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR,
>> css->guc_info.submission_version);
>> + compatibility->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
>> css->guc_info.submission_version);
>> - uc_fw->private_data_size = css->private_data_size;
>> + uc_fw->private_data_size = css->guc_info.private_data_size;
>> return 0;
>> }
>> @@ -417,8 +417,8 @@ static int parse_css_header(struct xe_uc_fw
>> *uc_fw, const void *fw_data, size_t
>> css = (struct uc_css_header *)fw_data;
>> /* Check integrity of size values inside CSS header */
>> - size = (css->header_size_dw - css->key_size_dw -
>> css->modulus_size_dw -
>> - css->exponent_size_dw) * sizeof(u32);
>> + size = (css->header_size_dw - css->rsa_info.key_size_dw -
>> css->rsa_info.modulus_size_dw -
>> + css->rsa_info.exponent_size_dw) * sizeof(u32);
>> if (unlikely(size != sizeof(struct uc_css_header))) {
>> drm_warn(&xe->drm,
>> "%s firmware %s: unexpected header size: %zu != %zu\n",
>> @@ -431,7 +431,7 @@ static int parse_css_header(struct xe_uc_fw
>> *uc_fw, const void *fw_data, size_t
>> uc_fw->ucode_size = (css->size_dw - css->header_size_dw) *
>> sizeof(u32);
>> /* now RSA */
>> - uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
>> + uc_fw->rsa_size = css->rsa_info.key_size_dw * sizeof(u32);
>> /* At least, it should have header, uCode and RSA. Size of
>> all three. */
>> size = sizeof(struct uc_css_header) + uc_fw->ucode_size +
>> @@ -444,9 +444,9 @@ static int parse_css_header(struct xe_uc_fw
>> *uc_fw, const void *fw_data, size_t
>> }
>> /* Get version numbers from the CSS header */
>> - release->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR,
>> css->sw_version);
>> - release->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR,
>> css->sw_version);
>> - release->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
>> css->sw_version);
>> + release->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR,
>> css->guc_info.sw_version);
>> + release->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR,
>> css->guc_info.sw_version);
>> + release->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
>> css->guc_info.sw_version);
>> if (uc_fw->type == XE_UC_FW_TYPE_GUC)
>> return guc_read_css_info(uc_fw, css);
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> index 87ade41209d0..44d529dbce3b 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> @@ -44,6 +44,39 @@
>> * in fw. So driver will load a truncated firmware in this case.
>> */
>> +struct uc_css_rsa_info {
>> + u32 key_size_dw;
>> + u32 modulus_size_dw;
>> + u32 exponent_size_dw;
>> +} __packed;
>> +
>> +struct uc_css_guc_info {
>> + u32 time;
>> +#define CSS_TIME_HOUR (0xFF << 0)
>> +#define CSS_DATE_MIN (0xFF << 8)
>> +#define CSS_DATE_SEC (0xFFFF << 16)
>> + u32 reserved0[5];
>> + u32 sw_version;
>> +#define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
>> +#define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
>> +#define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
>> + u32 submission_version;
>> + u32 reserved1[11];
>> + u32 header_info;
>> +#define CSS_HEADER_INFO_SVN (0xFF)
>> +#define CSS_HEADER_INFO_COPY_VALID (0x1 << 31)
>> + u32 private_data_size;
>> + u32 ukernel_info;
>> +#define CSS_UKERNEL_INFO_DEVICEID (0xFFFF << 16)
>> +#define CSS_UKERNEL_INFO_PRODKEY (0xFF << 8)
>> +#define CSS_UKERNEL_INFO_BUILDTYPE (0x3 << 2)
>> +#define CSS_UKERNEL_INFO_BUILDTYPE_PROD 0
>> +#define CSS_UKERNEL_INFO_BUILDTYPE_PREPROD 1
>> +#define CSS_UKERNEL_INFO_BUILDTYPE_DEBUG 2
>> +#define CSS_UKERNEL_INFO_ENCSTATUS (0x1 << 1)
>> +#define CSS_UKERNEL_INFO_COPY_VALID (0x1 << 0)
>
> I'd skip defining CSS_UKERNEL_INFO_COPY_VALID and
> CSS_HEADER_INFO_COPY_VALID. They're only set in the HW's copy of those
> fields, which are only consumed by bootrom; we'll never see them set
> from the driver POV (or even from the POV of someone hexdumping the
> binary), so having the define has no use.
It's more about not having holes in the definition leaving people to
wonder if that is a bug in the driver, a deliberate obfuscation, etc.
One could call it _RESERVED but that seems inaccurate when it does
actually have a definition.
>
>> +} __packed;
>> +
>> struct uc_css_header {
>> u32 module_type;
>> /*
>> @@ -52,36 +85,21 @@ struct uc_css_header {
>> */
>> u32 header_size_dw;
>> u32 header_version;
>> - u32 module_id;
>> + u32 reserved0;
>> u32 module_vendor;
>> u32 date;
>> -#define CSS_DATE_DAY (0xFF << 0)
>> -#define CSS_DATE_MONTH (0xFF << 8)
>> -#define CSS_DATE_YEAR (0xFFFF << 16)
>> +#define CSS_DATE_DAY (0xFF << 0)
>> +#define CSS_DATE_MONTH (0xFF << 8)
>> +#define CSS_DATE_YEAR (0xFFFF << 16)
>> u32 size_dw; /* uCode plus header_size_dw */
>> - u32 key_size_dw;
>> - u32 modulus_size_dw;
>> - u32 exponent_size_dw;
>> - u32 time;
>> -#define CSS_TIME_HOUR (0xFF << 0)
>> -#define CSS_DATE_MIN (0xFF << 8)
>> -#define CSS_DATE_SEC (0xFFFF << 16)
>> - char username[8];
>> - char buildnumber[12];
>> - u32 sw_version;
>> -#define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
>> -#define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
>> -#define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
>> union {
>> - u32 submission_version; /* only applies to GuC */
>> - u32 reserved2;
>> + u32 reserved1[3];
>> + struct uc_css_rsa_info rsa_info;
>
> Do we actually support any binary where this block is reserved instead
> of containing the rsa_info?
Not yet, but there are plans...
John.
>
> Daniele
>
>> };
>> - u32 reserved0[12];
>> union {
>> - u32 private_data_size; /* only applies to GuC */
>> - u32 reserved1;
>> + u32 reserved2[22];
>> + struct uc_css_guc_info guc_info;
>> };
>> - u32 header_info;
>> } __packed;
>> static_assert(sizeof(struct uc_css_header) == 128);
>
More information about the Intel-xe
mailing list