[PATCH 0/6] corstone1000: fwu metadata and GPT

[PATCH 0/6] corstone1000: fwu metadata and GPT

[Rui Miguel Silva]

Now that the nvmxip block driver is merged we can add on top
of it the platform code to use GPT and FWU metadata in the
Corstone1000.

But first, push 2 fixes that are needed to make all this work:
 - move nvmxip header to include
 - setup fwu metadata structures as packed (we have a 32bit
   writer - Secure enclave Cortex-M0 and a 64bit reader host
   Cortex-A35)

Cheers,
     Rui

Rui Miguel Silva (6):
  fwu_metadata: make sure structures are packed
  nvmxip: move header to include
  corstone1000: add fwu-metadata store info
  corstone1000: add boot index
  corstone1000: set kernel_addr based on boot_idx
  corstone1000: add nvmxip, fwu-mdata and gpt options

 arch/arm/dts/corstone1000.dtsi             |  7 ++-
 board/armltd/corstone1000/corstone1000.c   | 73 +++++++++++++++++++++-
 board/armltd/corstone1000/corstone1000.env | 10 +--
 configs/corstone1000_defconfig             | 14 ++++-
 include/fwu_mdata.h                        |  6 +-
 {drivers/mtd/nvmxip => include}/nvmxip.h   |  0
 6 files changed, 95 insertions(+), 15 deletions(-)
 rename {drivers/mtd/nvmxip => include}/nvmxip.h (100%)

-- 
2.40.0

[PATCH 1/6] fwu_metadata: make sure structures are packed

[Rui Miguel Silva]

The fwu metadata in the metadata partitions
should/are packed to guarantee that the info is
correct in all platforms. Also the size of them
are used to calculate the crc32 and that is important
to get it right.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 include/fwu_mdata.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
index 8fda4f4ac225..c61221a91735 100644
--- a/include/fwu_mdata.h
+++ b/include/fwu_mdata.h
@@ -22,7 +22,7 @@ struct fwu_image_bank_info {
 	efi_guid_t  image_uuid;
 	uint32_t accepted;
 	uint32_t reserved;
-};
+} __packed;
 
 /**
  * struct fwu_image_entry - information for a particular type of image
@@ -38,7 +38,7 @@ struct fwu_image_entry {
 	efi_guid_t image_type_uuid;
 	efi_guid_t location_uuid;
 	struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
-};
+} __packed;
 
 /**
  * struct fwu_mdata - FWU metadata structure for multi-bank updates
@@ -62,6 +62,6 @@ struct fwu_mdata {
 	uint32_t previous_active_index;
 
 	struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
-};
+} __packed;
 
 #endif /* _FWU_MDATA_H_ */
-- 
2.40.0

Re: [PATCH 1/6] fwu_metadata: make sure structures are packed

[Ilias Apalodimas]

On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:

Hi Rui,
>
> The fwu metadata in the metadata partitions
> should/are packed to guarantee that the info is
> correct in all platforms. Also the size of them
> are used to calculate the crc32 and that is important
> to get it right.
>
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> ---
>  include/fwu_mdata.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
> index 8fda4f4ac225..c61221a91735 100644
> --- a/include/fwu_mdata.h
> +++ b/include/fwu_mdata.h
> @@ -22,7 +22,7 @@ struct fwu_image_bank_info {
>         efi_guid_t  image_uuid;
>         uint32_t accepted;
>         uint32_t reserved;
> -};
> +} __packed;
>
>  /**
>   * struct fwu_image_entry - information for a particular type of image
> @@ -38,7 +38,7 @@ struct fwu_image_entry {
>         efi_guid_t image_type_uuid;
>         efi_guid_t location_uuid;
>         struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> -};
> +} __packed;
>
>  /**
>   * struct fwu_mdata - FWU metadata structure for multi-bank updates
> @@ -62,6 +62,6 @@ struct fwu_mdata {
>         uint32_t previous_active_index;
>
>         struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> -};
> +} __packed;
>
>  #endif /* _FWU_MDATA_H_ */
> --
> 2.40.0
>

Yep, that's a good catch.  The spec defines 'just' the offsets and not
the C representation, so those should be indeed packed.

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/6] fwu_metadata: make sure structures are packed

[Heinrich Schuchardt]

On 5/3/23 10:06, Ilias Apalodimas wrote:
> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:
>
> Hi Rui,
>>
>> The fwu metadata in the metadata partitions
>> should/are packed to guarantee that the info is
>> correct in all platforms. Also the size of them
>> are used to calculate the crc32 and that is important
>> to get it right.
>>
>> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
>> ---
>>   include/fwu_mdata.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
>> index 8fda4f4ac225..c61221a91735 100644
>> --- a/include/fwu_mdata.h
>> +++ b/include/fwu_mdata.h
>> @@ -22,7 +22,7 @@ struct fwu_image_bank_info {
>>          efi_guid_t  image_uuid;
>>          uint32_t accepted;
>>          uint32_t reserved;
>> -};
>> +} __packed;

This structure is naturally packed. Why should adding a __packed
attribute make a difference?

>>
>>   /**
>>    * struct fwu_image_entry - information for a particular type of image
>> @@ -38,7 +38,7 @@ struct fwu_image_entry {
>>          efi_guid_t image_type_uuid;
>>          efi_guid_t location_uuid;
>>          struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
>> -};
>> +} __packed;

ditto

>>
>>   /**
>>    * struct fwu_mdata - FWU metadata structure for multi-bank updates
>> @@ -62,6 +62,6 @@ struct fwu_mdata {
>>          uint32_t previous_active_index;
>>
>>          struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
>> -};
>> +} __packed;

Depending on the alignment of efi_guid_t __packed might make a
difference here. But as efi_guid_t is defined as 4 aligned I can't see
an impact here either.

Best regards

Heinrich

>>
>>   #endif /* _FWU_MDATA_H_ */
>> --
>> 2.40.0
>>
>
> Yep, that's a good catch.  The spec defines 'just' the offsets and not
> the C representation, so those should be indeed packed.
>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/6] fwu_metadata: make sure structures are packed

[Rui Miguel Silva]

Hi Heinrich,
thanks for the review,
Heinrich Schuchardt <xypron.glpk@gmx.de> writes:

> On 5/3/23 10:06, Ilias Apalodimas wrote:
>> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:
>>
>> Hi Rui,
>>>
>>> The fwu metadata in the metadata partitions
>>> should/are packed to guarantee that the info is
>>> correct in all platforms. Also the size of them
>>> are used to calculate the crc32 and that is important
>>> to get it right.
>>>
>>> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
>>> ---
>>>   include/fwu_mdata.h | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
>>> index 8fda4f4ac225..c61221a91735 100644
>>> --- a/include/fwu_mdata.h
>>> +++ b/include/fwu_mdata.h
>>> @@ -22,7 +22,7 @@ struct fwu_image_bank_info {
>>>          efi_guid_t  image_uuid;
>>>          uint32_t accepted;
>>>          uint32_t reserved;
>>> -};
>>> +} __packed;
>
> This structure is naturally packed. Why should adding a __packed
> attribute make a difference?

Agree.

>
>>>
>>>   /**
>>>    * struct fwu_image_entry - information for a particular type of image
>>> @@ -38,7 +38,7 @@ struct fwu_image_entry {
>>>          efi_guid_t image_type_uuid;
>>>          efi_guid_t location_uuid;
>>>          struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
>>> -};
>>> +} __packed;
>
> ditto

Agree.

>
>>>
>>>   /**
>>>    * struct fwu_mdata - FWU metadata structure for multi-bank updates
>>> @@ -62,6 +62,6 @@ struct fwu_mdata {
>>>          uint32_t previous_active_index;
>>>
>>>          struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
>>> -};
>>> +} __packed;
>
> Depending on the alignment of efi_guid_t __packed might make a
> difference here. But as efi_guid_t is defined as 4 aligned I can't see
> an impact here either.

but efi_guid_t is defined as 8 aligned, right?
Even though, I think that this type of data written to disk/flash
without guaranteeing (trusting natural picketing) packed and endianness
is never a good idea.

Cheers,
   Rui

>
> Best regards
>
> Heinrich
>
>>>
>>>   #endif /* _FWU_MDATA_H_ */
>>> --
>>> 2.40.0
>>>
>>
>> Yep, that's a good catch.  The spec defines 'just' the offsets and not
>> the C representation, so those should be indeed packed.
>>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/6] fwu_metadata: make sure structures are packed

[Ilias Apalodimas]

Hi,

On Mon, 29 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:
>
> Hi Heinrich,
> thanks for the review,
> Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
>
> > On 5/3/23 10:06, Ilias Apalodimas wrote:
> >> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:
> >>
> >> Hi Rui,
> >>>
> >>> The fwu metadata in the metadata partitions
> >>> should/are packed to guarantee that the info is
> >>> correct in all platforms. Also the size of them
> >>> are used to calculate the crc32 and that is important
> >>> to get it right.
> >>>
> >>> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> >>> ---
> >>>   include/fwu_mdata.h | 6 +++---
> >>>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
> >>> index 8fda4f4ac225..c61221a91735 100644
> >>> --- a/include/fwu_mdata.h
> >>> +++ b/include/fwu_mdata.h
> >>> @@ -22,7 +22,7 @@ struct fwu_image_bank_info {
> >>>          efi_guid_t  image_uuid;
> >>>          uint32_t accepted;
> >>>          uint32_t reserved;
> >>> -};
> >>> +} __packed;
> >
> > This structure is naturally packed. Why should adding a __packed
> > attribute make a difference?
>
> Agree.
>
> >
> >>>
> >>>   /**
> >>>    * struct fwu_image_entry - information for a particular type of image
> >>> @@ -38,7 +38,7 @@ struct fwu_image_entry {
> >>>          efi_guid_t image_type_uuid;
> >>>          efi_guid_t location_uuid;
> >>>          struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> >>> -};
> >>> +} __packed;
> >
> > ditto
>
> Agree.
>
> >
> >>>
> >>>   /**
> >>>    * struct fwu_mdata - FWU metadata structure for multi-bank updates
> >>> @@ -62,6 +62,6 @@ struct fwu_mdata {
> >>>          uint32_t previous_active_index;
> >>>
> >>>          struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> >>> -};
> >>> +} __packed;
> >
> > Depending on the alignment of efi_guid_t __packed might make a
> > difference here. But as efi_guid_t is defined as 4 aligned I can't see
> > an impact here either.
>
> but efi_guid_t is defined as 8 aligned, right?

No, we have it as 4byte aligned,

> Even though, I think that this type of data written to disk/flash
> without guaranteeing (trusting natural picketing) packed and endianness
> is never a good idea.

I think we should overall define these as packed regardless.  The spec
might change at some point and add a few fields ane even though it
doesn't explicitly requires packing it only mentions offsets.  This is
going to make a difference in code generation, but it's in a path
noone cares about speed.

Thanks
/Ilias

>
> Cheers,
>    Rui
>
> >
> > Best regards
> >
> > Heinrich
> >
> >>>
> >>>   #endif /* _FWU_MDATA_H_ */
> >>> --
> >>> 2.40.0
> >>>
> >>
> >> Yep, that's a good catch.  The spec defines 'just' the offsets and not
> >> the C representation, so those should be indeed packed.
> >>
> >> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/6] fwu_metadata: make sure structures are packed

[Rui Miguel Silva]

Ilias Apalodimas <ilias.apalodimas@linaro.org> writes:

> Hi,
>
> On Mon, 29 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:
>>
>> Hi Heinrich,
>> thanks for the review,
>> Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
>>
>> > On 5/3/23 10:06, Ilias Apalodimas wrote:
>> >> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:
>> >>
>> >> Hi Rui,
>> >>>
>> >>> The fwu metadata in the metadata partitions
>> >>> should/are packed to guarantee that the info is
>> >>> correct in all platforms. Also the size of them
>> >>> are used to calculate the crc32 and that is important
>> >>> to get it right.
>> >>>
>> >>> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
>> >>> ---
>> >>>   include/fwu_mdata.h | 6 +++---
>> >>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> >>>
>> >>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
>> >>> index 8fda4f4ac225..c61221a91735 100644
>> >>> --- a/include/fwu_mdata.h
>> >>> +++ b/include/fwu_mdata.h
>> >>> @@ -22,7 +22,7 @@ struct fwu_image_bank_info {
>> >>>          efi_guid_t  image_uuid;
>> >>>          uint32_t accepted;
>> >>>          uint32_t reserved;
>> >>> -};
>> >>> +} __packed;
>> >
>> > This structure is naturally packed. Why should adding a __packed
>> > attribute make a difference?
>>
>> Agree.
>>
>> >
>> >>>
>> >>>   /**
>> >>>    * struct fwu_image_entry - information for a particular type of image
>> >>> @@ -38,7 +38,7 @@ struct fwu_image_entry {
>> >>>          efi_guid_t image_type_uuid;
>> >>>          efi_guid_t location_uuid;
>> >>>          struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
>> >>> -};
>> >>> +} __packed;
>> >
>> > ditto
>>
>> Agree.
>>
>> >
>> >>>
>> >>>   /**
>> >>>    * struct fwu_mdata - FWU metadata structure for multi-bank updates
>> >>> @@ -62,6 +62,6 @@ struct fwu_mdata {
>> >>>          uint32_t previous_active_index;
>> >>>
>> >>>          struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
>> >>> -};
>> >>> +} __packed;
>> >
>> > Depending on the alignment of efi_guid_t __packed might make a
>> > difference here. But as efi_guid_t is defined as 4 aligned I can't see
>> > an impact here either.
>>
>> but efi_guid_t is defined as 8 aligned, right?

>
> No, we have it as 4byte aligned,

Sorry, was looking at an relative old branch. (only with this:
1dd705cf99 efi: use 32-bit alignment for efi_guid_t in v2023.04 it
changed to 4 aligned)

and, please note, that there is a definition of efi_guid_t
as aligned 8 in eficapsule tool:
https://elixir.bootlin.com/u-boot/latest/source/tools/eficapsule.h#L27

>
>> Even though, I think that this type of data written to disk/flash
>> without guaranteeing (trusting natural picketing) packed and endianness
>> is never a good idea.
>
> I think we should overall define these as packed regardless.  The spec
> might change at some point and add a few fields ane even though it
> doesn't explicitly requires packing it only mentions offsets.  This is
> going to make a difference in code generation, but it's in a path
> noone cares about speed.

Agree. Thanks for the feedback.

Cheers,
  Rui
  
>
> Thanks
> /Ilias
>
>>
>> Cheers,
>>    Rui
>>
>> >
>> > Best regards
>> >
>> > Heinrich
>> >
>> >>>
>> >>>   #endif /* _FWU_MDATA_H_ */
>> >>> --
>> >>> 2.40.0
>> >>>
>> >>
>> >> Yep, that's a good catch.  The spec defines 'just' the offsets and not
>> >> the C representation, so those should be indeed packed.
>> >>
>> >> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 2/6] nvmxip: move header to include

[Rui Miguel Silva]

Move header to include to allow external code
to get the internal bdev structures to access
block device operations.

as at it, just add the UCLASS_NVMXIP string
so we get the correct output in partitions
listing.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 {drivers/mtd/nvmxip => include}/nvmxip.h | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename {drivers/mtd/nvmxip => include}/nvmxip.h (100%)

diff --git a/drivers/mtd/nvmxip/nvmxip.h b/include/nvmxip.h
similarity index 100%
rename from drivers/mtd/nvmxip/nvmxip.h
rename to include/nvmxip.h
-- 
2.40.0

Re: [PATCH 2/6] nvmxip: move header to include

[Heinrich Schuchardt]

On 5/2/23 15:11, Rui Miguel Silva wrote:
> Move header to include to allow external code
> to get the internal bdev structures to access
> block device operations.
>
> as at it, just add the UCLASS_NVMXIP string
> so we get the correct output in partitions
> listing.
>
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> ---
>   {drivers/mtd/nvmxip => include}/nvmxip.h | 0
>   1 file changed, 0 insertions(+), 0 deletions(-)
>   rename {drivers/mtd/nvmxip => include}/nvmxip.h (100%)
>
> diff --git a/drivers/mtd/nvmxip/nvmxip.h b/include/nvmxip.h
> similarity index 100%
> rename from drivers/mtd/nvmxip/nvmxip.h
> rename to include/nvmxip.h

We expect the code to compile after each single patch. Otherwise
bisecting will not work.

You cannot move an include without adjusting all references, e.g.
test/dm/nvmxip.c:20:#include "../../drivers/mtd/nvmxip/nvmxip.h"

Best regards

Heinrich

Re: [PATCH 2/6] nvmxip: move header to include

[Rui Miguel Silva]

Hi Heinrich,
Thanks for the review.

Heinrich Schuchardt <xypron.glpk@gmx.de> writes:

> On 5/2/23 15:11, Rui Miguel Silva wrote:
>> Move header to include to allow external code
>> to get the internal bdev structures to access
>> block device operations.
>>
>> as at it, just add the UCLASS_NVMXIP string
>> so we get the correct output in partitions
>> listing.
>>
>> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
>> ---
>>   {drivers/mtd/nvmxip => include}/nvmxip.h | 0
>>   1 file changed, 0 insertions(+), 0 deletions(-)
>>   rename {drivers/mtd/nvmxip => include}/nvmxip.h (100%)
>>
>> diff --git a/drivers/mtd/nvmxip/nvmxip.h b/include/nvmxip.h
>> similarity index 100%
>> rename from drivers/mtd/nvmxip/nvmxip.h
>> rename to include/nvmxip.h
>
> We expect the code to compile after each single patch. Otherwise
> bisecting will not work.
>
> You cannot move an include without adjusting all references, e.g.
> test/dm/nvmxip.c:20:#include "../../drivers/mtd/nvmxip/nvmxip.h"

Hrrr, thanks for noticing this. I will fix it in v2.

Cheers,
  Rui
>
> Best regards
>
> Heinrich

[PATCH 3/6] corstone1000: add fwu-metadata store info

[Rui Miguel Silva]

Add fwu-mdata node and handle for the reference
nvmxip-qspi.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 arch/arm/dts/corstone1000.dtsi | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/corstone1000.dtsi b/arch/arm/dts/corstone1000.dtsi
index 533dfdf8e1ca..1e0ec075e4cd 100644
--- a/arch/arm/dts/corstone1000.dtsi
+++ b/arch/arm/dts/corstone1000.dtsi
@@ -38,7 +38,7 @@
 		reg = <0x88200000 0x77e00000>;
 	};
 
-	nvmxip-qspi@08000000 {
+	nvmxip: nvmxip-qspi@08000000 {
 		compatible = "nvmxip,qspi";
 		reg = <0x08000000 0x2000000>;
 		lba_shift = <9>;
@@ -106,6 +106,11 @@
 		method = "smc";
 	};
 
+	fwu-mdata {
+		compatible = "u-boot,fwu-mdata-gpt";
+		fwu-mdata-store = <&nvmxip>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.40.0

[PATCH 4/6] corstone1000: add boot index

[Rui Miguel Silva]

it is expected that the firmware that runs before
u-boot somehow provide the information of the bank
for now we will fetch the info from the metadata
since the Secure enclave is the one responsible for
this information.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 board/armltd/corstone1000/corstone1000.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/board/armltd/corstone1000/corstone1000.c b/board/armltd/corstone1000/corstone1000.c
index 6ec8e6144fb4..1bead7a0a8b4 100644
--- a/board/armltd/corstone1000/corstone1000.c
+++ b/board/armltd/corstone1000/corstone1000.c
@@ -8,6 +8,8 @@
 #include <common.h>
 #include <cpu_func.h>
 #include <dm.h>
+#include <env.h>
+#include <env.h>
 #include <netdev.h>
 #include <dm/platform_data/serial_pl01x.h>
 #include <asm/armv8/mmu.h>
@@ -87,6 +89,20 @@ int dram_init_banksize(void)
 	return 0;
 }
 
-void reset_cpu(void)
+void fwu_plat_get_bootidx(uint *boot_idx)
 {
+	int ret;
+
+	/*
+	 * in our platform, the Secure Enclave is the one who controls
+	 * all the boot tries and status, so, every time we get here
+	 * we know that the we are booting from the active index
+	 */
+	ret = fwu_get_active_index(boot_idx);
+	if (ret < 0) {
+		*boot_idx = CONFIG_FWU_NUM_BANKS;
+		log_err("corstone1000: failed to read active index\n");
+	}
+
+	return ret;
 }
-- 
2.40.0

[PATCH 5/6] corstone1000: set kernel_addr based on boot_idx

[Rui Miguel Silva]

We need to distinguish between boot banks and from which
partition to load the kernel+initramfs to memory.

For that, fetch the boot index, fetch the correspondent
partition, calculate the correct kernel address and
then set the env variable kernel_addr with that value.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 board/armltd/corstone1000/corstone1000.c | 56 +++++++++++++++++++++++-
 configs/corstone1000_defconfig           |  1 +
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/board/armltd/corstone1000/corstone1000.c b/board/armltd/corstone1000/corstone1000.c
index 1bead7a0a8b4..a4567449f1be 100644
--- a/board/armltd/corstone1000/corstone1000.c
+++ b/board/armltd/corstone1000/corstone1000.c
@@ -5,16 +5,24 @@
  * Rui Miguel Silva <rui.silva@linaro.org>
  */
 
+#include <blk.h>
 #include <common.h>
 #include <cpu_func.h>
 #include <dm.h>
 #include <env.h>
-#include <env.h>
 #include <netdev.h>
+#include <nvmxip.h>
+#include <part.h>
 #include <dm/platform_data/serial_pl01x.h>
 #include <asm/armv8/mmu.h>
 #include <asm/global_data.h>
 
+#define CORSTONE1000_KERNEL_PARTS 2
+#define CORSTONE1000_KERNEL_PRIMARY "kernel_primary"
+#define CORSTONE1000_KERNEL_SECONDARY "kernel_secondary"
+
+static int corstone1000_boot_idx;
+
 static struct mm_region corstone1000_mem_map[] = {
 	{
 		/* CVM */
@@ -103,6 +111,52 @@ void fwu_plat_get_bootidx(uint *boot_idx)
 		*boot_idx = CONFIG_FWU_NUM_BANKS;
 		log_err("corstone1000: failed to read active index\n");
 	}
+}
+
+int board_late_init(void)
+{
+	struct disk_partition part_info;
+	struct udevice *dev, *bdev;
+	struct nvmxip_plat *plat;
+	struct blk_desc *desc;
+	int ret;
+
+	ret = uclass_first_device_err(UCLASS_NVMXIP, &dev);
+	if (ret < 0) {
+		log_err("Cannot find kernel device\n");
+		return ret;
+	}
+
+	plat = dev_get_plat(dev);
+	device_find_first_child(dev, &bdev);
+	desc = dev_get_uclass_plat(bdev);
+	ret = fwu_get_active_index(&corstone1000_boot_idx);
+	if (ret < 0) {
+		log_err("corstone1000: failed to read boot index\n");
+		return ret;
+	}
+
+	if (!corstone1000_boot_idx)
+		ret = part_get_info_by_name(desc, CORSTONE1000_KERNEL_PRIMARY,
+					    &part_info);
+	else
+		ret = part_get_info_by_name(desc, CORSTONE1000_KERNEL_SECONDARY,
+					    &part_info);
+
+	if (ret < 0) {
+		log_err("failed to fetch kernel partition index: %d\n",
+			corstone1000_boot_idx);
+		return ret;
+	}
+
+	ret = 0;
+
+	ret |= env_set_hex("kernel_addr", plat->phys_base +
+			   (part_info.start * part_info.blksz));
+	ret |= env_set_hex("kernel_size", part_info.size * part_info.blksz);
+
+	if (ret < 0)
+		log_err("failed to setup kernel addr and size\n");
 
 	return ret;
 }
diff --git a/configs/corstone1000_defconfig b/configs/corstone1000_defconfig
index 2d391048cd67..5be5335bdfc1 100644
--- a/configs/corstone1000_defconfig
+++ b/configs/corstone1000_defconfig
@@ -20,6 +20,7 @@ CONFIG_CONSOLE_RECORD=y
 CONFIG_LOGLEVEL=7
 # CONFIG_DISPLAY_CPUINFO is not set
 # CONFIG_DISPLAY_BOARDINFO is not set
+CONFIG_BOARD_LATE_INIT=y
 CONFIG_SYS_MAXARGS=64
 CONFIG_SYS_CBSIZE=512
 # CONFIG_CMD_CONSOLE is not set
-- 
2.40.0

[PATCH 6/6] corstone1000: add nvmxip, fwu-mdata and gpt options

[Rui Miguel Silva]

Enable the newest features: nvmxip, fwu-metadata and
gpt. Commands to print the partition info, gpt info
and fwu metadata will be available.

Adjust also env boot script the address of the
bootbank with the new gpt layout, and also remove
the not needed kernel address bank0 and bank1
and retrieve function that would test the bank flag
before and now we are getting the info from the fwu
metadata.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 board/armltd/corstone1000/corstone1000.c   |  1 +
 board/armltd/corstone1000/corstone1000.env | 10 +---------
 configs/corstone1000_defconfig             | 13 ++++++++++++-
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/board/armltd/corstone1000/corstone1000.c b/board/armltd/corstone1000/corstone1000.c
index a4567449f1be..01c80aaf9d77 100644
--- a/board/armltd/corstone1000/corstone1000.c
+++ b/board/armltd/corstone1000/corstone1000.c
@@ -10,6 +10,7 @@
 #include <cpu_func.h>
 #include <dm.h>
 #include <env.h>
+#include <fwu.h>
 #include <netdev.h>
 #include <nvmxip.h>
 #include <part.h>
diff --git a/board/armltd/corstone1000/corstone1000.env b/board/armltd/corstone1000/corstone1000.env
index b24ff07fc6bd..ee318b1b1c30 100644
--- a/board/armltd/corstone1000/corstone1000.env
+++ b/board/armltd/corstone1000/corstone1000.env
@@ -1,13 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0+ */
 
 usb_pgood_delay=250
-boot_bank_flag=0x08002000
-kernel_addr_bank_0=0x083EE000
-kernel_addr_bank_1=0x0936E000
-retrieve_kernel_load_addr=
-	if itest.l *${boot_bank_flag} == 0; then
-		setenv kernel_addr $kernel_addr_bank_0;
-	else
-		setenv kernel_addr $kernel_addr_bank_1;
-	fi;
+boot_bank_flag=0x08005006
 kernel_addr_r=0x88200000
diff --git a/configs/corstone1000_defconfig b/configs/corstone1000_defconfig
index 5be5335bdfc1..a8a79fd10568 100644
--- a/configs/corstone1000_defconfig
+++ b/configs/corstone1000_defconfig
@@ -15,7 +15,7 @@ CONFIG_DISTRO_DEFAULTS=y
 CONFIG_BOOTDELAY=3
 CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="console=ttyAMA0 loglevel=9 ip=dhcp earlyprintk"
-CONFIG_BOOTCOMMAND="run retrieve_kernel_load_addr; echo Loading kernel from $kernel_addr to memory ... ; loadm $kernel_addr $kernel_addr_r 0xc00000; usb start; usb reset; run distro_bootcmd; bootefi $kernel_addr_r $fdtcontroladdr;"
+CONFIG_BOOTCOMMAND="echo Loading kernel from $kernel_addr to memory ... ; loadm $kernel_addr $kernel_addr_r 0xc00000; usb start; usb reset; run distro_bootcmd; bootefi $kernel_addr_r $fdtcontroladdr;"
 CONFIG_CONSOLE_RECORD=y
 CONFIG_LOGLEVEL=7
 # CONFIG_DISPLAY_CPUINFO is not set
@@ -24,11 +24,16 @@ CONFIG_BOARD_LATE_INIT=y
 CONFIG_SYS_MAXARGS=64
 CONFIG_SYS_CBSIZE=512
 # CONFIG_CMD_CONSOLE is not set
+CONFIG_CMD_FWU_METADATA=y
 CONFIG_CMD_BOOTZ=y
 CONFIG_SYS_BOOTM_LEN=0x800000
 # CONFIG_CMD_XIMG is not set
+CONFIG_CMD_NVMXIP=y
+CONFIG_CMD_GPT=y
+# CONFIG_RANDOM_UUID is not set
 CONFIG_CMD_LOADM=y
 # CONFIG_CMD_LOADS is not set
+CONFIG_CMD_MMC=y
 CONFIG_CMD_USB=y
 # CONFIG_CMD_SETEXPR is not set
 # CONFIG_CMD_NFS is not set
@@ -40,6 +45,8 @@ CONFIG_OF_CONTROL=y
 CONFIG_VERSION_VARIABLE=y
 CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_REGMAP=y
+CONFIG_FWU_MDATA=y
+CONFIG_FWU_MDATA_GPT_BLK=y
 CONFIG_MISC=y
 # CONFIG_MMC is not set
 CONFIG_NVMXIP_QSPI=y
@@ -51,6 +58,10 @@ CONFIG_RAM=y
 CONFIG_DM_RTC=y
 CONFIG_RTC_EMULATION=y
 CONFIG_DM_SERIAL=y
+CONFIG_SYSRESET=y
 CONFIG_USB=y
 CONFIG_USB_ISP1760=y
+CONFIG_EFI_CAPSULE_ON_DISK=y
+CONFIG_EFI_IGNORE_OSINDICATIONS=y
+CONFIG_FWU_MULTI_BANK_UPDATE=y
 CONFIG_ERRNO_STR=y
-- 
2.40.0

Re: [PATCH 0/6] corstone1000: fwu metadata and GPT

[Rui Miguel Silva]

Hi,
Rui Miguel Silva <rui.silva@linaro.org> writes:
> Now that the nvmxip block driver is merged we can add on top
> of it the platform code to use GPT and FWU metadata in the
> Corstone1000.
>
> But first, push 2 fixes that are needed to make all this work:
>  - move nvmxip header to include
>  - setup fwu metadata structures as packed (we have a 32bit
>    writer - Secure enclave Cortex-M0 and a 64bit reader host
>    Cortex-A35)

Gentle ping.

Cheers,
   Rui
>
> Cheers,
>      Rui
>
> Rui Miguel Silva (6):
>   fwu_metadata: make sure structures are packed
>   nvmxip: move header to include
>   corstone1000: add fwu-metadata store info
>   corstone1000: add boot index
>   corstone1000: set kernel_addr based on boot_idx
>   corstone1000: add nvmxip, fwu-mdata and gpt options
>
>  arch/arm/dts/corstone1000.dtsi             |  7 ++-
>  board/armltd/corstone1000/corstone1000.c   | 73 +++++++++++++++++++++-
>  board/armltd/corstone1000/corstone1000.env | 10 +--
>  configs/corstone1000_defconfig             | 14 ++++-
>  include/fwu_mdata.h                        |  6 +-
>  {drivers/mtd/nvmxip => include}/nvmxip.h   |  0
>  6 files changed, 95 insertions(+), 15 deletions(-)
>  rename {drivers/mtd/nvmxip => include}/nvmxip.h (100%)
>
> -- 
> 2.40.0

Re: [PATCH 0/6] corstone1000: fwu metadata and GPT

[Rui Miguel Silva]

Hi,
Rui Miguel Silva <rui.silva@linaro.org> writes:
> Hi,
> Rui Miguel Silva <rui.silva@linaro.org> writes:
>> Now that the nvmxip block driver is merged we can add on top
>> of it the platform code to use GPT and FWU metadata in the
>> Corstone1000.
>>
>> But first, push 2 fixes that are needed to make all this work:
>>  - move nvmxip header to include
>>  - setup fwu metadata structures as packed (we have a 32bit
>>    writer - Secure enclave Cortex-M0 and a 64bit reader host
>>    Cortex-A35)
>
> Gentle ping.
>
> Cheers,
>    Rui

It would be great to have some feedback on this ones.
Many thanks.

Cheers,
    Rui
    
>>
>> Cheers,
>>      Rui
>>
>> Rui Miguel Silva (6):
>>   fwu_metadata: make sure structures are packed
>>   nvmxip: move header to include
>>   corstone1000: add fwu-metadata store info
>>   corstone1000: add boot index
>>   corstone1000: set kernel_addr based on boot_idx
>>   corstone1000: add nvmxip, fwu-mdata and gpt options
>>
>>  arch/arm/dts/corstone1000.dtsi             |  7 ++-
>>  board/armltd/corstone1000/corstone1000.c   | 73 +++++++++++++++++++++-
>>  board/armltd/corstone1000/corstone1000.env | 10 +--
>>  configs/corstone1000_defconfig             | 14 ++++-
>>  include/fwu_mdata.h                        |  6 +-
>>  {drivers/mtd/nvmxip => include}/nvmxip.h   |  0
>>  6 files changed, 95 insertions(+), 15 deletions(-)
>>  rename {drivers/mtd/nvmxip => include}/nvmxip.h (100%)
>>
>> -- 
>> 2.40.0