[v2 2/6] arm: socfpga: soc64: Support Vendor Authorized Boot (VAB)

[Lim, Elly Siew Chin]

Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg at chromium.org>
> Sent: Friday, January 8, 2021 11:24 AM
> To: Lim, Elly Siew Chin <elly.siew.chin.lim at intel.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Marek Vasut
> <marex at denx.de>; Tan, Ley Foon <ley.foon.tan at intel.com>; See, Chin Liang
> <chin.liang.see at intel.com>; Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com>; Chee, Tien Fong
> <tien.fong.chee at intel.com>; Westergreen, Dalon
> <dalon.westergreen at intel.com>; Gan, Yau Wai <yau.wai.gan at intel.com>
> Subject: Re: [v2 2/6] arm: socfpga: soc64: Support Vendor Authorized Boot
> (VAB)
> 
> Hi,
> 
> On Thu, 7 Jan 2021 at 18:11, Lim, Elly Siew Chin <elly.siew.chin.lim at intel.com>
> wrote:
> >
> > Hi Simon,
> >
> > > -----Original Message-----
> > > From: Simon Glass <sjg at chromium.org>
> > > Sent: Thursday, January 7, 2021 8:36 PM
> > > To: Lim, Elly Siew Chin <elly.siew.chin.lim at intel.com>
> > > Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Marek Vasut
> > > <marex at denx.de>; Tan, Ley Foon <ley.foon.tan at intel.com>; See, Chin
> > > Liang <chin.liang.see at intel.com>; Simon Goldschmidt
> > > <simon.k.r.goldschmidt at gmail.com>; Chee, Tien Fong
> > > <tien.fong.chee at intel.com>; Westergreen, Dalon
> > > <dalon.westergreen at intel.com>; Gan, Yau Wai <yau.wai.gan at intel.com>
> > > Subject: Re: [v2 2/6] arm: socfpga: soc64: Support Vendor Authorized
> > > Boot
> > > (VAB)
> > >
> > > On Thu, 7 Jan 2021 at 03:03, Siew Chin Lim
> > > <elly.siew.chin.lim at intel.com>
> > > wrote:
> > > >
> > > > Vendor Authorized Boot is a security feature for authenticating
> > > > the images such as U-Boot, ARM trusted Firmware, Linux kernel,
> > > > device tree blob and etc loaded from FIT. After those images are
> > > > loaded from FIT, the VAB certificate and signature block appended
> > > > at the end of each image are sent to Secure Device Manager (SDM) for
> authentication.
> > > > U-Boot will validate the SHA384 of the image against the SHA384
> > > > hash stored in the VAB certificate before sending the image to SDM
> > > > for authentication.
> > > >
> > > > Signed-off-by: Siew Chin Lim <elly.siew.chin.lim at intel.com>
> > > >
> > > > ---
> > > > v2
> > > > ---
> > > > - Renamed SECURE_VAB_AUTH* to SOCFPGA_SECURE_VAB_AUTH*
> > > > - Changes in secure_vab.c
> > > >   - Changed to use SZ_1K for 1024
> > > >   - Updated comment in secure_vab.c of "... the certificate for T"
> > > >   - The code will report error before end of the function if reach
> > > >     maximum retry.
> > > >   - In board_prep_linux function, only execute linux_qspi_enable
> > > >     command if it exists in enviroment variable. It is optional.
> > > > ---
> > > >  arch/arm/mach-socfpga/Kconfig                    |  15 ++
> > > >  arch/arm/mach-socfpga/Makefile                   |   2 +
> > > >  arch/arm/mach-socfpga/include/mach/mailbox_s10.h |   1 +
> > > >  arch/arm/mach-socfpga/include/mach/secure_vab.h  |  63 ++++++++
> > > >  arch/arm/mach-socfpga/secure_vab.c               | 193
> > > +++++++++++++++++++++++
> > > >  common/Kconfig.boot                              |   2 +-
> > > >  6 files changed, 275 insertions(+), 1 deletion(-)  create mode
> > > > 100644 arch/arm/mach-socfpga/include/mach/secure_vab.h
> > > >  create mode 100644 arch/arm/mach-socfpga/secure_vab.c
> > >
> > > I think this could use a few more function comments. Also try to use
> > > if() instead of #if
> >
> > Noted. I will change #if in *.c to " if (IS_ENABLED(CONFIG...))".
> >
> > Besides, can you please help to elaborate more about  "I think this could use a
> few more function comments."?

Noted, I will review this. Thanks.
> 
> Non-trivial functions should have a comment in the standard style that explains
> their purpose/action and documents input and output args and return value.
> 
> Regards,
> Simon