From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anatolij Gustschin <agust@denx.de>
Date: Mon, 4 Dec 2017 14:33:32 +0100
Subject: [U-Boot] [PATCH v3 5/6] binman: add ROM image signing for Bay
	Trail SoC
In-Reply-To: <CAEUhbmUJ7CNqPdZqxgxjMrBMYj__w6qPy9G-EnH_LQg7B0Scdg@mail.gmail.com>
References: <20171117011605.5916-1-agust@denx.de>
 <CAEUhbmUJ7CNqPdZqxgxjMrBMYj__w6qPy9G-EnH_LQg7B0Scdg@mail.gmail.com>
Message-ID: <20171204143332.4227c647@crub>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Bin,

On Mon, 4 Dec 2017 14:30:03 +0800
Bin Meng bmeng.cn at gmail.com wrote:
...
> > diff --git a/arch/x86/dts/u-boot.dtsi b/arch/x86/dts/u-boot.dtsi
> > index 7e37d4f394..98e2309108 100644
> > --- a/arch/x86/dts/u-boot.dtsi
> > +++ b/arch/x86/dts/u-boot.dtsi
> > @@ -15,6 +15,13 @@
> >                 sort-by-pos;
> >                 pad-byte = <0xff>;
> >                 size = <CONFIG_ROM_SIZE>;
> > +#ifdef CONFIG_BAYTRAIL_SECURE_BOOT  
> 
> This needs to be a generic macro like CONFIG_SECURE_BOOT as this
> affects all x86 rom images.

OK, will fix it.
 
> > +               sign;
> > +#ifdef CONFIG_SYS_SOC  
> 
> I believe CONFIG_SYS_SOC is defined by every board, so no need to do
> #ifdef here.

OK, I can drop it.

...
> > +OEM_PRIV_KEY_FILE_NAME = 'oemkey.pem'
> > +OEM_PUB_KEY_FILE_NAME = 'pub_oemkey.pem'
> > +OEM_PUBKEY_BIN_FILE_NAME = 'pub_oemkey.bin'
> > +OEM_PUBKEY_AND_SIG_FILE_NAME = 'oem_pub_sig.bin'  
> 
> This deserves a comment block on how there files are generated on the host.

OK.

...
> > +# FSP Stage2 size is 0x1f400. For debug FSP it is 0x2f400,
> > +# you must change it here wenn building with debug FSP image!  
> 
> typo: wenn -> when

OK, thanks.

> > +FSP_STAGE_2_SIZE = 0x1f400
> > +FSP_UPD_SIZE = 0xc00
> > +IBB_SIZE = 0x1fc00
> > +MANIFEST_SIZE = 0x400
> > +OEM_BLOCK_MAX_SIZE = 0x190
> > +U_BOOT_ROM_SIZE = 0x800000  
> 
> Can this file size be determined from the CONFIG_ROM_SIZE?
> 
> > +ROMFILE_SYS_TEXT_BASE = 0x00700000  
> 
> and calculate this by ourselves?

I cannot invest more time on this, I'm having many issues to add
coverage tests for this file, so I'll give it up. We are far beyond
the planned efforts for this topic and I cannot spend time to polish
this further, sorry.

Thanks,
Anatolij