Re: [Intel-xe] [PATCH 05/12] drm/xe/gsc: Parse GSC FW header

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 11/7/2023 3:45 PM, John Harrison wrote:
> On 10/27/2023 15:29, Daniele Ceraolo Spurio wrote:
>> The GSC blob starts with a layout header, from which we can move to the
>> boot directory, which in turns allows us to find the CPD. The CPD uses
>> the same format as the one in the HuC binary, so we can re-use the same
>> parsing code to get to the manifest, which contains the release and
>> security versions of the FW.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gsc_types.h |   3 +
>>   drivers/gpu/drm/xe/xe_uc_fw.c     |  77 ++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_uc_fw_abi.h | 113 ++++++++++++++++++++++++++++++
>>   3 files changed, 193 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gsc_types.h 
>> b/drivers/gpu/drm/xe/xe_gsc_types.h
>> index 135f156e3736..1bc50583fe58 100644
>> --- a/drivers/gpu/drm/xe/xe_gsc_types.h
>> +++ b/drivers/gpu/drm/xe/xe_gsc_types.h
>> @@ -14,6 +14,9 @@
>>   struct xe_gsc {
>>       /** @fw: Generic uC firmware management */
>>       struct xe_uc_fw fw;
>> +
>> +    /** @security_version: SVN found in the fetched blob */
>> +    u32 security_version;
> There is no official structure to this version number?

no, it's just a linearly increasing integer, there are no 
minor/patch/build versions involved.

>
>>   };
>>     #endif
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c 
>> b/drivers/gpu/drm/xe/xe_uc_fw.c
>> index af3e5cba606f..bb38a76eb4a6 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -12,6 +12,7 @@
>>   #include "xe_bo.h"
>>   #include "xe_device_types.h"
>>   #include "xe_force_wake.h"
>> +#include "xe_gsc.h"
>>   #include "xe_gt.h"
>>   #include "xe_map.h"
>>   #include "xe_mmio.h"
>> @@ -488,6 +489,13 @@ static int parse_cpd_header(struct xe_uc_fw 
>> *uc_fw, const void *data, size_t siz
>>       release->minor = manifest->fw_version.minor;
>>       release->patch = manifest->fw_version.hotfix;
>>   +    if (uc_fw->type == XE_UC_FW_TYPE_GSC) {
>> +        struct xe_gsc * gsc = container_of(uc_fw, struct xe_gsc, fw);
>> +
>> +        release->build = manifest->fw_version.build;
>> +        gsc->security_version = manifest->security_version;
>> +    }
>> +
>>       /* then optionally look for the css header */
>>       if (css_entry) {
>>           int ret;
>> @@ -517,6 +525,73 @@ static int parse_cpd_header(struct xe_uc_fw 
>> *uc_fw, const void *data, size_t siz
>>       return 0;
>>   }
>>   +static int parse_gsc_layout(struct xe_uc_fw *uc_fw, const void 
>> *data, size_t size)
>> +{
>> +    struct xe_gt *gt = uc_fw_to_gt(uc_fw);
>> +    const struct gsc_layout_pointers *layout = data;
>> +    const struct gsc_bpdt_header *bpdt_header = NULL;
>> +    const struct gsc_bpdt_entry *bpdt_entry = NULL;
>> +    size_t min_size = sizeof(*layout);
>> +    int i;
>> +
>> +    if (size < min_size) {
>> +        xe_gt_err(gt, "GSC FW too small! %zu < %zu\n", size, min_size);
>> +        return -ENODATA;
>> +    }
>> +
>> +    min_size = layout->boot1.offset + layout->boot1.size;
>> +    if (size < min_size) {
>> +        xe_gt_err(gt, "GSC FW too small for boot section! %zu < %zu\n",
>> +              size, min_size);
>> +        return -ENODATA;
>> +    }
>> +
>> +    min_size = sizeof(*bpdt_header);
>> +    if (layout->boot1.size < min_size) {
>> +        xe_gt_err(gt, "GSC FW boot section too small for BPDT 
>> header: %u < %zu\n",
>> +              layout->boot1.size, min_size);
>> +        return -ENODATA;
>> +    }
>> +
>> +    bpdt_header = data + layout->boot1.offset;
>> +    if (bpdt_header->signature != GSC_BPDT_HEADER_SIGNATURE) {
>> +        xe_gt_err(gt, "invalid signature for BPDT header: 0x%08x!\n",
>> +              bpdt_header->signature);
>> +        return -EINVAL;
>> +    }
>> +
>> +    min_size += sizeof(*bpdt_entry) * bpdt_header->descriptor_count;
>> +    if (layout->boot1.size < min_size) {
>> +        xe_gt_err(gt, "GSC FW boot section too small for BPDT 
>> entries: %u < %zu\n",
>> +              layout->boot1.size, min_size);
>> +        return -ENODATA;
>> +    }
>> +
>> +    bpdt_entry = (void *)bpdt_header + sizeof(*bpdt_header);
>> +    for (i = 0; i < bpdt_header->descriptor_count; i++, bpdt_entry++) {
>> +        if ((bpdt_entry->type & GSC_BPDT_ENTRY_TYPE_MASK) !=
>> +            GSC_BPDT_ENTRY_TYPE_GSC_RBE)
>> +            continue;
>> +
>> +        min_size = bpdt_entry->sub_partition_offset;
>> +
>> +        /* the CPD header parser will check that the CPD header fits */
>> +        if (layout->boot1.size < min_size) {
>> +            xe_gt_err(gt, "GSC FW boot section too small for CPD 
>> offset: %u < %zu\n",
>> +                  layout->boot1.size, min_size);
>> +            return -ENODATA;
>> +        }
>> +
>> +        return parse_cpd_header(uc_fw,
>> +                    (void *)bpdt_header + min_size,
>> +                    layout->boot1.size - min_size,
> Could compare this calculation against bpdt_entry->sub_partition_size 
> before bothering to try to decode a partial CPD?

The CPD parser checks for the size. I could add an extra check here and 
bail early, but why add extra case to optimize a scenario that should 
never happen?

>
>> +                    "RBEP.man", NULL);
>> +    }
>> +
>> +    xe_gt_err(gt, "couldn't find CPD header in GSC binary!\n");
>> +    return -ENODATA;
>> +}
>> +
>>   static int parse_headers(struct xe_uc_fw *uc_fw, const struct 
>> firmware *fw)
>>   {
>>       int ret;
>> @@ -526,6 +601,8 @@ static int parse_headers(struct xe_uc_fw *uc_fw, 
>> const struct firmware *fw)
>>        * releases use GSC CPD headers.
>>        */
>>       switch (uc_fw->type) {
>> +    case XE_UC_FW_TYPE_GSC:
>> +        return parse_gsc_layout(uc_fw, fw->data, fw->size);
>>       case XE_UC_FW_TYPE_HUC:
>>           ret = parse_cpd_header(uc_fw, fw->data, fw->size, 
>> "HUCP.man", "huc_fw");
>>           if (!ret || ret != -ENOENT)
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw_abi.h 
>> b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> index d6725c963251..edf2a448f4bb 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> @@ -140,6 +140,58 @@ static_assert(sizeof(struct uc_css_header) == 128);
>>    *    |      RSA Key (MTL+ only)                       |
>>    *    |      ...                                       |
>>    *    +================================================+
>> + *
>> + * The GSC binary starts instead with a layout header, which 
>> contains the
>> + * locations of the various partitions of the binary. The one we're 
>> interested
>> + * in is the boot1 partition, where we can find a BPDT header 
>> followed by
>> + * entries, one of which points to the RBE sub-section of the 
>> partition, which
>> + * contains the CPD. The GSC blob does not contain a CSS-based 
>> binary, so we
>> + * only need to look for the manifest, which is under the "RBEP.man" 
>> CPD entry.
>> + * Note that we have no need to find where the actual FW code is 
>> inside the
>> + * image because the GSC ROM will itself parse the headers to find 
>> it and load
>> + * it.
>> + * The GSC firmware header layout looks like this::
>> + *
>> + *    +================================================+
>> + *    |  Layout Pointers                               |
>> + *    |      ...                                       |
>> + *    |      Boot1 offset >---------------------------|------o
>> + *    |      ...                                       |      |
>> + *    +================================================+      |
>> + *                                                            |
>> + *    +================================================+      |
>> + *    |  BPDT header |<-----o
>> + *    +================================================+
>> + *    |  BPDT entries[]                                |
>> + *    |      entry1                                    |
>> + *    |      ...                                       |
>> + *    |      entryX                                    |
>> + *    |          type == GSC_RBE                       |
>> + *    |          offset >-----------------------------|------o
>> + *    |      ...                                       |      |
>> + *    +================================================+      |
>> + *                                                            |
>> + *    +================================================+      |
>> + *    |  CPD Header |<-----o
>> + *    +================================================+
>> + *    |  CPD entries[]                                 |
>> + *    |      entry1                                    |
>> + *    |      ...                                       |
>> + *    |      entryX                                    |
>> + *    |          "RBEP.man"                            |
>> + *    |           ...                                  |
>> + *    |           offset >----------------------------|------o
>> + *    |      ...                                       |      |
>> + *    +================================================+      |
>> + *                                                            |
>> + *    +================================================+      |
>> + *    | Manifest Header |<-----o
>> + *    |  ...                                           |
>> + *    |  FW version                                    |
>> + *    |  ...                                           |
>> + *    |  Security version                              |
>> + *    |  ...                                           |
>> + *    +================================================+
>>    */
>>     struct gsc_version {
>> @@ -149,6 +201,67 @@ struct gsc_version {
>>       u16 build;
>>   } __packed;
>>   +struct gsc_partition {
>> +    u32 offset;
>> +    u32 size;
>> +} __packed;
>> +
>> +struct gsc_layout_pointers {
>> +    u8 rom_bypass_vector[16];
>> +
>> +    /* size of pointers layout not including ROM bypass vector */
> pointers layout -> layout pointers?

I'll just simplify it to "size of this header section"

>
>> +    u16 size;
>> +
>> +    /*
>> +     * bit0: Backup copy of layout pointers exist
> exist -> exists

ack

>
>> +     * bits1-15: reserved
>> +     */
>> +    u8 flags;
>> +
>> +    u8 reserved;
>> +
>> +    u32 crc32;
> Should we be doing anything to test against this crc or the one in the 
> bpdt_header below?

No, the GSC ROM checks those at load time.

Daniele

>
> John.
>
>> +
>> +    struct gsc_partition datap;
>> +    struct gsc_partition boot1;
>> +    struct gsc_partition boot2;
>> +    struct gsc_partition boot3;
>> +    struct gsc_partition boot4;
>> +    struct gsc_partition boot5;
>> +    struct gsc_partition temp_pages;
>> +} __packed;
>> +
>> +/* Boot partition structures */
>> +struct gsc_bpdt_header {
>> +    u32 signature;
>> +#define GSC_BPDT_HEADER_SIGNATURE 0x000055AA
>> +
>> +    u16 descriptor_count; /* num of entries after the header */
>> +
>> +    u8 version;
>> +    u8 configuration;
>> +
>> +    u32 crc32;
>> +
>> +    u32 build_version;
>> +    struct gsc_version tool_version;
>> +} __packed;
>> +
>> +struct gsc_bpdt_entry {
>> +    /*
>> +     * Bits 0-15: BPDT entry type
>> +     * Bits 16-17: reserved
>> +     * Bit 18: code sub-partition
>> +     * Bits 19-31: reserved
>> +     */
>> +    u32 type;
>> +#define GSC_BPDT_ENTRY_TYPE_MASK GENMASK(15, 0)
>> +#define GSC_BPDT_ENTRY_TYPE_GSC_RBE 0x1
>> +
>> +    u32 sub_partition_offset; /* from the base of the BPDT header */
>> +    u32 sub_partition_size;
>> +} __packed;
>> +
>>   /* Code partition directory (CPD) structures */
>>   struct gsc_cpd_header_v2 {
>>       u32 header_marker;
>