[PATCH i-g-t v2 3/6] tools/intel-gfx-fw-info: Embed fw decoding

[Lucas De Marchi]

On Wed, Aug 21, 2024 at 02:06:30PM GMT, Gustavo Sousa wrote:
>Quoting Lucas De Marchi (2024-08-20 20:29:25-03:00)
>>Use a factory class method to decide what class to use depending on the
>>magic and embed the calls to cstruct in each of them. Main motivation is
>>that in future the decode method will need to seek the binary as it
>>can't really know the version just checking the beginning of the file.
>>
>>For now, keep the open/close of the file in the main function, but
>>eventually can be migrated as well.
>>
>>Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>---
>> tools/intel-gfx-fw-info | 73 ++++++++++++++++++++++++++++-------------
>> 1 file changed, 51 insertions(+), 22 deletions(-)
>>
>>diff --git a/tools/intel-gfx-fw-info b/tools/intel-gfx-fw-info
>>index 3105144b7..0f1da8bff 100755
>>--- a/tools/intel-gfx-fw-info
>>+++ b/tools/intel-gfx-fw-info
>>@@ -105,8 +105,38 @@ def FIELD_GET(mask: int, value: int) -> int:
>>
>>
>> class Fw(abc.ABC):
>>-    def __init__(self, fw):
>>-        self.fw = fw
>>+    def __init__(self, cparser, f):
>>+        self.f = f
>>+        self.cparser = cparser
>>+        self.fw = None
>>+
>>+    def checksum(self):
>>+        self.f.seek(0, 0)
>>+        return hashlib.sha256(self.f.read()).hexdigest()
>>+
>>+    def dump(self, **kw):
>>+        cstruct.dumpstruct(self.fw, kw)
>
>s/kw/**kw/
>
>>+
>>+    @classmethod
>>+    def create(cls, f):
>>+        cparser = cstruct.cstruct()
>>+        cparser.load(CDEF)
>>+
>>+        magic = cparser.magic(f).data
>>+        default = None
>>+
>>+        for s in cls.__subclasses__():
>>+            if s.MAGIC is None:
>>+                default = s
>>+            elif s.MAGIC == magic:
>>+                f.seek(0, 0)
>>+                return s(cparser, f)
>>+
>>+        if default:
>>+            f.seek(0, 0)
>
>Is this seek() necessary? Looks like functions that use f already rewind
>before using f.

yeah... I went back and forth between leaving it inside or outside.
When we add support for GSC we may want it outside since GSC basically
adds some headers before the  gsc_cpd_header_v2 that is used by HuC.

I will remove the seek() from here for now.

>
>>+            return default(cparser, f)
>
>I believe a cleaner way of doing this is to have a single "constructor"
>variable so that we have a single point of instantiation. Something
>like:
>
>    constructor = None
>
>    for s in cls.__subclasses__():
>        if s.MAGIC is None:
>            constructor = s
>        elif s.MAGIC == magic:
>            constructor = s
>            break
>
>    if constructor:
>        f.seek(0, 0)
>        return constructor(cparser, f)

yeah, better. I will update the patch with that.

>
>>+
>>+        return None
>>
>>     @abc.abstractmethod
>>     def decode(self):
>>@@ -114,8 +144,13 @@ class Fw(abc.ABC):
>>
>>
>> class FwCss(Fw):
>>+    MAGIC = None
>>+
>>     def decode(self):
>>-        data = []
>>+        self.f.seek(0, 0)
>>+        self.fw = self.cparser.uc_css_header(self.f)
>>+
>>+        data = ["header-type: CSS"]
>>
>>         CSS_SW_VERSION_UC_MAJOR = 0xFF << 16
>>         CSS_SW_VERSION_UC_MINOR = 0xFF << 8
>>@@ -137,8 +172,13 @@ class FwCss(Fw):
>>
>>
>> class FwGsc(Fw):
>>+    MAGIC = b"$CPD"
>>+
>>     def decode(self):
>>-        data = []
>>+        self.f.seek(0, 0)
>>+        self.fw = self.cparser.uc_huc_gsc_header(self.f)
>>+
>>+        data = ["header-type: GSC"]
>>
>>         HUC_GSC_MINOR_VER_HI_MASK = 0xFF << 16
>>         HUC_GSC_MAJOR_VER_HI_MASK = 0xFF
>>@@ -167,40 +207,29 @@ def parse_args(argv: typing.List[str]) -> argparse.Namespace:
>>     return parser.parse_args(argv)
>>
>>
>>-def calculate_checksum(f: typing.BinaryIO) -> str:
>>-    return hashlib.sha256(f.read()).hexdigest()
>>-
>>-
>> def main(argv: typing.List[str]) -> int:
>>     args = parse_args(argv)
>>
>>-    cparser = cstruct.cstruct()
>>-    cparser.load(CDEF)
>>-
>>     checksum = None
>>
>>     try:
>>         with open(args.filename, mode="rb") as f:
>>-            magic = cparser.magic(f)
>>-            f.seek(0, 0)
>>-            if magic.data == b"$CPD":
>>-                fw = FwGsc(cparser.uc_huc_gsc_header(f))
>>-            else:
>>-                fw = FwCss(cparser.uc_css_header(f))
>>+            fw = Fw.create(f)
>>+            if not fw:
>>+                logging.fatal("Unknown firmware type in '{args.filename}'")
>
>Note that this doesn't cause the script to abort...

there is a sys.exit(1) squashed in the wrong patch (5). I will bring it
here.

Thanks
Lucas De Marchi

>
>>
>>+            decoded_data = fw.decode()
>
>...meaning that we crash here if fw is None.
>
>--
>Gustavo Sousa
>
>>             if args.checksum:
>>-                f.seek(0, 0)
>>-                checksum = calculate_checksum(f)
>>-
>>+                checksum = fw.checksum()
>>     except FileNotFoundError as e:
>>         logging.fatal(e)
>>         return 1
>>
>>-    print(*fw.decode(), sep="\n")
>>+    print(*decoded_data, sep="\n")
>>
>>     if args.raw:
>>         print("raw dump:", end="")
>>-        cstruct.dumpstruct(fw.fw, color=sys.stdout.isatty())
>>+        fw.dump(color=sys.stdout.isatty())
>>
>>     if checksum:
>>         print(f"checksum: {checksum}")
>>--
>>2.43.0
>>