Re: [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary
On Fri, 2023-05-26 at 18:27 -0700, Ceraolo Spurio, Daniele wrote:
>
> > > 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
> > 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;
> >
>
> I've just realized that I didn't reply to this comment. The specs have
> the structure defined that way, so I tried to keep a 1:1 match like we
> usually do. I think switching to a partition_info structure is ok, but
> I'll avoid the array because the GSC partition are 1-based, which could
> cause confusion (i.e. partition boot1 would be bootregion[0]).
alan: sure - that's fine - let's stick to align with the spec definitions
Re: [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary
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;
I've just realized that I didn't reply to this comment. The specs have
the structure defined that way, so I tried to keep a 1:1 match like we
usually do. I think switching to a partition_info structure is ok, but
I'll avoid the array because the GSC partition are 1-based, which could
cause confusion (i.e. partition boot1 would be bootregion[0]).
Daniele
Re: [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary
On Thu, 2023-05-25 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> On 5/24/2023 10:14 PM, Teres Alexis, Alan Previn wrote:
> > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote:
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?
>
> I'm not sure what you mean by re-using intel_uc_fw.file_selected. Is
> that for the call from intel_uc_fw_version_from_meu_manifest? I'm
> purposely not doing that. Note that the GSC has 3 versions:
>
> Release version (incremented with each build and encoded in the header)
> Security version (also encoded in the header)
> Compatibility version (queried via message to the GSC)
>
> The one we care about for communicating with the GSC is the last one, so
> that's the one I stored in intel_uc_fw.file_selected (in the next
> patch). The other 2 versions are not strictly required to use the GSC
> and we only fetch them for debug purposes, so if something goes wrong we
> know exactly what we've loaded.
>
> Daniele
alan: okay thanks - seeing that now in the next patch... (and i also forgot that
the GSC release version doesnt reflect interface versioning in anyway like GuC
does).
In that case, above additional versions are fine. Would definitely love to see
additional comments under "GSC-specific-additions" that explain those 3
versioning
items and what we care about as how you have explained here.
Re: [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary
On 5/24/2023 10:14 PM, Teres Alexis, Alan Previn wrote:
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?
I have no idea what RBE means, the specs just says to look for the
section named that way. I didn't dig because we don't really care about
its name, just that the CPD header is in there.
+* 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;
it's a cut & paste typo, it should be
min_size = layout->boot1_offset + layout->boot1_size;
thanks for spotting it.
actually since we are accessing a bpdt_header hanging off that offset, it
should rather be:
min_size = layout->boot1_offset + sizeof(*bpdt_header);
I can add a check to make sure that boot1_size >= 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
will do (same for the comments below)
+ 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(>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
Re: [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary
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(>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
[PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary
The release and security versions of the GSC binary are not used at
runtime to decide interface compatibility (there is a separate version
for that), but they're still useful for debug, so it is still worth
extracting them and printing them out in dmesg.
To get to these version, we need to navigate through various headers in
the binary. See in-code comment for details.
Signed-off-by: Daniele Ceraolo Spurio
Cc: Alan Previn
---
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 130 +-
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 2 +
.../drm/i915/gt/uc/intel_gsc_meu_headers.h| 83 ++-
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 3 +
drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 13 +-
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 30 ++--
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 3 +
7 files changed, 237 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
index 4814901fac9e..df53c13e99a3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -10,6 +10,7 @@
#include "gt/intel_gt_print.h"
#include "gt/intel_ring.h"
#include "intel_gsc_fw.h"
+#include "intel_gsc_meu_headers.h"
#define GSC_FW_STATUS_REG _MMIO(0x116C40)
#define GSC_FW_CURRENT_STATE REG_GENMASK(3, 0)
@@ -42,6 +43,129 @@ bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
return fw_status & GSC_FW_INIT_COMPLETE_BIT;
}
+static inline u32 cpd_entry_offset(const struct intel_gsc_cpd_entry *entry)
+{
+ return entry->offset & INTEL_GSC_CPD_ENTRY_OFFSET_MASK;
+}
+
+int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data,
size_t size)
+{
+ struct intel_gsc_uc *gsc = container_of(gsc_fw, struct intel_gsc_uc,
fw);
+ struct intel_gt *gt = gsc_uc_to_gt(gsc);
+ const struct intel_gsc_layout_pointers *layout = data;
+ const struct intel_gsc_bpdt_header *bpdt_header = NULL;
+ const struct intel_gsc_bpdt_entry *bpdt_entry = NULL;
+ const struct intel_gsc_cpd_header_v2 *cpd_header = NULL;
+ const struct intel_gsc_cpd_entry *cpd_entry = NULL;
+ const struct intel_gsc_manifest_header *manifest;
+ size_t min_size = sizeof(*layout);
+ int i;
+
+ if (size < min_size) {
+ gt_err(gt, "GSC FW too small! %zu < %zu\n", size, min_size);
+ return -ENODATA;
+ }
+
+ /*
+* 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
+* 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
+* --
+* [ intel_gsc_bpdt_entry[]]
+* [ entry1]
+* [ ... ]
+* [ entryX]
+* [ type == GSC_RBE ]
+* [ offset >-]--o
+* [ ... ] |
+* -- |
+* |
+* -- |
+* [ intel_gsc_cpd_header_v2 ]<-o
+* --
+* [ intel_gsc_cpd_entry[] ]
+* [ entry1]
+* [ ... ]
+* [ entryX]
+* [ "RBEP.man"]
+* [ ... ]
+* [ offset >]--o
+
