[Intel-xe] [PATCH v2 2/5] drm/xe/huc: Extract version and binary offset from new HuC headers
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Thu Oct 19 23:46:10 UTC 2023
On 10/19/2023 4:07 PM, Lucas De Marchi wrote:
> On Wed, Oct 18, 2023 at 03:57:04PM -0700, Daniele Ceraolo Spurio wrote:
>> The GSC-enabled HuC binary starts with a GSC header, which is followed
>> by the legacy-style CSS header and the binary itself. We can parse the
>> GSC headers to find the HuC version and the location of the binary to
>> be used for the DMA transfer.
>>
>> The parsing function has been designed to be re-used for the GSC binary,
>> so the entry names are external parameters (because the GSC uses
>> different ones) and the CSS entry is optional (because the GSC doesn't
>> have it).
>>
>> v2: move new code to uc_fw.c, better comments and error checking, split
>> old code move to separate patch (Lucas), move headers and
>> documentation to uc_fw_abi.h.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>> Cc: John Harrison <John.C.Harrison at Intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> Documentation/gpu/xe/xe_firmware.rst | 3 +
>> drivers/gpu/drm/xe/xe_uc_fw.c | 137 ++++++++++++++++++++++++++-
>> drivers/gpu/drm/xe/xe_uc_fw.h | 2 +-
>> drivers/gpu/drm/xe/xe_uc_fw_abi.h | 120 +++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_uc_fw_types.h | 2 +
>> 5 files changed, 261 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/gpu/xe/xe_firmware.rst
>> b/Documentation/gpu/xe/xe_firmware.rst
>> index f1ac6f608930..afcb561cd37d 100644
>> --- a/Documentation/gpu/xe/xe_firmware.rst
>> +++ b/Documentation/gpu/xe/xe_firmware.rst
>> @@ -10,6 +10,9 @@ Firmware Layout
>> .. kernel-doc:: drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> :doc: CSS-based Firmware Layout
>>
>> +.. kernel-doc:: drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> + :doc: GSC-based Firmware Layout
>> +
>> Write Once Protected Content Memory (WOPCM) Layout
>> ==================================================
>>
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c
>> b/drivers/gpu/drm/xe/xe_uc_fw.c
>> index 7345f0409cf9..2680b5082ea0 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -403,9 +403,142 @@ static int parse_css_header(struct xe_uc_fw
>> *uc_fw, const void *fw_data, size_t
>> return 0;
>> }
>>
>> +static bool is_cpd_header(const void* data)
>> +{
>> + const u32 *marker = data;
>> +
>> + return *marker == GSC_CPD_HEADER_MARKER;
>> +}
>> +
>> +static inline u32 entry_offset(const struct gsc_cpd_entry *entry)
>
> no need for the inline. Let the compiler figure it out.
>
>> +{
>> + return entry->offset & GSC_CPD_ENTRY_OFFSET_MASK;
>> +}
>> +
>> +/* Refer to the "GSC-based Firmware Layout" documentation entry for
>> details */
>> +static int parse_cpd_header(struct xe_uc_fw *uc_fw, const void
>> *data, size_t size,
>> + const char *manifest_entry, const char *css_entry)
>> +{
>> + struct xe_gt *gt = uc_fw_to_gt(uc_fw);
>> + struct xe_device *xe = gt_to_xe(gt);
>> + const struct gsc_cpd_header_v2 *header = data;
>> + const struct gsc_cpd_entry *entry;
>> + size_t min_size = sizeof(*header);
>> + int i;
>> +
>> + if (!manifest_entry)
>> + return -EINVAL;
>
> should this be an assert since it's a programming error to pass this as
> NULL?
>
>> +
>> + if (size < sizeof(*header)) {
>
> min_size
>
>> + xe_gt_err(gt, "FW too small! %zu < %zu\n", size, min_size);
>
> or change this one to be sizeof() and only assign min_size for later
> use.
>
>> + return -ENODATA;
>> + }
>> +
>> + if (!is_cpd_header(header)) {
>> + xe_gt_err(gt, "Trying to CPD parse a non-CPD header\n");
>> + return -ENODATA;
>> + }
>> +
>> + if (header->header_length < sizeof(struct gsc_cpd_header_v2)) {
>> + xe_gt_err(gt, "invalid CPD header length %u!\n",
>> header->header_length);
>> + return -EINVAL;
>> + }
>> +
>> + min_size = header->header_length + sizeof(*entry) *
>> header->num_of_entries;
>> + if (size < min_size) {
>> + xe_gt_err(gt, "FW too small! %zu < %zu\n", size, min_size);
>> + return -ENODATA;
>> + }
>> +
>> + entry = data + header->header_length;
>> +
>> + for (i = 0; i < header->num_of_entries; i++, entry++) {
>> + if (strcmp(entry->name, manifest_entry) == 0) {
>> + const struct gsc_manifest_header *manifest;
>> +
>> + min_size = entry_offset(entry) + sizeof(struct
>> gsc_manifest_header);
>> + if (size < min_size) {
>> + xe_gt_err(gt, "FW too small! %zu < %zu\n", size,
>> min_size);
>> + return -ENODATA;
>> + }
>> +
>> + manifest = data + entry_offset(entry);
>> +
>> + uc_fw->major_ver_found = manifest->fw_version.major;
>> + uc_fw->minor_ver_found = manifest->fw_version.minor;
>> + uc_fw->patch_ver_found = manifest->fw_version.hotfix;
>
> continue;
>
> if we can't ever have css_entry == manifest_entry
Yeah they can never be the same
>
>> + }
>> +
>> + if (css_entry && strcmp(entry->name, css_entry) == 0) {
>> + u32 offset = entry_offset(entry);
>> + int ret;
>> +
>> + /*
>> + * This section does not contain a CSS entry on DG2. We
>> + * don't support DG2 HuC right now, so no need to handle
>> + * it, just add a reminder in case that changes.
>> + */
>> + xe_assert(xe, xe->info.platform != XE_DG2);
>> +
>> + /* the CSS header parser will check that the CSS header
>> fits */
>> + if (offset > size) {
>> + xe_gt_err(gt, "FW too small! %zu < %u\n", size,
>> offset);
>> + return -ENODATA;
>> + }
>> +
>> + ret = parse_css_header(uc_fw, data + offset, size -
>> offset);
>> + if (ret)
>> + return ret;
>> +
>> + uc_fw->css_offset = offset;
>> + }
>
> do we want to stop the search as soon as both are found? maybe even as 2
> loops instead of one.
I like the 2 loops idea. There shouldn't be many entries anyway, so not
a perf issue.
>
>> + }
>> +
>> + if (!uc_fw->major_ver_found) {
>> + xe_gt_err(gt, "Failed to find %s version in manifest!\n",
>> + xe_uc_fw_type_repr(uc_fw->type));
>> + return -ENODATA;
>> + }
>> +
>> + if (css_entry && !uc_fw->css_offset) {
>> + xe_gt_err(gt, "Failed to find CSS offset for %s\n",
>> + xe_uc_fw_type_repr(uc_fw->type));
>> + return -ENODATA;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int parse_headers(struct xe_uc_fw *uc_fw, const struct
>> firmware *fw)
>> {
>> - return parse_css_header(uc_fw, fw->data, fw->size);
>> + /*
>> + * Quick check to make sure there is enough space for the
>> headers. FWs
>> + * are expected to be hundreds of KB big, so we can keep it
>> simple and
>> + * start by checking that we have at least a page (which is
>> enough for
>> + * the headers), while further checks will be done based on the
>> headers
>> + * contents.
>> + */
>> + if (fw->size < PAGE_SIZE)
>> + return -ENODATA;
>
> functions below should be able to survive with no crashes if they always
> check the there's enough data in the buffer to cover what they are
> trying to access. I don't like much checking for an arbitrary PAGE_SIZE
> number and eventually failing that in future platforms when/if the
> header grows.
This was here to avoid having to put a size check in is_cpd_header(),
but I can go with your suggestion below instead and move is_cpd_header()
inside parse_cpd_header() and remove this check.
ack on all the other comments, I'll respin once CI is back online so
I'll be able to get results for merge.
Daniele
>
>> +
>> + /*
>> + * All GuC releases and older HuC ones use CSS headers, while
>> newer HuC
>> + * releases use GSC CPD headers.
>> + */
>> + switch (uc_fw->type) {
>> + case XE_UC_FW_TYPE_HUC:
>> + if (is_cpd_header(fw->data))
>> + return parse_cpd_header(uc_fw, fw->data, fw->size,
>> + "HUCP.man", "huc_fw");
>
> alternative to use one specific error number like ENOENT in
> parse_cpd_header(), so you know to fallback on that
>
> case XE_UC_FW_TYPE_HUC:
> ret = parse_cpd_header(uc_fw, fw->data, fw->size, "HUCP.man",
> "huc_fw");
> if (!ret || ret != -ENOENT)
> return ret;
>
> fallthrough;
>
> then the is_cpd_header() can just be inside the function actually
> parsing the cpd_header and you can remove the extra error message from
> there.
>
>> + fallthrough;
>> + case XE_UC_FW_TYPE_GUC:
>> + return parse_css_header(uc_fw, fw->data, fw->size);
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> }
>>
>> int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
>> @@ -511,7 +644,7 @@ static int uc_fw_xfer(struct xe_uc_fw *uc_fw, u32
>> offset, u32 dma_flags)
>> xe_force_wake_assert_held(gt_to_fw(gt), XE_FW_GT);
>>
>> /* Set the source address for the uCode */
>> - src_offset = uc_fw_ggtt_offset(uc_fw);
>> + src_offset = uc_fw_ggtt_offset(uc_fw) + uc_fw->css_offset;
>> xe_mmio_write32(gt, DMA_ADDR_0_LOW, lower_32_bits(src_offset));
>> xe_mmio_write32(gt, DMA_ADDR_0_HIGH, upper_32_bits(src_offset));
>>
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.h
>> b/drivers/gpu/drm/xe/xe_uc_fw.h
>> index a519c77d4962..1d1a0c156cdf 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.h
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.h
>> @@ -21,7 +21,7 @@ void xe_uc_fw_print(struct xe_uc_fw *uc_fw, struct
>> drm_printer *p);
>>
>> static inline u32 xe_uc_fw_rsa_offset(struct xe_uc_fw *uc_fw)
>> {
>> - return sizeof(struct uc_css_header) + uc_fw->ucode_size;
>> + return sizeof(struct uc_css_header) + uc_fw->ucode_size +
>> uc_fw->css_offset;
>> }
>>
>> static inline void xe_uc_fw_change_status(struct xe_uc_fw *uc_fw,
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> index edae7bb3cd72..d6725c963251 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>> @@ -85,4 +85,124 @@ struct uc_css_header {
>> } __packed;
>> static_assert(sizeof(struct uc_css_header) == 128);
>>
>> +/**
>> + * DOC: GSC-based Firmware Layout
>> + *
>> + * The GSC-based firmware structure is used for GSC releases on all
>> platforms
>> + * and for HuC releases starting from DG2/MTL. Older HuC releases
>> use the
>> + * CSS-based layout instead. Differently from the CSS headers, the
>> GSC headers
>> + * uses a directory + entries structure (i.e., there is array of
>> addresses
>> + * pointing to specific header extensions identified by a name).
>> Although the
>> + * header structures are the same, some of the entries are specific
>> to GSC while
>> + * others are specific to HuC. The manifest header entry, which
>> includes basic
>> + * information about the binary (like the version) is always
>> present, but it is
>> + * named differently based on the binary type.
>> + *
>> + * The HuC binary starts with a Code Partition Directory (CPD)
>> header. The
>> + * entries we're interested in for use in the driver are:
>> + *
>> + * 1. "HUCP.man": points to the manifest header for the HuC.
>> + * 2. "huc_fw": points to the FW code. On platforms that support
>> load via DMA
>> + * and 2-step HuC authentication (i.e. MTL+) this is a full
>> CSS-based binary,
>> + * while if the GSC is the one doing the load (which only happens
>> on DG2)
>> + * this section only contains the uCode.
>> + *
>> + * The GSC-based HuC firmware layout looks like this::
>> + *
>> + * +================================================+
>> + * | CPD Header |
>> + * +================================================+
>> + * | CPD entries[] |
>> + * | entry1 |
>> + * | ... |
>> + * | entryX |
>> + * | "HUCP.man" |
>> + * | ... |
>> + * | offset >----------------------------|------o
>> + * | ... | |
>> + * | entryY | |
>> + * | "huc_fw" | |
>> + * | ... | |
>> + * | offset >----------------------------|----------o
>> + * +================================================+ | |
>> + * | |
>> + * +================================================+ | |
>> + * | Manifest Header |<-----o |
>> + * | ... | |
>> + * | FW version | |
>> + * | ... | |
>> + * +================================================+ |
>> + * |
>> + * +================================================+ |
>> + * | FW binary |<---------o
>> + * | CSS (MTL+ only) |
>> + * | uCode |
>> + * | RSA Key (MTL+ only) |
>> + * | ... |
>> + * +================================================+
>
> awesome! much easier now.
>
> Consider the comments above just nits and feel free to incorporate the
> suggestions or leave as is.
>
>
> Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
>
> thanks
> Lucas De Marchi
>
>> + */
>> +
>> +struct gsc_version {
>> + u16 major;
>> + u16 minor;
>> + u16 hotfix;
>> + u16 build;
>> +} __packed;
>> +
>> +/* Code partition directory (CPD) structures */
>> +struct gsc_cpd_header_v2 {
>> + u32 header_marker;
>> +#define GSC_CPD_HEADER_MARKER 0x44504324
>> +
>> + u32 num_of_entries;
>> + u8 header_version;
>> + u8 entry_version;
>> + u8 header_length; /* in bytes */
>> + u8 flags;
>> + u32 partition_name;
>> + u32 crc32;
>> +} __packed;
>> +
>> +struct gsc_cpd_entry {
>> + u8 name[12];
>> +
>> + /*
>> + * Bits 0-24: offset from the beginning of the code partition
>> + * Bit 25: huffman compressed
>> + * Bits 26-31: reserved
>> + */
>> + u32 offset;
>> +#define GSC_CPD_ENTRY_OFFSET_MASK GENMASK(24, 0)
>> +#define GSC_CPD_ENTRY_HUFFMAN_COMP BIT(25)
>> +
>> + /*
>> + * Module/Item length, in bytes. For Huffman-compressed modules,
>> this
>> + * refers to the uncompressed size. For software-compressed
>> modules,
>> + * this refers to the compressed size.
>> + */
>> + u32 length;
>> +
>> + u8 reserved[4];
>> +} __packed;
>> +
>> +struct gsc_manifest_header {
>> + u32 header_type; /* 0x4 for manifest type */
>> + u32 header_length; /* in dwords */
>> + u32 header_version;
>> + u32 flags;
>> + u32 vendor;
>> + u32 date;
>> + u32 size; /* In dwords, size of entire manifest (header +
>> extensions) */
>> + u32 header_id;
>> + u32 internal_data;
>> + struct gsc_version fw_version;
>> + u32 security_version;
>> + struct gsc_version meu_kit_version;
>> + u32 meu_manifest_version;
>> + u8 general_data[4];
>> + u8 reserved3[56];
>> + u32 modulus_size; /* in dwords */
>> + u32 exponent_size; /* in dwords */
>> +} __packed;
>> +
>> #endif
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw_types.h
>> b/drivers/gpu/drm/xe/xe_uc_fw_types.h
>> index 444bff83cdbe..1650599303c8 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw_types.h
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw_types.h
>> @@ -113,6 +113,8 @@ struct xe_uc_fw {
>> u32 rsa_size;
>> /** @ucode_size: micro kernel size */
>> u32 ucode_size;
>> + /** @css_offset: offset within the blob at which the CSS is
>> located */
>> + u32 css_offset;
>>
>> /** @private_data_size: size of private data found in uC css
>> header */
>> u32 private_data_size;
>> --
>> 2.41.0
>>
More information about the Intel-xe
mailing list