Re: [PATCH v4 10/12] bloblist: Adjust the bloblist header
On Thu, 28 Dec 2023 at 15:37, Simon Glass wrote:
>
> Hi Ilias,
>
> On Thu, Dec 28, 2023 at 7:37 AM Ilias Apalodimas
> wrote:
> >
> > Hi Raymond,
> >
> > [...]
> >
> > >
> > > void bloblist_show_list(void)
> > > @@ -463,7 +477,7 @@ void bloblist_reloc(void *to, uint to_size, void
> > > *from, uint from_size)
> > >
> > > memcpy(to, from, from_size);
> > > hdr = to;
> > > - hdr->size = to_size;
> > > + hdr->total_size = to_size;
> > > }
> > >
> > > int bloblist_init(void)
> > > @@ -493,7 +507,7 @@ int bloblist_init(void)
> > > addr, ret);
> > > } else {
> > > /* Get the real size, if it is not what we
> > > expected */
> > > - size = gd->bloblist->size;
> > > + size = gd->bloblist->total_size;
> > > }
> > > }
> > > if (ret) {
> > > diff --git a/include/bloblist.h b/include/bloblist.h
> > > index 7024d7bf9e..4ec4b3d449 100644
> > > --- a/include/bloblist.h
> > > +++ b/include/bloblist.h
> > > @@ -166,32 +166,33 @@ enum bloblist_tag_t {
> > > * from the last.
> > > *
> > > * @magic: BLOBLIST_MAGIC
> > > + * @chksum: checksum for the entire bloblist allocated area. Since any
> > > of the
> > > + * blobs can be altered after being created, this checksum is only
> > > valid
> > > + * when the bloblist is finalized before jumping to the next stage
> > > of boot.
> > > + * This is the value needed to make all checksummed bytes sum to 0
> > > * @version: BLOBLIST_VERSION
> > > * @hdr_size: Size of this header, normally sizeof(struct bloblist_hdr).
> > > The
> > > * first bloblist_rec starts at this offset from the start of the
> > > header
> > > - * @flags: Space for BLOBLISTF... flags (none yet)
> > > - * @size: Total size of the bloblist (non-zero if valid) including this
> > > header.
> > > - * The bloblist extends for this many bytes from the start of this
> > > header.
> > > - * When adding new records, the bloblist can grow up to this size.
> > > - * @alloced: Total size allocated so far for this bloblist. This starts
> > > out as
> > > + * @align_log2: Power of two of the maximum alignment required by this
> > > list
> > > + * @used_size: Size allocated so far for this bloblist. This starts out
> > > as
> > > * sizeof(bloblist_hdr) since we need at least that much space to
> > > store a
> > > * valid bloblist
> > > + * @total_size: The number of total bytes that the bloblist can occupy.
> > > + * Any blob producer must check if there is sufficient space before
> > > adding
> > > + * a record to the bloblist.
> > > + * @flags: Space for BLOBLISTF... flags (none yet)
> > > * @spare: Spare space (for future use)
> > > - * @chksum: checksum for the entire bloblist allocated area. Since any
> > > of the
> > > - * blobs can be altered after being created, this checksum is only
> > > valid
> > > - * when the bloblist is finalised before jumping to the next stage
> > > of boot.
> > > - * This is the value needed to make all checksummed bytes sum to 0
> > > */
> > > struct bloblist_hdr {
> > > u32 magic;
> > > - u32 version;
> > > - u32 hdr_size;
> > > + u8 chksum;
> > > + u8 version;
> > > + u8 hdr_size;
> > > + u8 align_log2;
> > > + u32 used_size;
> > > + u32 total_size;
> > > u32 flags;
> > > -
> > > - u32 size;
> > > - u32 alloced;
> > > u32 spare;
> > > - u32 chksum;
> > > };
> >
> > The patch generally looks ok, but while we are renaming things, can't
> > we just copy what the spec says instead of slightly changing the
> > names?
> >
> > With this applied we end up with
> > struct bloblist_hdr {
> > u32 magic;
> > u8 chksum;
> > u8 version;
> > u8 hdr_size;
> > u8 align_log2;
> > u32 used_size;
> > u32 total_size;
> > u32 flags;
> > u32 spare;
> > };
> >
> > Can you at least rename the ones you touch here properly?
> > - Drop the patch that renames spare to _spare
> > - used_size -> size
> > - total_size -> max_size (which btw mean different things)
> > - spare -> reserved
>
> Yes that would be good, but since the spec only recently changed the
> names, how about a 'rename' patch on top, if that is easier?
>
Sure I dont mind,
Reviewed-by: Ilias Apalodimas
> I was hoping that this series could be applied as is, or close to it.
> It looks like a good base to me.
>
> Regards,
> Simon
Re: [PATCH v4 10/12] bloblist: Adjust the bloblist header
Hi Ilias,
On Thu, 28 Dec 2023 at 02:37, Ilias Apalodimas
wrote:
> Hi Raymond,
>
> [...]
>
> >
> > void bloblist_show_list(void)
> > @@ -463,7 +477,7 @@ void bloblist_reloc(void *to, uint to_size, void
> *from, uint from_size)
> >
> > memcpy(to, from, from_size);
> > hdr = to;
> > - hdr->size = to_size;
> > + hdr->total_size = to_size;
> > }
> >
> > int bloblist_init(void)
> > @@ -493,7 +507,7 @@ int bloblist_init(void)
> > addr, ret);
> > } else {
> > /* Get the real size, if it is not what we
> expected */
> > - size = gd->bloblist->size;
> > + size = gd->bloblist->total_size;
> > }
> > }
> > if (ret) {
> > diff --git a/include/bloblist.h b/include/bloblist.h
> > index 7024d7bf9e..4ec4b3d449 100644
> > --- a/include/bloblist.h
> > +++ b/include/bloblist.h
> > @@ -166,32 +166,33 @@ enum bloblist_tag_t {
> > * from the last.
> > *
> > * @magic: BLOBLIST_MAGIC
> > + * @chksum: checksum for the entire bloblist allocated area. Since any
> of the
> > + * blobs can be altered after being created, this checksum is only
> valid
> > + * when the bloblist is finalized before jumping to the next stage
> of boot.
> > + * This is the value needed to make all checksummed bytes sum to 0
> > * @version: BLOBLIST_VERSION
> > * @hdr_size: Size of this header, normally sizeof(struct
> bloblist_hdr). The
> > * first bloblist_rec starts at this offset from the start of the
> header
> > - * @flags: Space for BLOBLISTF... flags (none yet)
> > - * @size: Total size of the bloblist (non-zero if valid) including this
> header.
> > - * The bloblist extends for this many bytes from the start of this
> header.
> > - * When adding new records, the bloblist can grow up to this size.
> > - * @alloced: Total size allocated so far for this bloblist. This starts
> out as
> > + * @align_log2: Power of two of the maximum alignment required by this
> list
> > + * @used_size: Size allocated so far for this bloblist. This starts out
> as
> > * sizeof(bloblist_hdr) since we need at least that much space to
> store a
> > * valid bloblist
> > + * @total_size: The number of total bytes that the bloblist can occupy.
> > + * Any blob producer must check if there is sufficient space before
> adding
> > + * a record to the bloblist.
> > + * @flags: Space for BLOBLISTF... flags (none yet)
> > * @spare: Spare space (for future use)
> > - * @chksum: checksum for the entire bloblist allocated area. Since any
> of the
> > - * blobs can be altered after being created, this checksum is only
> valid
> > - * when the bloblist is finalised before jumping to the next stage
> of boot.
> > - * This is the value needed to make all checksummed bytes sum to 0
> > */
> > struct bloblist_hdr {
> > u32 magic;
> > - u32 version;
> > - u32 hdr_size;
> > + u8 chksum;
> > + u8 version;
> > + u8 hdr_size;
> > + u8 align_log2;
> > + u32 used_size;
> > + u32 total_size;
> > u32 flags;
> > -
> > - u32 size;
> > - u32 alloced;
> > u32 spare;
> > - u32 chksum;
> > };
>
> The patch generally looks ok, but while we are renaming things, can't
> we just copy what the spec says instead of slightly changing the
> names?
>
> With this applied we end up with
> struct bloblist_hdr {
> u32 magic;
> u8 chksum;
> u8 version;
> u8 hdr_size;
> u8 align_log2;
> u32 used_size;
> u32 total_size;
> u32 flags;
> u32 spare;
> };
>
> Can you at least rename the ones you touch here properly?
> - Drop the patch that renames spare to _spare
> - used_size -> size
> - total_size -> max_size (which btw mean different things)
> - spare -> reserved
>
> I was been told by Simon for the naming changes of `used_size` and
`total_size` at:
https://github.com/FirmwareHandoff/firmware_handoff/pull/25
I will prefer to keep this minor fix until spec v1.0 is released since the
spec
is still continuously being updated without upgrading the revision number.
When a v1.0 spec is released I will update the TF-A, OP-TEE and U-Boot
together.
[...]
Thanks and regards,
Raymond
Re: [PATCH v4 10/12] bloblist: Adjust the bloblist header
Hi Ilias,
On Thu, Dec 28, 2023 at 7:37 AM Ilias Apalodimas
wrote:
>
> Hi Raymond,
>
> [...]
>
> >
> > void bloblist_show_list(void)
> > @@ -463,7 +477,7 @@ void bloblist_reloc(void *to, uint to_size, void *from,
> > uint from_size)
> >
> > memcpy(to, from, from_size);
> > hdr = to;
> > - hdr->size = to_size;
> > + hdr->total_size = to_size;
> > }
> >
> > int bloblist_init(void)
> > @@ -493,7 +507,7 @@ int bloblist_init(void)
> > addr, ret);
> > } else {
> > /* Get the real size, if it is not what we expected
> > */
> > - size = gd->bloblist->size;
> > + size = gd->bloblist->total_size;
> > }
> > }
> > if (ret) {
> > diff --git a/include/bloblist.h b/include/bloblist.h
> > index 7024d7bf9e..4ec4b3d449 100644
> > --- a/include/bloblist.h
> > +++ b/include/bloblist.h
> > @@ -166,32 +166,33 @@ enum bloblist_tag_t {
> > * from the last.
> > *
> > * @magic: BLOBLIST_MAGIC
> > + * @chksum: checksum for the entire bloblist allocated area. Since any of
> > the
> > + * blobs can be altered after being created, this checksum is only
> > valid
> > + * when the bloblist is finalized before jumping to the next stage of
> > boot.
> > + * This is the value needed to make all checksummed bytes sum to 0
> > * @version: BLOBLIST_VERSION
> > * @hdr_size: Size of this header, normally sizeof(struct bloblist_hdr).
> > The
> > * first bloblist_rec starts at this offset from the start of the
> > header
> > - * @flags: Space for BLOBLISTF... flags (none yet)
> > - * @size: Total size of the bloblist (non-zero if valid) including this
> > header.
> > - * The bloblist extends for this many bytes from the start of this
> > header.
> > - * When adding new records, the bloblist can grow up to this size.
> > - * @alloced: Total size allocated so far for this bloblist. This starts
> > out as
> > + * @align_log2: Power of two of the maximum alignment required by this list
> > + * @used_size: Size allocated so far for this bloblist. This starts out as
> > * sizeof(bloblist_hdr) since we need at least that much space to
> > store a
> > * valid bloblist
> > + * @total_size: The number of total bytes that the bloblist can occupy.
> > + * Any blob producer must check if there is sufficient space before
> > adding
> > + * a record to the bloblist.
> > + * @flags: Space for BLOBLISTF... flags (none yet)
> > * @spare: Spare space (for future use)
> > - * @chksum: checksum for the entire bloblist allocated area. Since any of
> > the
> > - * blobs can be altered after being created, this checksum is only
> > valid
> > - * when the bloblist is finalised before jumping to the next stage of
> > boot.
> > - * This is the value needed to make all checksummed bytes sum to 0
> > */
> > struct bloblist_hdr {
> > u32 magic;
> > - u32 version;
> > - u32 hdr_size;
> > + u8 chksum;
> > + u8 version;
> > + u8 hdr_size;
> > + u8 align_log2;
> > + u32 used_size;
> > + u32 total_size;
> > u32 flags;
> > -
> > - u32 size;
> > - u32 alloced;
> > u32 spare;
> > - u32 chksum;
> > };
>
> The patch generally looks ok, but while we are renaming things, can't
> we just copy what the spec says instead of slightly changing the
> names?
>
> With this applied we end up with
> struct bloblist_hdr {
> u32 magic;
> u8 chksum;
> u8 version;
> u8 hdr_size;
> u8 align_log2;
> u32 used_size;
> u32 total_size;
> u32 flags;
> u32 spare;
> };
>
> Can you at least rename the ones you touch here properly?
> - Drop the patch that renames spare to _spare
> - used_size -> size
> - total_size -> max_size (which btw mean different things)
> - spare -> reserved
Yes that would be good, but since the spec only recently changed the
names, how about a 'rename' patch on top, if that is easier?
I was hoping that this series could be applied as is, or close to it.
It looks like a good base to me.
Regards,
Simon
Re: [PATCH v4 10/12] bloblist: Adjust the bloblist header
Hi Raymond,
[...]
>
> void bloblist_show_list(void)
> @@ -463,7 +477,7 @@ void bloblist_reloc(void *to, uint to_size, void *from,
> uint from_size)
>
> memcpy(to, from, from_size);
> hdr = to;
> - hdr->size = to_size;
> + hdr->total_size = to_size;
> }
>
> int bloblist_init(void)
> @@ -493,7 +507,7 @@ int bloblist_init(void)
> addr, ret);
> } else {
> /* Get the real size, if it is not what we expected */
> - size = gd->bloblist->size;
> + size = gd->bloblist->total_size;
> }
> }
> if (ret) {
> diff --git a/include/bloblist.h b/include/bloblist.h
> index 7024d7bf9e..4ec4b3d449 100644
> --- a/include/bloblist.h
> +++ b/include/bloblist.h
> @@ -166,32 +166,33 @@ enum bloblist_tag_t {
> * from the last.
> *
> * @magic: BLOBLIST_MAGIC
> + * @chksum: checksum for the entire bloblist allocated area. Since any of the
> + * blobs can be altered after being created, this checksum is only valid
> + * when the bloblist is finalized before jumping to the next stage of
> boot.
> + * This is the value needed to make all checksummed bytes sum to 0
> * @version: BLOBLIST_VERSION
> * @hdr_size: Size of this header, normally sizeof(struct bloblist_hdr). The
> * first bloblist_rec starts at this offset from the start of the header
> - * @flags: Space for BLOBLISTF... flags (none yet)
> - * @size: Total size of the bloblist (non-zero if valid) including this
> header.
> - * The bloblist extends for this many bytes from the start of this
> header.
> - * When adding new records, the bloblist can grow up to this size.
> - * @alloced: Total size allocated so far for this bloblist. This starts out
> as
> + * @align_log2: Power of two of the maximum alignment required by this list
> + * @used_size: Size allocated so far for this bloblist. This starts out as
> * sizeof(bloblist_hdr) since we need at least that much space to store a
> * valid bloblist
> + * @total_size: The number of total bytes that the bloblist can occupy.
> + * Any blob producer must check if there is sufficient space before
> adding
> + * a record to the bloblist.
> + * @flags: Space for BLOBLISTF... flags (none yet)
> * @spare: Spare space (for future use)
> - * @chksum: checksum for the entire bloblist allocated area. Since any of the
> - * blobs can be altered after being created, this checksum is only valid
> - * when the bloblist is finalised before jumping to the next stage of
> boot.
> - * This is the value needed to make all checksummed bytes sum to 0
> */
> struct bloblist_hdr {
> u32 magic;
> - u32 version;
> - u32 hdr_size;
> + u8 chksum;
> + u8 version;
> + u8 hdr_size;
> + u8 align_log2;
> + u32 used_size;
> + u32 total_size;
> u32 flags;
> -
> - u32 size;
> - u32 alloced;
> u32 spare;
> - u32 chksum;
> };
The patch generally looks ok, but while we are renaming things, can't
we just copy what the spec says instead of slightly changing the
names?
With this applied we end up with
struct bloblist_hdr {
u32 magic;
u8 chksum;
u8 version;
u8 hdr_size;
u8 align_log2;
u32 used_size;
u32 total_size;
u32 flags;
u32 spare;
};
Can you at least rename the ones you touch here properly?
- Drop the patch that renames spare to _spare
- used_size -> size
- total_size -> max_size (which btw mean different things)
- spare -> reserved
Thanks
/Ilias
[...]
Thanks
/Ilias
[PATCH v4 10/12] bloblist: Adjust the bloblist header
From: Simon Glass
The v0.9 spec provides for a 24-byte header. Update the implementation
to match this.
Rename the fields of the bloblist header to align to the spec.
Adds an alignment field into the bloblist header.
Update the related bloblist APIs and UT testcases.
Signed-off-by: Simon Glass
Co-developed-by: Raymond Mao
Signed-off-by: Raymond Mao
---
Changes in v3
- Update the bloblist header to align to FW handoff spec up to commit 3592349.
- Update the related testcases.
Changes in v4
- Patch #14 from v3 is squashed into this patch.
common/bloblist.c | 86 +++---
include/bloblist.h | 44 ++--
test/bloblist.c| 37 ++--
3 files changed, 95 insertions(+), 72 deletions(-)
diff --git a/common/bloblist.c b/common/bloblist.c
index 1c97d61e4a..6e019087ff 100644
--- a/common/bloblist.c
+++ b/common/bloblist.c
@@ -80,7 +80,7 @@ const char *bloblist_tag_name(enum bloblist_tag_t tag)
static struct bloblist_rec *bloblist_first_blob(struct bloblist_hdr *hdr)
{
- if (hdr->alloced <= hdr->hdr_size)
+ if (hdr->used_size <= hdr->hdr_size)
return NULL;
return (struct bloblist_rec *)((void *)hdr + hdr->hdr_size);
}
@@ -119,7 +119,7 @@ static struct bloblist_rec *bloblist_next_blob(struct
bloblist_hdr *hdr,
{
ulong offset = bloblist_blob_end_ofs(hdr, rec);
- if (offset >= hdr->alloced)
+ if (offset >= hdr->used_size)
return NULL;
return (struct bloblist_rec *)((void *)hdr + offset);
}
@@ -156,9 +156,9 @@ static int bloblist_addrec(uint tag, int size, int
align_log2,
align_log2 = BLOBLIST_BLOB_ALIGN_LOG2;
/* Figure out where the new data will start */
- data_start = map_to_sysmem(hdr) + hdr->alloced + sizeof(*rec);
+ data_start = map_to_sysmem(hdr) + hdr->used_size + sizeof(*rec);
- /* Align the address and then calculate the offset from ->alloced */
+ /* Align the address and then calculate the offset from used size */
aligned_start = ALIGN(data_start, 1U << align_log2) - data_start;
/* If we need to create a dummy record, create it */
@@ -172,19 +172,20 @@ static int bloblist_addrec(uint tag, int size, int
align_log2,
return log_msg_ret("void", ret);
/* start the record after that */
- data_start = map_to_sysmem(hdr) + hdr->alloced + sizeof(*vrec);
+ data_start = map_to_sysmem(hdr) + hdr->used_size +
sizeof(*vrec);
}
/* Calculate the new allocated total */
new_alloced = data_start - map_to_sysmem(hdr) +
ALIGN(size, 1U << align_log2);
- if (new_alloced > hdr->size) {
- log_err("Failed to allocate %x bytes size=%x, need size=%x\n",
- size, hdr->size, new_alloced);
+ if (new_alloced > hdr->total_size) {
+ log_err("Failed to allocate %x bytes\n", size);
+ log_err("Used size=%x, total size=%x\n",
+ hdr->used_size, hdr->total_size);
return log_msg_ret("bloblist add", -ENOSPC);
}
- rec = (void *)hdr + hdr->alloced;
+ rec = (void *)hdr + hdr->used_size;
rec->tag_and_hdr_size = tag | sizeof(*rec) << BLOBLISTR_HDR_SIZE_SHIFT;
rec->size = size;
@@ -192,7 +193,7 @@ static int bloblist_addrec(uint tag, int size, int
align_log2,
/* Zero the record data */
memset((void *)rec + rec_hdr_size(rec), '\0', rec->size);
- hdr->alloced = new_alloced;
+ hdr->used_size = new_alloced;
*recp = rec;
return 0;
@@ -287,29 +288,30 @@ static int bloblist_resize_rec(struct bloblist_hdr *hdr,
int new_size)
{
int expand_by; /* Number of bytes to expand by (-ve to contract) */
- int new_alloced;/* New value for @hdr->alloced */
+ int new_alloced;
ulong next_ofs; /* Offset of the record after @rec */
expand_by = ALIGN(new_size - rec->size, BLOBLIST_BLOB_ALIGN);
- new_alloced = ALIGN(hdr->alloced + expand_by, BLOBLIST_BLOB_ALIGN);
+ new_alloced = ALIGN(hdr->used_size + expand_by, BLOBLIST_BLOB_ALIGN);
if (new_size < 0) {
log_debug("Attempt to shrink blob size below 0 (%x)\n",
new_size);
return log_msg_ret("size", -EINVAL);
}
- if (new_alloced > hdr->size) {
- log_err("Failed to allocate %x bytes size=%x, need size=%x\n",
- new_size, hdr->size, new_alloced);
+ if (new_alloced > hdr->total_size) {
+ log_err("Failed to allocate %x bytes\n", new_size);
+ log_err("Used size=%x, total size=%x\n",
+ hdr->used_size, hdr->total_size);
return log_msg_ret("alloc", -ENOSPC);
}
/* Move the following blobs up or down, if
