[Intel-xe] [PATCH v2 0/5] drm/xe: Clear-media HuC support for MTL

[Intel-xe] [PATCH v2 0/5] drm/xe: Clear-media HuC support for MTL

[Daniele Ceraolo Spurio]

MTL supports HuC on the media GT with a 2-step authentication flow:

1 - authentication via the GuC for clear-media
2 - authentication via the GSC for protected content

This series introduces support for the first step. The actual flow to
authenticate via the GuC is the same as in older platforms, but the
HuC binary now has a new manifest which we need to parse. From the
manifest we can extract the offset within the binary that we need to
provide to the GuC for loading.

The second step of the authentication will come as part of GSC support.

v2: stop using the filename to check which headers are available, split
up old code move to separate patch, add new code and docs to uc_fw files
instead of huc ones.

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>

Daniele Ceraolo Spurio (5):
  drm/xe/uc: Prepare for parsing of different header types
  drm/xe/huc: Extract version and binary offset from new HuC headers
  drm/xe/huc: HuC is not supported on GTs that don't have video engines
  drm/xe/huc: Don't re-auth HuC if it's already authenticated
  drm/xe/huc: Define HuC for MTL

 Documentation/gpu/xe/xe_firmware.rst |   5 +-
 drivers/gpu/drm/xe/xe_huc.c          |  17 +-
 drivers/gpu/drm/xe/xe_uc_fw.c        | 265 +++++++++++++++++++++------
 drivers/gpu/drm/xe/xe_uc_fw.h        |   2 +-
 drivers/gpu/drm/xe/xe_uc_fw_abi.h    | 127 ++++++++++++-
 drivers/gpu/drm/xe/xe_uc_fw_types.h  |   2 +
 6 files changed, 355 insertions(+), 63 deletions(-)

-- 
2.41.0

[Intel-xe] [PATCH v2 1/5] drm/xe/uc: Prepare for parsing of different header types

[Daniele Ceraolo Spurio]

GSC binaries and newer HuC ones use GSC-style headers instead of the
CSS. In preparation for adding support for such parsing, split out the
current parsing code to its own function, to make it cleaner to add the
new paths. The existing doc section has also been renamed to narrow it
to CSS-based binaries.

v2: new patch in series, split out from next patch for easier reviewing

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>
---
 Documentation/gpu/xe/xe_firmware.rst |   2 +-
 drivers/gpu/drm/xe/xe_uc_fw.c        | 117 +++++++++++++++------------
 drivers/gpu/drm/xe/xe_uc_fw_abi.h    |   7 +-
 3 files changed, 72 insertions(+), 54 deletions(-)

diff --git a/Documentation/gpu/xe/xe_firmware.rst b/Documentation/gpu/xe/xe_firmware.rst
index c01246ae99f5..f1ac6f608930 100644
--- a/Documentation/gpu/xe/xe_firmware.rst
+++ b/Documentation/gpu/xe/xe_firmware.rst
@@ -8,7 +8,7 @@ Firmware Layout
 ===============
 
 .. kernel-doc:: drivers/gpu/drm/xe/xe_uc_fw_abi.h
-   :doc: Firmware Layout
+   :doc: CSS-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 edaa195fa25f..7345f0409cf9 100644
--- a/drivers/gpu/drm/xe/xe_uc_fw.c
+++ b/drivers/gpu/drm/xe/xe_uc_fw.c
@@ -13,6 +13,7 @@
 #include "xe_device_types.h"
 #include "xe_force_wake.h"
 #include "xe_gt.h"
+#include "xe_huc.h"
 #include "xe_map.h"
 #include "xe_mmio.h"
 #include "xe_module.h"
@@ -344,57 +345,22 @@ static int uc_fw_check_version_requirements(struct xe_uc_fw *uc_fw)
 	return -ENOEXEC;
 }
 
-int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
+/* Refer to the "CSS-based Firmware Layout" documentation entry for details */
+static int parse_css_header(struct xe_uc_fw *uc_fw, const void *fw_data, size_t fw_size)
 {
 	struct xe_device *xe = uc_fw_to_xe(uc_fw);
-	struct xe_gt *gt = uc_fw_to_gt(uc_fw);
-	struct xe_tile *tile = gt_to_tile(gt);
-	struct device *dev = xe->drm.dev;
-	const struct firmware *fw = NULL;
 	struct uc_css_header *css;
-	struct xe_bo *obj;
 	size_t size;
-	int err;
-
-	/*
-	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
-	 * before we're looked at the HW caps to see if we have uc support
-	 */
-	BUILD_BUG_ON(XE_UC_FIRMWARE_UNINITIALIZED);
-	xe_assert(xe, !uc_fw->status);
-	xe_assert(xe, !uc_fw->path);
-
-	uc_fw_auto_select(xe, uc_fw);
-	xe_uc_fw_change_status(uc_fw, uc_fw->path ?
-			       XE_UC_FIRMWARE_SELECTED :
-			       XE_UC_FIRMWARE_NOT_SUPPORTED);
-
-	if (!xe_uc_fw_is_supported(uc_fw))
-		return 0;
-
-	uc_fw_override(uc_fw);
-
-	/* an empty path means the firmware is disabled */
-	if (!xe_device_uc_enabled(xe) || !(*uc_fw->path)) {
-		xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_DISABLED);
-		drm_dbg(&xe->drm, "%s disabled", xe_uc_fw_type_repr(uc_fw->type));
-		return 0;
-	}
-
-	err = request_firmware(&fw, uc_fw->path, dev);
-	if (err)
-		goto fail;
 
 	/* Check the size of the blob before examining buffer contents */
-	if (unlikely(fw->size < sizeof(struct uc_css_header))) {
+	if (unlikely(fw_size < sizeof(struct uc_css_header))) {
 		drm_warn(&xe->drm, "%s firmware %s: invalid size: %zu < %zu\n",
 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
-			 fw->size, sizeof(struct uc_css_header));
-		err = -ENODATA;
-		goto fail;
+			 fw_size, sizeof(struct uc_css_header));
+		return -ENODATA;
 	}
 
-	css = (struct uc_css_header *)fw->data;
+	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 -
@@ -403,9 +369,8 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
 		drm_warn(&xe->drm,
 			 "%s firmware %s: unexpected header size: %zu != %zu\n",
 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
-			 fw->size, sizeof(struct uc_css_header));
-		err = -EPROTO;
-		goto fail;
+			 fw_size, sizeof(struct uc_css_header));
+		return -EPROTO;
 	}
 
 	/* uCode size must calculated from other sizes */
@@ -417,12 +382,11 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
 	/* At least, it should have header, uCode and RSA. Size of all three. */
 	size = sizeof(struct uc_css_header) + uc_fw->ucode_size +
 		uc_fw->rsa_size;
-	if (unlikely(fw->size < size)) {
+	if (unlikely(fw_size < size)) {
 		drm_warn(&xe->drm, "%s firmware %s: invalid size: %zu < %zu\n",
 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
-			 fw->size, size);
-		err = -ENOEXEC;
-		goto fail;
+			 fw_size, size);
+		return -ENOEXEC;
 	}
 
 	/* Get version numbers from the CSS header */
@@ -433,6 +397,60 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
 	uc_fw->patch_ver_found = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
 					   css->sw_version);
 
+	if (uc_fw->type == XE_UC_FW_TYPE_GUC)
+		guc_read_css_info(uc_fw, css);
+
+	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);
+}
+
+int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
+{
+	struct xe_device *xe = uc_fw_to_xe(uc_fw);
+	struct xe_gt *gt = uc_fw_to_gt(uc_fw);
+	struct xe_tile *tile = gt_to_tile(gt);
+	struct device *dev = xe->drm.dev;
+	const struct firmware *fw = NULL;
+	struct xe_bo *obj;
+	int err;
+
+	/*
+	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
+	 * before we're looked at the HW caps to see if we have uc support
+	 */
+	BUILD_BUG_ON(XE_UC_FIRMWARE_UNINITIALIZED);
+	xe_assert(xe, !uc_fw->status);
+	xe_assert(xe, !uc_fw->path);
+
+	uc_fw_auto_select(xe, uc_fw);
+	xe_uc_fw_change_status(uc_fw, uc_fw->path ?
+			       XE_UC_FIRMWARE_SELECTED :
+			       XE_UC_FIRMWARE_NOT_SUPPORTED);
+
+	if (!xe_uc_fw_is_supported(uc_fw))
+		return 0;
+
+	uc_fw_override(uc_fw);
+
+	/* an empty path means the firmware is disabled */
+	if (!xe_device_uc_enabled(xe) || !(*uc_fw->path)) {
+		xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_DISABLED);
+		drm_dbg(&xe->drm, "%s disabled", xe_uc_fw_type_repr(uc_fw->type));
+		return 0;
+	}
+
+	err = request_firmware(&fw, uc_fw->path, dev);
+	if (err)
+		goto fail;
+
+	err = parse_headers(uc_fw, fw);
+	if (err)
+		goto fail;
+
 	drm_info(&xe->drm, "Using %s firmware from %s version %u.%u.%u\n",
 		 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
 		 uc_fw->major_ver_found, uc_fw->minor_ver_found, uc_fw->patch_ver_found);
@@ -441,9 +459,6 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
 	if (err)
 		goto fail;
 
-	if (uc_fw->type == XE_UC_FW_TYPE_GUC)
-		guc_read_css_info(uc_fw, css);
-
 	obj = xe_bo_create_from_data(xe, tile, fw->data, fw->size,
 				     ttm_bo_type_kernel,
 				     XE_BO_CREATE_VRAM_IF_DGFX(tile) |
diff --git a/drivers/gpu/drm/xe/xe_uc_fw_abi.h b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
index 89e994ed4e00..edae7bb3cd72 100644
--- a/drivers/gpu/drm/xe/xe_uc_fw_abi.h
+++ b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
@@ -10,9 +10,12 @@
 #include <linux/types.h>
 
 /**
- * DOC: Firmware Layout
+ * DOC: CSS-based Firmware Layout
  *
- * The GuC/HuC firmware layout looks like this::
+ * The CSS-based firmware structure is used for GuC releases on all platforms
+ * and for HuC releases up to DG1. Starting from DG2/MTL the HuC uses the GSC
+ * layout instead.
+ * The CSS firmware layout looks like this::
  *
  *      +======================================================================+
  *      |  Firmware blob                                                       |
-- 
2.41.0

[Intel-xe] [PATCH v2 2/5] drm/xe/huc: Extract version and binary offset from new HuC headers

[Daniele Ceraolo Spurio]

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@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>
---
 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)
+{
+	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;
+
+	if (size < sizeof(*header)) {
+		xe_gt_err(gt, "FW too small! %zu < %zu\n", size, min_size);
+		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;
+		}
+
+		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;
+		}
+	}
+
+	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;
+
+	/*
+	 * 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");
+		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)                       |
+ *	|      ...                                       |
+ *	+================================================+
+ */
+
+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

[Intel-xe] [PATCH v2 3/5] drm/xe/huc: HuC is not supported on GTs that don't have video engines

[Daniele Ceraolo Spurio]

On MTL-style multi-gt platforms, the HuC is only available on the media
GT, so we need to consider it as not supported on the render GT.

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>
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/xe_huc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_huc.c b/drivers/gpu/drm/xe/xe_huc.c
index 293403d16f25..386efa180c1c 100644
--- a/drivers/gpu/drm/xe/xe_huc.c
+++ b/drivers/gpu/drm/xe/xe_huc.c
@@ -35,10 +35,19 @@ huc_to_guc(struct xe_huc *huc)
 
 int xe_huc_init(struct xe_huc *huc)
 {
-	struct xe_device *xe = huc_to_xe(huc);
+	struct xe_gt *gt = huc_to_gt(huc);
+	struct xe_tile *tile = gt_to_tile(gt);
+	struct xe_device *xe = gt_to_xe(gt);
 	int ret;
 
 	huc->fw.type = XE_UC_FW_TYPE_HUC;
+
+	/* On platforms with a media GT the HuC is only available there */
+	if (tile->media_gt && (gt != tile->media_gt)) {
+		xe_uc_fw_change_status(&huc->fw, XE_UC_FIRMWARE_NOT_SUPPORTED);
+		return 0;
+	}
+
 	ret = xe_uc_fw_init(&huc->fw);
 	if (ret)
 		goto out;
-- 
2.41.0

[Intel-xe] [PATCH v2 4/5] drm/xe/huc: Don't re-auth HuC if it's already authenticated

[Daniele Ceraolo Spurio]

On newer platforms the HuC survives reset and stays authenticated, so no
need to re-authenticate it.

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>
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/xe_huc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_huc.c b/drivers/gpu/drm/xe/xe_huc.c
index 386efa180c1c..2f176badab26 100644
--- a/drivers/gpu/drm/xe/xe_huc.c
+++ b/drivers/gpu/drm/xe/xe_huc.c
@@ -83,6 +83,12 @@ int xe_huc_auth(struct xe_huc *huc)
 
 	xe_assert(xe, !xe_uc_fw_is_running(&huc->fw));
 
+	/* On newer platforms the HuC survives reset, so no need to re-auth */
+	if (xe_mmio_read32(gt, HUC_KERNEL_LOAD_INFO) & HUC_LOAD_SUCCESSFUL) {
+		xe_uc_fw_change_status(&huc->fw, XE_UC_FIRMWARE_RUNNING);
+		return 0;
+	}
+
 	if (!xe_uc_fw_is_loaded(&huc->fw))
 		return -ENOEXEC;
 
-- 
2.41.0

[Intel-xe] [PATCH v2 5/5] drm/xe/huc: Define HuC for MTL

[Daniele Ceraolo Spurio]

MTL uses a GSC-enabled binary. We expect all GSC-enabled binaries to be
defined with a "_gsc", so we can check for that in the name to
determine if the binary has the GSC headers or not.

v2: don't use the filename to identify the header type (Lucas)

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_uc_fw.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
index 2680b5082ea0..0fa1b3415eff 100644
--- a/drivers/gpu/drm/xe/xe_uc_fw.c
+++ b/drivers/gpu/drm/xe/xe_uc_fw.c
@@ -113,12 +113,13 @@ struct fw_blobs_by_type {
 	fw_def(ROCKETLAKE,	major_ver(i915,	guc,	tgl,	70, 5))		\
 	fw_def(TIGERLAKE,	major_ver(i915,	guc,	tgl,	70, 5))
 
-#define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver)				\
-	fw_def(DG1,		no_ver(i915,	huc,	dg1))			\
-	fw_def(ALDERLAKE_P,	no_ver(i915,	huc,	tgl))			\
-	fw_def(ALDERLAKE_S,	no_ver(i915,	huc,	tgl))			\
-	fw_def(ROCKETLAKE,	no_ver(i915,	huc,	tgl))			\
-	fw_def(TIGERLAKE,	no_ver(i915,	huc,	tgl))
+#define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver)		\
+	fw_def(METEORLAKE,	no_ver(i915,	huc_gsc,	mtl))		\
+	fw_def(DG1,		no_ver(i915,	huc,		dg1))		\
+	fw_def(ALDERLAKE_P,	no_ver(i915,	huc,		tgl))		\
+	fw_def(ALDERLAKE_S,	no_ver(i915,	huc,		tgl))		\
+	fw_def(ROCKETLAKE,	no_ver(i915,	huc,		tgl))		\
+	fw_def(TIGERLAKE,	no_ver(i915,	huc,		tgl))
 
 #define MAKE_FW_PATH(dir__, uc__, shortname__, version__)			\
 	__stringify(dir__) "/" __stringify(shortname__) "_" __stringify(uc__) version__ ".bin"
-- 
2.41.0

Re: [Intel-xe] [PATCH v2 1/5] drm/xe/uc: Prepare for parsing of different header types

[Lucas De Marchi]

On Wed, Oct 18, 2023 at 03:57:03PM -0700, Daniele Ceraolo Spurio wrote:
>GSC binaries and newer HuC ones use GSC-style headers instead of the
>CSS. In preparation for adding support for such parsing, split out the
>current parsing code to its own function, to make it cleaner to add the
>new paths. The existing doc section has also been renamed to narrow it
>to CSS-based binaries.
>
>v2: new patch in series, split out from next patch for easier reviewing
>
>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>
>---
> Documentation/gpu/xe/xe_firmware.rst |   2 +-
> drivers/gpu/drm/xe/xe_uc_fw.c        | 117 +++++++++++++++------------
> drivers/gpu/drm/xe/xe_uc_fw_abi.h    |   7 +-
> 3 files changed, 72 insertions(+), 54 deletions(-)
>
>diff --git a/Documentation/gpu/xe/xe_firmware.rst b/Documentation/gpu/xe/xe_firmware.rst
>index c01246ae99f5..f1ac6f608930 100644
>--- a/Documentation/gpu/xe/xe_firmware.rst
>+++ b/Documentation/gpu/xe/xe_firmware.rst
>@@ -8,7 +8,7 @@ Firmware Layout
> ===============
>
> .. kernel-doc:: drivers/gpu/drm/xe/xe_uc_fw_abi.h
>-   :doc: Firmware Layout
>+   :doc: CSS-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 edaa195fa25f..7345f0409cf9 100644
>--- a/drivers/gpu/drm/xe/xe_uc_fw.c
>+++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>@@ -13,6 +13,7 @@
> #include "xe_device_types.h"
> #include "xe_force_wake.h"
> #include "xe_gt.h"
>+#include "xe_huc.h"
> #include "xe_map.h"
> #include "xe_mmio.h"
> #include "xe_module.h"
>@@ -344,57 +345,22 @@ static int uc_fw_check_version_requirements(struct xe_uc_fw *uc_fw)
> 	return -ENOEXEC;
> }
>
>-int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
>+/* Refer to the "CSS-based Firmware Layout" documentation entry for details */
>+static int parse_css_header(struct xe_uc_fw *uc_fw, const void *fw_data, size_t fw_size)
> {
> 	struct xe_device *xe = uc_fw_to_xe(uc_fw);
>-	struct xe_gt *gt = uc_fw_to_gt(uc_fw);
>-	struct xe_tile *tile = gt_to_tile(gt);
>-	struct device *dev = xe->drm.dev;
>-	const struct firmware *fw = NULL;
> 	struct uc_css_header *css;
>-	struct xe_bo *obj;
> 	size_t size;
>-	int err;
>-
>-	/*
>-	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
>-	 * before we're looked at the HW caps to see if we have uc support
>-	 */
>-	BUILD_BUG_ON(XE_UC_FIRMWARE_UNINITIALIZED);
>-	xe_assert(xe, !uc_fw->status);
>-	xe_assert(xe, !uc_fw->path);
>-
>-	uc_fw_auto_select(xe, uc_fw);
>-	xe_uc_fw_change_status(uc_fw, uc_fw->path ?
>-			       XE_UC_FIRMWARE_SELECTED :
>-			       XE_UC_FIRMWARE_NOT_SUPPORTED);
>-
>-	if (!xe_uc_fw_is_supported(uc_fw))
>-		return 0;
>-
>-	uc_fw_override(uc_fw);
>-
>-	/* an empty path means the firmware is disabled */
>-	if (!xe_device_uc_enabled(xe) || !(*uc_fw->path)) {
>-		xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_DISABLED);
>-		drm_dbg(&xe->drm, "%s disabled", xe_uc_fw_type_repr(uc_fw->type));
>-		return 0;
>-	}
>-
>-	err = request_firmware(&fw, uc_fw->path, dev);
>-	if (err)
>-		goto fail;
>
> 	/* Check the size of the blob before examining buffer contents */
>-	if (unlikely(fw->size < sizeof(struct uc_css_header))) {
>+	if (unlikely(fw_size < sizeof(struct uc_css_header))) {
> 		drm_warn(&xe->drm, "%s firmware %s: invalid size: %zu < %zu\n",
> 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>-			 fw->size, sizeof(struct uc_css_header));
>-		err = -ENODATA;
>-		goto fail;
>+			 fw_size, sizeof(struct uc_css_header));
>+		return -ENODATA;
> 	}
>
>-	css = (struct uc_css_header *)fw->data;
>+	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 -
>@@ -403,9 +369,8 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 		drm_warn(&xe->drm,
> 			 "%s firmware %s: unexpected header size: %zu != %zu\n",
> 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>-			 fw->size, sizeof(struct uc_css_header));
>-		err = -EPROTO;
>-		goto fail;
>+			 fw_size, sizeof(struct uc_css_header));
>+		return -EPROTO;
> 	}
>
> 	/* uCode size must calculated from other sizes */
>@@ -417,12 +382,11 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 	/* At least, it should have header, uCode and RSA. Size of all three. */
> 	size = sizeof(struct uc_css_header) + uc_fw->ucode_size +
> 		uc_fw->rsa_size;
>-	if (unlikely(fw->size < size)) {
>+	if (unlikely(fw_size < size)) {
> 		drm_warn(&xe->drm, "%s firmware %s: invalid size: %zu < %zu\n",
> 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>-			 fw->size, size);
>-		err = -ENOEXEC;
>-		goto fail;
>+			 fw_size, size);
>+		return -ENOEXEC;


Unrelated to this specific patch, but this makes it more evident:
why are we returning 3 different error codes for the same thing?
Maybe we can make them all -ENODATA or -EINVAL



Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi


> 	}
>
> 	/* Get version numbers from the CSS header */
>@@ -433,6 +397,60 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 	uc_fw->patch_ver_found = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
> 					   css->sw_version);
>
>+	if (uc_fw->type == XE_UC_FW_TYPE_GUC)
>+		guc_read_css_info(uc_fw, css);
>+
>+	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);
>+}
>+
>+int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
>+{
>+	struct xe_device *xe = uc_fw_to_xe(uc_fw);
>+	struct xe_gt *gt = uc_fw_to_gt(uc_fw);
>+	struct xe_tile *tile = gt_to_tile(gt);
>+	struct device *dev = xe->drm.dev;
>+	const struct firmware *fw = NULL;
>+	struct xe_bo *obj;
>+	int err;
>+
>+	/*
>+	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
>+	 * before we're looked at the HW caps to see if we have uc support
>+	 */
>+	BUILD_BUG_ON(XE_UC_FIRMWARE_UNINITIALIZED);
>+	xe_assert(xe, !uc_fw->status);
>+	xe_assert(xe, !uc_fw->path);
>+
>+	uc_fw_auto_select(xe, uc_fw);
>+	xe_uc_fw_change_status(uc_fw, uc_fw->path ?
>+			       XE_UC_FIRMWARE_SELECTED :
>+			       XE_UC_FIRMWARE_NOT_SUPPORTED);
>+
>+	if (!xe_uc_fw_is_supported(uc_fw))
>+		return 0;
>+
>+	uc_fw_override(uc_fw);
>+
>+	/* an empty path means the firmware is disabled */
>+	if (!xe_device_uc_enabled(xe) || !(*uc_fw->path)) {
>+		xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_DISABLED);
>+		drm_dbg(&xe->drm, "%s disabled", xe_uc_fw_type_repr(uc_fw->type));
>+		return 0;
>+	}
>+
>+	err = request_firmware(&fw, uc_fw->path, dev);
>+	if (err)
>+		goto fail;
>+
>+	err = parse_headers(uc_fw, fw);
>+	if (err)
>+		goto fail;
>+
> 	drm_info(&xe->drm, "Using %s firmware from %s version %u.%u.%u\n",
> 		 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
> 		 uc_fw->major_ver_found, uc_fw->minor_ver_found, uc_fw->patch_ver_found);
>@@ -441,9 +459,6 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 	if (err)
> 		goto fail;
>
>-	if (uc_fw->type == XE_UC_FW_TYPE_GUC)
>-		guc_read_css_info(uc_fw, css);
>-
> 	obj = xe_bo_create_from_data(xe, tile, fw->data, fw->size,
> 				     ttm_bo_type_kernel,
> 				     XE_BO_CREATE_VRAM_IF_DGFX(tile) |
>diff --git a/drivers/gpu/drm/xe/xe_uc_fw_abi.h b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>index 89e994ed4e00..edae7bb3cd72 100644
>--- a/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>+++ b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>@@ -10,9 +10,12 @@
> #include <linux/types.h>
>
> /**
>- * DOC: Firmware Layout
>+ * DOC: CSS-based Firmware Layout
>  *
>- * The GuC/HuC firmware layout looks like this::
>+ * The CSS-based firmware structure is used for GuC releases on all platforms
>+ * and for HuC releases up to DG1. Starting from DG2/MTL the HuC uses the GSC
>+ * layout instead.
>+ * The CSS firmware layout looks like this::
>  *
>  *      +======================================================================+
>  *      |  Firmware blob                                                       |
>-- 
>2.41.0
>

Re: [Intel-xe] [PATCH v2 2/5] drm/xe/huc: Extract version and binary offset from new HuC headers

[Lucas De Marchi]

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@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>
>---
> 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

>+		}
>+
>+		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.

>+	}
>+
>+	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.

>+
>+	/*
>+	 * 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@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
>

Re: [Intel-xe] [PATCH v2 1/5] drm/xe/uc: Prepare for parsing of different header types

[Lucas De Marchi]

On Wed, Oct 18, 2023 at 03:57:03PM -0700, Daniele Ceraolo Spurio wrote:
>GSC binaries and newer HuC ones use GSC-style headers instead of the
>CSS. In preparation for adding support for such parsing, split out the
>current parsing code to its own function, to make it cleaner to add the
>new paths. The existing doc section has also been renamed to narrow it
>to CSS-based binaries.
>
>v2: new patch in series, split out from next patch for easier reviewing
>
>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>
>---
> Documentation/gpu/xe/xe_firmware.rst |   2 +-
> drivers/gpu/drm/xe/xe_uc_fw.c        | 117 +++++++++++++++------------
> drivers/gpu/drm/xe/xe_uc_fw_abi.h    |   7 +-
> 3 files changed, 72 insertions(+), 54 deletions(-)
>
>diff --git a/Documentation/gpu/xe/xe_firmware.rst b/Documentation/gpu/xe/xe_firmware.rst
>index c01246ae99f5..f1ac6f608930 100644
>--- a/Documentation/gpu/xe/xe_firmware.rst
>+++ b/Documentation/gpu/xe/xe_firmware.rst
>@@ -8,7 +8,7 @@ Firmware Layout
> ===============
>
> .. kernel-doc:: drivers/gpu/drm/xe/xe_uc_fw_abi.h
>-   :doc: Firmware Layout
>+   :doc: CSS-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 edaa195fa25f..7345f0409cf9 100644
>--- a/drivers/gpu/drm/xe/xe_uc_fw.c
>+++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>@@ -13,6 +13,7 @@
> #include "xe_device_types.h"
> #include "xe_force_wake.h"
> #include "xe_gt.h"
>+#include "xe_huc.h"

it doesn't seem this include belongs to this patch


Lucas De Marchi

> #include "xe_map.h"
> #include "xe_mmio.h"
> #include "xe_module.h"
>@@ -344,57 +345,22 @@ static int uc_fw_check_version_requirements(struct xe_uc_fw *uc_fw)
> 	return -ENOEXEC;
> }
>
>-int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
>+/* Refer to the "CSS-based Firmware Layout" documentation entry for details */
>+static int parse_css_header(struct xe_uc_fw *uc_fw, const void *fw_data, size_t fw_size)
> {
> 	struct xe_device *xe = uc_fw_to_xe(uc_fw);
>-	struct xe_gt *gt = uc_fw_to_gt(uc_fw);
>-	struct xe_tile *tile = gt_to_tile(gt);
>-	struct device *dev = xe->drm.dev;
>-	const struct firmware *fw = NULL;
> 	struct uc_css_header *css;
>-	struct xe_bo *obj;
> 	size_t size;
>-	int err;
>-
>-	/*
>-	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
>-	 * before we're looked at the HW caps to see if we have uc support
>-	 */
>-	BUILD_BUG_ON(XE_UC_FIRMWARE_UNINITIALIZED);
>-	xe_assert(xe, !uc_fw->status);
>-	xe_assert(xe, !uc_fw->path);
>-
>-	uc_fw_auto_select(xe, uc_fw);
>-	xe_uc_fw_change_status(uc_fw, uc_fw->path ?
>-			       XE_UC_FIRMWARE_SELECTED :
>-			       XE_UC_FIRMWARE_NOT_SUPPORTED);
>-
>-	if (!xe_uc_fw_is_supported(uc_fw))
>-		return 0;
>-
>-	uc_fw_override(uc_fw);
>-
>-	/* an empty path means the firmware is disabled */
>-	if (!xe_device_uc_enabled(xe) || !(*uc_fw->path)) {
>-		xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_DISABLED);
>-		drm_dbg(&xe->drm, "%s disabled", xe_uc_fw_type_repr(uc_fw->type));
>-		return 0;
>-	}
>-
>-	err = request_firmware(&fw, uc_fw->path, dev);
>-	if (err)
>-		goto fail;
>
> 	/* Check the size of the blob before examining buffer contents */
>-	if (unlikely(fw->size < sizeof(struct uc_css_header))) {
>+	if (unlikely(fw_size < sizeof(struct uc_css_header))) {
> 		drm_warn(&xe->drm, "%s firmware %s: invalid size: %zu < %zu\n",
> 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>-			 fw->size, sizeof(struct uc_css_header));
>-		err = -ENODATA;
>-		goto fail;
>+			 fw_size, sizeof(struct uc_css_header));
>+		return -ENODATA;
> 	}
>
>-	css = (struct uc_css_header *)fw->data;
>+	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 -
>@@ -403,9 +369,8 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 		drm_warn(&xe->drm,
> 			 "%s firmware %s: unexpected header size: %zu != %zu\n",
> 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>-			 fw->size, sizeof(struct uc_css_header));
>-		err = -EPROTO;
>-		goto fail;
>+			 fw_size, sizeof(struct uc_css_header));
>+		return -EPROTO;
> 	}
>
> 	/* uCode size must calculated from other sizes */
>@@ -417,12 +382,11 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 	/* At least, it should have header, uCode and RSA. Size of all three. */
> 	size = sizeof(struct uc_css_header) + uc_fw->ucode_size +
> 		uc_fw->rsa_size;
>-	if (unlikely(fw->size < size)) {
>+	if (unlikely(fw_size < size)) {
> 		drm_warn(&xe->drm, "%s firmware %s: invalid size: %zu < %zu\n",
> 			 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>-			 fw->size, size);
>-		err = -ENOEXEC;
>-		goto fail;
>+			 fw_size, size);
>+		return -ENOEXEC;
> 	}
>
> 	/* Get version numbers from the CSS header */
>@@ -433,6 +397,60 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 	uc_fw->patch_ver_found = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
> 					   css->sw_version);
>
>+	if (uc_fw->type == XE_UC_FW_TYPE_GUC)
>+		guc_read_css_info(uc_fw, css);
>+
>+	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);
>+}
>+
>+int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
>+{
>+	struct xe_device *xe = uc_fw_to_xe(uc_fw);
>+	struct xe_gt *gt = uc_fw_to_gt(uc_fw);
>+	struct xe_tile *tile = gt_to_tile(gt);
>+	struct device *dev = xe->drm.dev;
>+	const struct firmware *fw = NULL;
>+	struct xe_bo *obj;
>+	int err;
>+
>+	/*
>+	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
>+	 * before we're looked at the HW caps to see if we have uc support
>+	 */
>+	BUILD_BUG_ON(XE_UC_FIRMWARE_UNINITIALIZED);
>+	xe_assert(xe, !uc_fw->status);
>+	xe_assert(xe, !uc_fw->path);
>+
>+	uc_fw_auto_select(xe, uc_fw);
>+	xe_uc_fw_change_status(uc_fw, uc_fw->path ?
>+			       XE_UC_FIRMWARE_SELECTED :
>+			       XE_UC_FIRMWARE_NOT_SUPPORTED);
>+
>+	if (!xe_uc_fw_is_supported(uc_fw))
>+		return 0;
>+
>+	uc_fw_override(uc_fw);
>+
>+	/* an empty path means the firmware is disabled */
>+	if (!xe_device_uc_enabled(xe) || !(*uc_fw->path)) {
>+		xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_DISABLED);
>+		drm_dbg(&xe->drm, "%s disabled", xe_uc_fw_type_repr(uc_fw->type));
>+		return 0;
>+	}
>+
>+	err = request_firmware(&fw, uc_fw->path, dev);
>+	if (err)
>+		goto fail;
>+
>+	err = parse_headers(uc_fw, fw);
>+	if (err)
>+		goto fail;
>+
> 	drm_info(&xe->drm, "Using %s firmware from %s version %u.%u.%u\n",
> 		 xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
> 		 uc_fw->major_ver_found, uc_fw->minor_ver_found, uc_fw->patch_ver_found);
>@@ -441,9 +459,6 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 	if (err)
> 		goto fail;
>
>-	if (uc_fw->type == XE_UC_FW_TYPE_GUC)
>-		guc_read_css_info(uc_fw, css);
>-
> 	obj = xe_bo_create_from_data(xe, tile, fw->data, fw->size,
> 				     ttm_bo_type_kernel,
> 				     XE_BO_CREATE_VRAM_IF_DGFX(tile) |
>diff --git a/drivers/gpu/drm/xe/xe_uc_fw_abi.h b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>index 89e994ed4e00..edae7bb3cd72 100644
>--- a/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>+++ b/drivers/gpu/drm/xe/xe_uc_fw_abi.h
>@@ -10,9 +10,12 @@
> #include <linux/types.h>
>
> /**
>- * DOC: Firmware Layout
>+ * DOC: CSS-based Firmware Layout
>  *
>- * The GuC/HuC firmware layout looks like this::
>+ * The CSS-based firmware structure is used for GuC releases on all platforms
>+ * and for HuC releases up to DG1. Starting from DG2/MTL the HuC uses the GSC
>+ * layout instead.
>+ * The CSS firmware layout looks like this::
>  *
>  *      +======================================================================+
>  *      |  Firmware blob                                                       |
>-- 
>2.41.0
>

Re: [Intel-xe] [PATCH v2 5/5] drm/xe/huc: Define HuC for MTL

[Lucas De Marchi]

On Wed, Oct 18, 2023 at 03:57:07PM -0700, Daniele Ceraolo Spurio wrote:
>MTL uses a GSC-enabled binary. We expect all GSC-enabled binaries to be
>defined with a "_gsc", so we can check for that in the name to
>determine if the binary has the GSC headers or not.

this commit message is outdated now. Other than that,

	Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>
>v2: don't use the filename to identify the header type (Lucas)
>
>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_uc_fw.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
>index 2680b5082ea0..0fa1b3415eff 100644
>--- a/drivers/gpu/drm/xe/xe_uc_fw.c
>+++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>@@ -113,12 +113,13 @@ struct fw_blobs_by_type {
> 	fw_def(ROCKETLAKE,	major_ver(i915,	guc,	tgl,	70, 5))		\
> 	fw_def(TIGERLAKE,	major_ver(i915,	guc,	tgl,	70, 5))
>
>-#define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver)				\
>-	fw_def(DG1,		no_ver(i915,	huc,	dg1))			\
>-	fw_def(ALDERLAKE_P,	no_ver(i915,	huc,	tgl))			\
>-	fw_def(ALDERLAKE_S,	no_ver(i915,	huc,	tgl))			\
>-	fw_def(ROCKETLAKE,	no_ver(i915,	huc,	tgl))			\
>-	fw_def(TIGERLAKE,	no_ver(i915,	huc,	tgl))
>+#define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver)		\
>+	fw_def(METEORLAKE,	no_ver(i915,	huc_gsc,	mtl))		\
>+	fw_def(DG1,		no_ver(i915,	huc,		dg1))		\
>+	fw_def(ALDERLAKE_P,	no_ver(i915,	huc,		tgl))		\
>+	fw_def(ALDERLAKE_S,	no_ver(i915,	huc,		tgl))		\
>+	fw_def(ROCKETLAKE,	no_ver(i915,	huc,		tgl))		\
>+	fw_def(TIGERLAKE,	no_ver(i915,	huc,		tgl))
>
> #define MAKE_FW_PATH(dir__, uc__, shortname__, version__)			\
> 	__stringify(dir__) "/" __stringify(shortname__) "_" __stringify(uc__) version__ ".bin"
>-- 
>2.41.0
>

Re: [Intel-xe] [PATCH v2 2/5] drm/xe/huc: Extract version and binary offset from new HuC headers

[Daniele Ceraolo Spurio]

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@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>
>> ---
>> 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@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
>>