On Wed, Oct 02, 2024 at 04:55:30PM -0600, Simon Glass wrote:
>
> Hi Brian,
>
> On Wed, 2 Oct 2024 at 00:41, Brian Ruley <brian.ru...@gehealthcare.com> wrote:
> >
> > On Mon, Sep 30, 2024 at 12:52:24PM -0600, Simon Glass wrote:
> > >
> > > WARNING: This email originated from outside of GE HealthCare. Please
> > > validate the sender's email address before clicking on links or
> > > attachments as they may not be safe.
> > >
> > > Hi Brian,
> > >
> > > On Mon, 30 Sept 2024 at 10:10, Brian Ruley <brian.ru...@gehealthcare.com>
> > > wrote:
> > > >
> > > > Conform to the style guide used in the project by making the following
> > > > changes:
> > > > * Use single quotes for multiline strings (except docstrings)
> > > > * Fix line width to 79 cols
> > > > * Use f-string instead of formatting a regular string
> > > >
> > > > Signed-off-by: Brian Ruley <brian.ru...@gehealthcare.com>
> > > > ---
> > > > tools/binman/etype/nxp_imx8mcst.py | 28 +++++++++++++++++++++-------
> > > > 1 file changed, 21 insertions(+), 7 deletions(-)
> > >
> > > Reviewed-by: Simon Glass <s...@chromium.org>
> > >
> > > Thanks for doing this.
> > >
> > > Some of my comments on the other patch could be applied here, if you
> > > prefer.
> > >
> > > >
> > > > diff --git a/tools/binman/etype/nxp_imx8mcst.py
> > > > b/tools/binman/etype/nxp_imx8mcst.py
> > > > index 8221517b0c..0c744a00d7 100644
> > > > --- a/tools/binman/etype/nxp_imx8mcst.py
> > > > +++ b/tools/binman/etype/nxp_imx8mcst.py
> > > > @@ -23,7 +23,7 @@ from u_boot_pylib import tools
> > > > MAGIC_NXP_IMX_IVT = 0x412000d1
> > > > MAGIC_FITIMAGE = 0xedfe0dd0
> > > >
> > > > -csf_config_template = """
> > > > +csf_config_template = '''
> > > > [Header]
> > > > Version = 4.3
> > > > Hash Algorithm = sha256
> > > > @@ -53,7 +53,7 @@ csf_config_template = """
> > > > [Authenticate Data]
> > > > Verification index = 2
> > > > Blocks = 0x1234 0x78 0xabcd "data.bin"
> > > > -"""
> > > > +'''
> > > >
> > > > class Entry_nxp_imx8mcst(Entry_mkimage):
> > > > """NXP i.MX8M CST .cfg file generator and cst invoker
> > > > @@ -69,9 +69,21 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
> > > > def ReadNode(self):
> > > > super().ReadNode()
> > > > self.loader_address = fdt_util.GetInt(self._node,
> > > > 'nxp,loader-address')
> > > > - self.srk_table = os.getenv('SRK_TABLE',
> > > > fdt_util.GetString(self._node, 'nxp,srk-table',
> > > > 'SRK_1_2_3_4_table.bin'))
> > > > - self.csf_crt = os.getenv('CSF_KEY',
> > > > fdt_util.GetString(self._node, 'nxp,csf-crt',
> > > > 'CSF1_1_sha256_4096_65537_v3_usr_crt.pem'))
> > > > - self.img_crt = os.getenv('IMG_KEY',
> > > > fdt_util.GetString(self._node, 'nxp,img-crt',
> > > > 'IMG1_1_sha256_4096_65537_v3_usr_crt.pem'))
> > > > + self.srk_table = os.getenv(
> > > > + 'SRK_TABLE', fdt_util.GetString(
> > > > + self._node, 'nxp,srk-table',
> > > > + 'SRK_1_2_3_4_table.bin'
> > > > + ))
> > > > + self.csf_crt = os.getenv(
> > > > + 'CSF_KEY', fdt_util.GetString(
> > > > + self._node, 'nxp,csf-crt',
> > > > + 'CSF1_1_sha256_4096_65537_v3_usr_crt.pem'
> > > > + ))
> > > > + self.img_crt = os.getenv(
> > > > + 'IMG_KEY', fdt_util.GetString(
> > > > + self._node, 'nxp,img-crt',
> > > > + 'IMG1_1_sha256_4096_65537_v3_usr_crt.pem'
> > > > + ))
> > > > self.unlock = fdt_util.GetBool(self._node, 'nxp,unlock')
> > > > self.ReadEntries()
> > > >
> > > > @@ -118,7 +130,7 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
> > > > tools.write_file(output_dname, data)
> > > >
> > > > # Generate CST configuration file used to sign payload
> > > > - cfg_fname = tools.get_output_filename('nxp.csf-config-txt.%s'
> > > > % uniq)
> > > > + cfg_fname =
> > > > tools.get_output_filename(f'nxp.csf-config-txt.{uniq}')
> > > > config = configparser.ConfigParser()
> > > > # Do not make key names lowercase
> > > > config.optionxform = str
> > > > @@ -127,7 +139,9 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
> > > > config['Install SRK']['File'] = '"' + self.srk_table + '"'
> > > > config['Install CSFK']['File'] = '"' + self.csf_crt + '"'
> > > > config['Install Key']['File'] = '"' + self.img_crt + '"'
> > > > - config['Authenticate Data']['Blocks'] = hex(signbase) + ' 0 '
> > > > + hex(len(data)) + ' "' + str(output_dname) + '"'
> > > > + config['Authenticate Data']['Blocks'] = (hex(signbase) + ' 0 '
> > > > + + hex(len(data)) + '
> > > > "'
> > > > + + str(output_dname) +
> > > > '"')
> > > > if not self.unlock:
> > > > config.remove_section('Unlock')
> > > > with open(cfg_fname, 'w') as cfgf:
> > > > --
> > > > 2.39.5
> > > >
> > >
> > > Regards,
> > > Simon
> >
> > Hi Simon,
> >
> > Sure thing, I'm glad to contribute. I'm quite new to upstreaming so this
> > is a good learning experience for me, and I thank you for your patience.
>
> Well you seem to have figured it out :-)
>
> >
> > I didn't have time to respond yesterday, but I sent a new revision
> > based on your comments.
> >
> > Only one I didn't quite get was
> >
> > > All three options seem to read the 'nxp,srk-crt' property, so you can
> > > do that once the if() to reduce the amount of duplicated code.
> >
> > Each is reading a different property "img-crt", "csf-crt", and
> > "srk-crt", with the latter read only if "fast-auth" is set.
>
> Yes I see that now.
>
> >
> > As for the testing part, after I got `binman test` working, I
> > experimented with creating a test for the `nxp-imx8mcst` etype, using
> > what you had done as a starting point. FYI, it doesn't depend on the
> > `nxp-imx8mimage` because it's for invoking `mkimage` when building
> > SPL, for example. So, what I did is just create a fake FIT image
> > inside the nxp-imx8mcst node, and run the test. I can send the patch
> > if you wish.
> >
> > The test, however, was unsuccessful because a PKI tree is needed to
> > sign the assets. I thought maybe you would have a comment on that?
>
> Could you use a test file? There is another etype which has test
> yaml-files in tools/binman/test/yaml/
>
> There is also tools/binman/test/key.key , I think for the vblock etype.
>
> Regards,
> Simon
Hi Simon,
I got the test working.
> Could you use a test file? There is another etype which has test
> yaml-files in tools/binman/test/yaml/
Like a test FIT image or what? The image has to be either a FIT or
mkimage.
> There is also tools/binman/test/key.key , I think for the vblock etype.
I tried using it but didn't get it to work (tbh, I didn't try that many
times). Regardless, it's just easier to create keys of the type CST
expects, and include them in the test. I'll send the patch, so you can
take a look.
Also, you haven't responded to the v4 patch I sent -- is it good? Just
want to make sure if there's something I need to do still regarding
that.
Best,
Brian
