On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote:
alan: snip
> +int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, size_t size)
> +{
alan:snip
> + /*
> + * The GSC binary starts with the pointer layout, which contains the
> + * locations of the various partitions of the binary. The one we're
> + * interested in to get the version 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. From here, we can parse the CPD
alan: nit: could we add the meaning of 'RBE', probably not here but in the header file where GSC_RBE is defined?
> + * header and the following entries to find the manifest location
> + * (entry identified by the "RBEP.man" name), from which we can finally
> + * extract the version.
> + *
> + * --------------------------------------------------
> + * [ intel_gsc_layout_pointers ]
> + * [ ... ]
> + * [ boot1_offset >---------------------------]------o
> + * [ ... ] |
> + * -------------------------------------------------- |
> + * |
> + * -------------------------------------------------- |
> + * [ intel_gsc_bpdt_header ]<-----o
> + * --------------------------------------------------
alan: special thanks for the diagram - love these! :)
alan: snip
> + min_size = layout->boot1_offset + layout->boot1_offset > size;
alan: latter is a binary so + 1? or is this a typo and supposed to be:
min_size = layout->boot1_offset;
actually since we are accessing a bpdt_header hanging off that offset, it should rather be:
min_size = layout->boot1_offset + sizeof(*bpdt_header);
> + if (size < min_size) {
> + gt_err(gt, "GSC FW too small for boot section! %zu < %zu\n",
> + size, min_size);
> + return -ENODATA;
> + }
> +
> + bpdt_header = data + layout->boot1_offset;
> + if (bpdt_header->signature != INTEL_GSC_BPDT_HEADER_SIGNATURE) {
> + gt_err(gt, "invalid signature for meu BPDT header: 0x%08x!\n",
> + bpdt_header->signature);
> + return -EINVAL;
> + }
> +
alan: IIUC, to be strict about the size-crawl-checking, we should check minsize
again - this time with the additional "bpdt_header->descriptor_count * sizeof(*bpdt_entry)".
(hope i got that right?) - adding that check before parsing through the (bpdt_entry++)'s ->type
> + bpdt_entry = (void *)bpdt_header + sizeof(*bpdt_header);
> + for (i = 0; i < bpdt_header->descriptor_count; i++, bpdt_entry++) {
> + if ((bpdt_entry->type & INTEL_GSC_BPDT_ENTRY_TYPE_MASK) !=
> + INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE)
> + continue;
> +
> + cpd_header = (void *)bpdt_header + bpdt_entry->sub_partition_offset;
> + break;
> + }
> +
> + if (!cpd_header) {
> + gt_err(gt, "couldn't find CPD header in GSC binary!\n");
> + return -ENODATA;
> + }
alan: same as above, so for size-crawl-checking, we should check minsize again with the addition of cpd_header, no?
> +
> + if (cpd_header->header_marker != INTEL_GSC_CPD_HEADER_MARKER) {
> + gt_err(gt, "invalid marker for meu CPD header in GSC bin: 0x%08x!\n",
> + cpd_header->header_marker);
> + return -EINVAL;
> + }
alan: and again here, the size crawl checking with cpd_header->num_of_entries * *cpd_entry
> + cpd_entry = (void *)cpd_header + cpd_header->header_length;
> + for (i = 0; i < cpd_header->num_of_entries; i++, cpd_entry++) {
> + if (strcmp(cpd_entry->name, "RBEP.man") == 0) {
> + manifest = (void *)cpd_header + cpd_entry_offset(cpd_entry);
> + intel_uc_fw_version_from_meu_manifest(&gsc->release,
> + manifest);
> + gsc->security_version = manifest->security_version;
> + break;
> + }
> + }
> +
> + return 0;
alan:snip
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> index fff8928218df..8d7b9e4f1ffc 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
alan:snip
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h
> index d55a66202576..8bce2b8aed84 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h
alan:snip
> +struct intel_gsc_layout_pointers {
> + u8 rom_bypass_vector[16];
alan:snip...
> + u32 temp_pages_offset;
> + u32 temp_pages_size;
> +} __packed;
alan: structure layout seems unnecessarily repetitive... why not ->
struct partition_info {
u32 offset;
u32 size;
};
struct intel_gsc_layout_pointers {
u8 rom_bypass_vector[16];
...
struct partition_info datap;
struct partition_info bootregion[5];
struct partition_info trace;
}__packed;
> +
alan: we should put the 'bpdt' acronym meaning and if its an intel specific
name, then a bit of additional comment explaining what it means.
> +struct intel_gsc_bpdt_header {
> + u32 signature;
> +#define INTEL_GSC_BPDT_HEADER_SIGNATURE 0x000055AA
> +
> + u16 descriptor_count; /* bum of entries after the header */
alan:s/bum/num
> +
> + u8 version;
> + u8 configuration;
> +
> + u32 crc32;
> +
> + u32 build_version;
> + struct intel_gsc_meu_version tool_version;
alan: nit: "struct intel_gsc_meu_version meu_version" is better no?
> +} __packed;
> +
> +
> +struct intel_gsc_bpdt_entry {
> + /*
> + * Bits 0-15: BPDT entry type
> + * Bits 16-17: reserved
> + * Bit 18: code sub-partition
> + * Bits 19-31: reserved
> + */
alan: nit: i think its neater to just put above comments next to the #defines on the lines following 'type' and
create a genmask for code-sub-partition (if we use it, else skip it?) - the benefit being a little less clutter
> + u32 type;
> +#define INTEL_GSC_BPDT_ENTRY_TYPE_MASK GENMASK(15,0)
> +#define INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE 0x1
> +
> + u32 sub_partition_offset; /* from the base of the BPDT header */
> + u32 sub_partition_size;
> +} __packed;
> +
alan:snip
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> @@ -17,6 +17,9 @@ struct intel_gsc_uc {
> struct intel_uc_fw fw;
>
> /* GSC-specific additions */
> + struct intel_uc_fw_ver release;
> + u32 security_version;
alan: for consistency and less redundancy, can't we add "security_version"
into 'struct intel_uc_fw_ver' (which is zero for firmware that doesn't
have it). That way, intel_gsc_uc can re-use intel_uc_fw.file_selected
just like huc?
alan:snip
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> index 0a0bd5504057..0c01d48b1dd9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
alan:snip
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 796f54a62eef..cd8fc194f7fa 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
alan:snip
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> index 8f2306627332..279244744d43 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
alan:snip
