On Mon, Sep 14, 2020 at 11:18:25PM -0700, Thirupathaiah Annapureddy wrote:
> Hi Simon,
>
> Thanks for the review.
>
> On 9/6/2020 6:43 PM, Simon Glass wrote:
> >>
> >> diff --git a/Kconfig b/Kconfig
> >> index 883e3f71d0..3959a6592c 100644
> >> --- a/Kconfig
> >> +++ b/Kconfig
> >> @@ -533,6 +533,15 @@ config FIT_CIPHER
> >> Enable the feature of data ciphering/unciphering in the tool
> >> mkimage
> >> and in the u-boot support of the FIT image.
> >>
> >> +config FIT_ARBP
> >
> > How about using ROLLBACK instead of ARBP. It is easier to understand.Looks
> > good to me. I will change it in the next version of the patch.
>
> >> +{
> >> + uint8_t type;
> >> + uint32_t image_arbvn;
> >> + uint32_t plat_arbvn = 0;
> >
> > Those three can be uint.
> fit_image_get_type() returns type as uint8_t.
> I can change it for the other two variables.
>
> >> static int fit_config_verify_sig(const void *fit, int conf_noffset,
> >> const void *sig_blob, int sig_offset)
> >> {
> >> @@ -401,6 +472,14 @@ static int fit_config_verify_sig(const void *fit, int
> >> conf_noffset,
> >> goto error;
> >> }
> >>
> >> +#if !defined(USE_HOSTCC)
> >
> > Do we need this £ifdef, or can we rely on IMAGE_ENABLE_ARBP?
> I believe we can rely on just IMAGE_ENABLE_ARBP.
>
> >> #define FIT_LOAD_PROP "load"
> >> +#define FIT_ARBVN_PROP "arbvn"
> >
> > ROLLBACK / "rollback"
> I will fix it in the next version.
>
> >
> >>
> >> /* configuration node */
> >> #define FIT_KERNEL_PROP "kernel"
> >> @@ -1085,6 +1086,7 @@ int fit_image_get_data_size_unciphered(const void
> >> *fit, int noffset,
> >> size_t *data_size);
> >> int fit_image_get_data_and_size(const void *fit, int noffset,
> >> const void **data, size_t *size);
> >> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn);
> >
> > Please add a full function comment
> comment was added before the function definition to be consistent
> with other functions.
>
> >> +int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
> >
> > This needs a driver since the rollback counter may be implemented by a
> > TPM or anything.
> Board specific hooks can leverage TPM library functions in that case.
> May I know why a driver is needed?Sorry for not getting in to this series sooner. One thing that I think would be very helpful is to see is a full demonstration on say a Raspberry Pi. I know I have a TPM2 module that supports Pi sitting around here. I assume you've also tested this on some HW platform. -- Tom
signature.asc
Description: PGP signature
